Wait for git credential helper task

[jonathan-conder-sm]

This is just something I noticed when reading the code (doesn't fix any user-facing bug that I'm aware of). It seems that [1] changed wait(t) to wait(p), which might be a typo? Without wait(t), any error thrown by t will be ignored.

I've folded wait(p) into the do expression, although the helper should already be dead by the time read! returns (unless it closes stdout early for some reason).

[1] 7577ec2

[vtjnash]

This was actually rather badly written. The standard pattern is to do all of the reading in one task and all of the writing in a different task. This implementation currently is chaotic in doing operations in both tasks and in the wrong event order (the read must come first).

Suggested change

	open(cmd, "r+") do p
	# Provide the helper with the credential information we know
	write(p, cred)
	write(p, "\n")
	t = @async close(p.in)
	# Process the response from the helper
	Base.read!(p, cred)
	wait(t)
	end
	open(pipeline(cmd; stderr), "r+") do p
	t = @async Base.read!(p, cred)
	write(p, cred)
	write(p, "\n")
	close(p.in)
	# Process the response from the helper
	fetch(t)
	end

It doesn't matter if the current task is used for read or for write. If the child dies, both the read and write pipes should close and cause both to end abruptly. I feel a slight preference towards having the wait for the writes to happen in the current task though and the wait for the read to happen in the other task. To be extra pedantic, it can also tie the lifetime of p to the lifetime of the read task too:

        t = @async try; Base.read!(p, cred); finally; close(p); end

We could swap the purpose of the tasks too, but I think this is possibly a bit more awkward:

    open(pipeline(cmd; stderr), "r+") do p
        t = @async try
            write(p, cred)
            write(p, "\n")
        finally
             close(p.in)
        end
        r = Base.read!(p, cred)
        # Process the response from the helper
        wait(t)
        r
    end

Alternatively, the writing could have been delegated to an IOBuffer:

    open(pipeline(`$(helper.cmd) $operation`; stdin=IOBuffer(cred * "\n"), stderr), "r") do p
        read!(p, cred)
    end

[jonathan-conder-sm]

I agree although don't really have a preference between these alternatives, they're all better than the code before & after my change though