Re: Re* [PATCH] git-remote-testgit: avoid process substitution
Johannes Sixt writes: > The patch below doesn't remove the bash dependency, yet, but it addresses > the problematic mismatch you noticed without the need for $LF. Can you > please queue it to move the topic forward? I'll send three-patch series I have (including the two discussed in the thread) on top of fc/transport-helper-error-reporting as a reply to this message. It seems to pass "make SHELL_PATH=/bin/dash test". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [PATCH] git-remote-testgit: avoid process substitution
Am 27.04.2013 01:26, schrieb Junio C Hamano:
> Felipe Contreras writes:
>
>> No, it wouldn't, but I don't think there's any way to do \<\> or \b in globs.
>
> This should do in the meantime, but it further needs:
>
> - J6t's sign off for the follow-up part to remove remaining
>bash-isms to complete this patch (the last part of the patch is
>from <5178c583.6000...@viscovery.net> and we can take half the
>log message from there);
The patch below doesn't remove the bash dependency, yet, but it addresses
the problematic mismatch you noticed without the need for $LF. Can you
please queue it to move the topic forward?
Removing the remaining bashisms and the following two can come later:
> - Rename it to git-remote-testgit.sh and tell Makefile to replace
>the shebang line with SHELL_PATH like other scripts;
>
> - Remove the "we need to have bash because we will run remote-testgit"
>logic from t5801
Here's my
Signed-off-by: Johannes Sixt
for this part in case someone wants to pick it up:
> diff --git a/git-remote-testgit b/git-remote-testgit
> index b395c8d..ffac950 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/bin/sh
> # Copyright (c) 2012 Felipe Contreras
>
> alias=$1
> @@ -23,7 +23,6 @@ then
> testgitmarks="$dir/testgit.marks"
> test -e "$gitmarks" || >"$gitmarks"
> test -e "$testgitmarks" || >"$testgitmarks"
> - testgitmarks_args=( "--"{import,export}"-marks=$testgitmarks" )
> fi
>
> while read line
> @@ -62,22 +61,31 @@ do
> echo "feature export-marks=$gitmarks"
> fi
> echo "feature done"
> - git fast-export "${testgitmarks_args[@]}" $refs |
> + git fast-export \
> + ${testgitmarks:+"--import-marks=$testgitmarks"} \
> + ${testgitmarks:+"--export-marks=$testgitmarks"} \
> + $refs |
> sed -e "s#refs/heads/#${prefix}/heads/#g"
> echo "done"
> ;;
> export)
> before=$(git for-each-ref --format='%(refname) %(objectname)')
>
> - git fast-import "${testgitmarks_args[@]}" --quiet
> + git fast-import \
> + ${testgitmarks:+"--import-marks=$testgitmarks"} \
> + ${testgitmarks:+"--export-marks=$testgitmarks"} \
> + --quiet
>
--- 8< ---
Subject: [PATCH] git-remote-testgit: avoid process substitution
The implementation of bash on Windows does not offer process substitution.
Signed-off-by: Johannes Sixt
---
git-remote-testgit | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/git-remote-testgit b/git-remote-testgit
index 23c9d40..979b13e 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -87,17 +87,18 @@ do
exit 1
fi
- before=$(git for-each-ref --format='%(refname) %(objectname)')
+ before=$(git for-each-ref --format=' %(refname) %(objectname) ')
git fast-import "${testgitmarks_args[@]}" --quiet
- after=$(git for-each-ref --format='%(refname) %(objectname)')
-
# figure out which refs were updated
- join -e 0 -o '0 1.2 2.2' -a 2 <(echo "$before") <(echo
"$after") |
- while read ref a b
+ git for-each-ref --format='%(refname) %(objectname)' |
+ while read ref a
do
- test $a == $b && continue
+ case "$before" in
+ *" $ref $a "*)
+ continue ;; # unchanged
+ esac
echo "ok $ref"
done
--
1.8.2.388.g36592d7
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re* [PATCH] git-remote-testgit: avoid process substitution
Felipe Contreras writes:
> No, it wouldn't, but I don't think there's any way to do \<\> or \b in globs.
This should do in the meantime, but it further needs:
- J6t's sign off for the follow-up part to remove remaining
bash-isms to complete this patch (the last part of the patch is
from <5178c583.6000...@viscovery.net> and we can take half the
log message from there);
- Rename it to git-remote-testgit.sh and tell Makefile to replace
the shebang line with SHELL_PATH like other scripts;
- Remove the "we need to have bash because we will run remote-testgit"
logic from t5801
diff --git a/git-remote-testgit b/git-remote-testgit
index b395c8d..ffac950 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh
# Copyright (c) 2012 Felipe Contreras
alias=$1
@@ -23,7 +23,6 @@ then
testgitmarks="$dir/testgit.marks"
test -e "$gitmarks" || >"$gitmarks"
test -e "$testgitmarks" || >"$testgitmarks"
- testgitmarks_args=( "--"{import,export}"-marks=$testgitmarks" )
fi
while read line
@@ -62,22 +61,31 @@ do
echo "feature export-marks=$gitmarks"
fi
echo "feature done"
- git fast-export "${testgitmarks_args[@]}" $refs |
+ git fast-export \
+ ${testgitmarks:+"--import-marks=$testgitmarks"} \
+ ${testgitmarks:+"--export-marks=$testgitmarks"} \
+ $refs |
sed -e "s#refs/heads/#${prefix}/heads/#g"
echo "done"
;;
export)
before=$(git for-each-ref --format='%(refname) %(objectname)')
- git fast-import "${testgitmarks_args[@]}" --quiet
-
- after=$(git for-each-ref --format='%(refname) %(objectname)')
+ git fast-export \
+ ${testgitmarks:+"--import-marks=$testgitmarks"} \
+ ${testgitmarks:+"--export-marks=$testgitmarks"} \
+ --quiet
# figure out which refs were updated
- join -e 0 -o '0 1.2 2.2' -a 2 <(echo "$before") <(echo
"$after") |
- while read ref a b
+ LF=$'\n'
+ git for-each-ref --format='%(refname) %(objectname)' |
+ while read ref a
do
- test $a == $b && continue
+ case "$LF$before$LF" in
+ *"$LF$ref $a$LF"*)
+ continue
+ ;;
+ esac
echo "ok $ref"
done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
On Fri, Apr 26, 2013 at 5:25 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> On Thu, Apr 25, 2013 at 12:56 AM, Johannes Sixt wrote: >>> From: Johannes Sixt >>> >>> Bash on Windows does not implement process substitution. >> >> Nevermind, reporting all the refs creates a lot of irrelevant output, >> this version is fine. > > When $before has this in it: > > refs/heads/refs/heads/master 664059...126eaa7 > > and your "read ref a" got this in the input: > > refs/heads/master 664059...126eaa7 > > would the pattern matching by case work corretly? No, it wouldn't, but I don't think there's any way to do \<\> or \b in globs. Alternatively, we could use the same $before and $after, and simply store the output somewhere; $GIT_DIR/testgit-before, for example. This should decrease the amount of changes needed, but I don't know if anybody would have problems with the use of 'join'. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
Felipe Contreras writes: > On Thu, Apr 25, 2013 at 12:56 AM, Johannes Sixt wrote: >> From: Johannes Sixt >> >> Bash on Windows does not implement process substitution. > > Nevermind, reporting all the refs creates a lot of irrelevant output, > this version is fine. When $before has this in it: refs/heads/refs/heads/master 664059...126eaa7 and your "read ref a" got this in the input: refs/heads/master 664059...126eaa7 would the pattern matching by case work corretly? > > Acknowledged-by: Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
On Thu, Apr 25, 2013 at 12:56 AM, Johannes Sixt wrote: > From: Johannes Sixt > > Bash on Windows does not implement process substitution. Nevermind, reporting all the refs creates a lot of irrelevant output, this version is fine. Acknowledged-by: Felipe Contreras -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
On Thu, Apr 25, 2013 at 3:06 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano wrote: >>> Felipe Contreras 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? 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
Felipe Contreras writes: > On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano wrote: >> Felipe Contreras 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano wrote: > Felipe Contreras 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. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
Felipe Contreras 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
On Thu, Apr 25, 2013 at 9:57 AM, Junio C Hamano wrote:
> Johannes Sixt writes:
>
>> From: Johannes Sixt
>>
>> Bash on Windows does not implement process substitution.
>>
>> Signed-off-by: Johannes Sixt
>> ---
>> ...
>> Here is a fix. It assumes that the list of refs after the import is
>> a superset of the refs before the import. (Can refs be deleted
>> via fast-import?)
>>
>> git-remote-testgit | 12 +++-
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/git-remote-testgit b/git-remote-testgit
>> index 23c9d40..e99d5fa 100755
>> --- a/git-remote-testgit
>> +++ b/git-remote-testgit
>> @@ -91,13 +91,15 @@ do
>>
>> git fast-import "${testgitmarks_args[@]}" --quiet
>>
>> - after=$(git for-each-ref --format='%(refname) %(objectname)')
>> -
>> # figure out which refs were updated
>> - join -e 0 -o '0 1.2 2.2' -a 2 <(echo "$before") <(echo
>> "$after") |
>> - while read ref a b
>> + git for-each-ref --format='%(refname) %(objectname)' |
>> + while read ref a
>> do
>> - test $a == $b && continue
>> + case "$before" in
>> + *"$ref $a"*)
>> + continue
>
> This just like the original 'join' depends on the two output from
> for-each-ref to be sorted the same way, which is true and fine. But
> I wonder one thing. When $before has this in it:
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).
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-remote-testgit: avoid process substitution
Johannes Sixt writes:
> From: Johannes Sixt
>
> Bash on Windows does not implement process substitution.
>
> Signed-off-by: Johannes Sixt
> ---
> ...
> Here is a fix. It assumes that the list of refs after the import is
> a superset of the refs before the import. (Can refs be deleted
> via fast-import?)
>
> git-remote-testgit | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> index 23c9d40..e99d5fa 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -91,13 +91,15 @@ do
>
> git fast-import "${testgitmarks_args[@]}" --quiet
>
> - after=$(git for-each-ref --format='%(refname) %(objectname)')
> -
> # figure out which refs were updated
> - join -e 0 -o '0 1.2 2.2' -a 2 <(echo "$before") <(echo
> "$after") |
> - while read ref a b
> + git for-each-ref --format='%(refname) %(objectname)' |
> + while read ref a
> do
> - test $a == $b && continue
> + case "$before" in
> + *"$ref $a"*)
> + continue
This just like the original 'join' depends on the two output from
for-each-ref to be sorted the same way, which is true and fine. But
I wonder one thing. When $before has this in it:
refs/heads/refs/heads/master 664059...126eaa7
and your "read ref a" got this in the input:
refs/heads/master 664059...126eaa7
would the pattern matching by case work corretly?
Doing something like this might be needed.
case "$LF$before$LF" in
*"$LF$ref $a$LF"*)
continue ;; # matches
esac
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
