Re: [PATCH] git-remote-testgit: avoid process substitution
On Thu, Apr 25, 2013 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 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).
Actually, I think the code already checks for that.
> 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?
Indeed, it's the same code for both.
--
Felipe Contreras
Felipe Contreras <felipe.contreras@gmail.com>