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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html