Re: [PATCH] git_connect: clarify conn->use_shell flag

[Jeff King <peff@peff.net>]

On Tue, Sep 08, 2015 at 10:18:41AM -0700, Junio C Hamano wrote:

> > To make the flow easier to follow, let's just set
> > conn->use_shell when we're setting up the "conn" struct, and
> > unset it (with a comment) in the historical GIT_SSH case.
> 
> Makes perfect sense.  I think another thing that falls into the same
> class wrt readability is 'putty'; if the code does putty=0 at the
> beginning of "various flavours of SSH", and sets it only when it
> checks with "various flavours of plink" when GIT_SSH_COMMAND is not
> set, the logic would be even clearer, I suspect.

Yeah, I think so.

> > Note that for clarity we leave "use_shell" on in the case
> > that we fall back to our default "ssh" This looks like a
> > behavior change, but in practice run-command.c optimizes
> > shell invocations without metacharacters into a straight
> > execve anyway.
> 
> Hmm, interesting.  I am not sure if that has to be the way, though.
> Wouldn't the resulting code become simpler if you do not do that?

Heh, I originally wrote it that way, and waffled between sending one or
the other.

> That is, is is what I have in mind on top of your patch.  Did I
> break something?
> 
>  connect.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

I think both changes are correct, and the result looks nice to read.
Feel free to squash them in as you apply.

-Peff