Re: [PATCH] git_connect: clarify conn->use_shell flag
- From
Jeff King <peff@peff.net>
- Date
- Sep 8, 2015, 21:40 UTC
- Message-ID
- <20150908214019.GA24159@sigill.intra.peff.net>
- In-Reply-To
- <xmqqwpw096dq.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 08, 2015 at 10:18:41AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > 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.
Show 8 quoted lines
> > 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.
Show 5 quoted lines
> 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