Re: [PATCH] git-remote-testgit: avoid process substitution

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2013, 20:06 UTC
Message-ID: <7vsj2e2x5d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMP44s3LhYt+eNaKWyoDWAPtepaOKXLhYsPXg5dPjYN8MoGA-w@mail.gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 26 quoted lines

> On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>>>> ...
>>>>> +             git for-each-ref --format='%(refname) %(objectname)' |
>>>>> +             while read ref a
>>>>>               do
>>>>> -                     test $a == $b && continue
>>>>> +                     case "$before" in
>>>>> +                     *"$ref $a"*)
>>>>> +                             continue
>>>>
>>> I wonder if we should bother with this at all. The purpose of the code
>>> was mainly to show to users that they should report the success only
>>> if the refs have been updated, but the code is becoming more
>>> obfuscated, a comment should do the trick. And then, we can just
>>> report success for all the refs (and explain in the comment why).
>>
>> Are you proposing to say "ok $ref" to everything we see in the
>> resulting repository, even the ones the caller of remote-testgit did
>> not ask us to do anything with?
>>
>> Wouldn't the caller be surprised if we did so?
>
> Why would it?  The only effective difference is what you'll see
> reported in the UI, but there's no user here.

You are correct that it affects only the UI, but isn't that because the current implementation of push_update_refs_status() blindly accepts whatever 'ok' response says without checking the ref mentioned against what it tried to push out? I was wondering if that codepath should stay that way forever, or if we may want add sanity checks later. If the latter, I suspect this would manifest as an ancient bug to say 'ok' for everything we have instead of what we actually pushed out (of course, the explanation in the comment would help).

So I am OK with that simpler approach. Care to roll the conclusion of your idea into a patch?

By the way, I noticed that Documentation/gitremote-helpers.txt does not even mention these 'ok' responses for 'export' command, but they should be the same as responses to 'push'. Perhaps we can share some text between the two?