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

Felipe Contreras [off-list ref] writes:

On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano [off-list ref] wrote:

quoted

Felipe Contreras [off-list ref] writes:

quoted

quoted

quoted

...
+             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?

On Thu, Apr 25, 2013 at 3:06 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano [off-list ref] wrote:

quoted

Felipe Contreras [off-list ref] writes:

quoted

quoted

quoted

...
+             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?

Will do.

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