# git rebase -i exec merger broke t3404-rebase-interactive.sh 7 messages from 2010-08-13 to 2010-08-22. Participants: Ævar Arnfjörð Bjarmason, Brian Gernhardt, Brandon Casey, Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/24731 ## Ævar Arnfjörð Bjarmason, 2010-08-13 16:37 Subject: git rebase -i exec merger broke t3404-rebase-interactive.sh Message-ID: URL: https://gitlist.dev/e/AANLkTinzBuR%2B9%3D%2BPwejJVwSkUiGODaP-RC7%3DagyLOgMt%40mail.gmail.com ``` 39e388728 (merging git rebase -i exec support) broke the funny names test in t3404-rebase-interactive.sh: expecting success: git rev-list A..funny >expect && test_tick && FAKE_LINES="1 2 3 4" git rebase -i A && git rev-list A.. >actual && test_cmp expect actual rebase -i script before editing: pick f6fec15 end with slash\ pick 4f2ade4 something (\000) that looks like octal pick f95dfbf something (\n) that looks like a newline pick 0e82af6 another commit rebase -i script after editing: pick f6fec15 end with slash\ pick 4f2ade4 something (\000) that looks like octal pick f95dfbf something (\n) that looks like a newline pick 0e82af6 another commit Unknown command: ) that looks like a newline Please fix this in the file /home/avar/g/git/t/trash directory.t3404-rebase-interactive/.git/rebase-merge/git-rebase-todo. not ok - 50 rebase-i history with funny messages # # git rev-list A..funny >expect && # test_tick && # FAKE_LINES="1 2 3 4" git rebase -i A && # git rev-list A.. >actual && # test_cmp expect actual # This one breaks under bash too, does it work for you Matthieu? If so what sort of environment are you executing it in? ``` ## Brian Gernhardt, 2010-08-13 17:20 Subject: Re: git rebase -i exec merger broke t3404-rebase-interactive.sh Message-ID: URL: https://gitlist.dev/e/D1A252AE-5D4C-4E51-9359-F4A443BB8A2E%40silverinsanity.com In-Reply-To: ``` On Aug 13, 2010, at 12:37 PM, Ævar Arnfjörð Bjarmason wrote: > 39e388728 (merging git rebase -i exec support) broke the funny names > test in t3404-rebase-interactive.sh: Just noticed this myself when your e-mail hit the list. > This one breaks under bash too, does it work for you Matthieu? If so > what sort of environment are you executing it in? Also broken here: OS X 10.6.4 using bash 3.2.48 and dash 0.5.6-20-ga92255d ~~ Brian ``` ## Brandon Casey, 2010-08-13 20:47 Subject: [PATCH 1/2] git-rebase--interactive.sh: rework skip_unnecessary_picks Message-ID: URL: https://gitlist.dev/e/vFgTzGXLhalxcMpLoOFhqltkPrQzeonuuQVYAuW79a2bfz1SRlvXGh7w8kLO_mBu9DM-e8Omq9k%40cipher.nrlssc.navy.mil In-Reply-To: ``` From: Brandon Casey Commit cd035b1c introduced the exec command to interactive rebase. In doing so, it modified the way that skip_unnecessary_picks iterates through the list of rebase commands so that it avoided collapsing multiple spaces into a single space. This is necessary for example if the argument to the exec command contains a path with multiple spaces in it. The way it did this was by reading each line of rebase commands into a single variable, and then breaking the individual components out using echo, sed, and cut. It used the individual broken-out components for decision making, and was still able to write the original line to the output file from the variable it had saved it in. But, since we only really need to look at anything other than the first element of the line when a 'pick' command is encountered, and even that is only necessary when we are still searching for "unnecessary" picks, and since newer rebase commands like 'exec' may not even require a sha1 field, let's make our read statement parse its input into a "command" variable, and a "rest" variable, and then only break out the sha1 from $rest, and call git-rev-parse, when absolutely necessary. I think this future proofs this subroutine, avoids calling git-rev-parse unnecessarily, and possibly with bogus arguments, and still accomplishes the goal of not mangling the $rest of the rebase command. Signed-off-by: Brandon Casey --- Brian Gernhardt wrote: > On Aug 13, 2010, at 12:37 PM, Ævar Arnfjörð Bjarmason wrote: > >> 39e388728 (merging git rebase -i exec support) broke the funny names >> test in t3404-rebase-interactive.sh: > > Just noticed this myself when your e-mail hit the list. > >> This one breaks under bash too, does it work for you Matthieu? If so >> what sort of environment are you executing it in? > > Also broken here: OS X 10.6.4 using bash 3.2.48 and dash 0.5.6-20-ga92255d The conversion to use printf instead of echo, introduced by 938791cd, was lost in the merge. This first patch is just a cleanup that I think simplifies and improves the code. The second patch should fix the breakage. -Brandon git-rebase--interactive.sh | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index bf49b5b..2e5bed0 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -619,25 +619,30 @@ do_rest () { # skip picking commits whose parents are unchanged skip_unnecessary_picks () { fd=3 - while read -r line + while read -r command rest do - command=$(echo "$line" | sed 's/ */ /' | cut -d ' ' -f 1) - sha1=$(echo "$line" | sed 's/ */ /' | cut -d ' ' -f 2) - rest=$(echo "$line" | sed 's/ */ /' | cut -d ' ' -f 3-) # fd=3 means we skip the command - case "$fd,$command,$(git rev-parse --verify --quiet "$sha1"^)" in - 3,pick,"$ONTO"*|3,p,"$ONTO"*) + case "$fd,$command" in + 3,pick|3,p) # pick a commit whose parent is current $ONTO -> skip - ONTO=$sha1 + sha1=$(echo "$rest" | cut -d ' ' -f 1) + case "$(git rev-parse --verify --quiet "$sha1"^)" in + "$ONTO"*) + ONTO=$sha1 + ;; + *) + fd=1 + ;; + esac ;; - 3,#*|3,,*) + 3,#*|3,) # copy comments ;; *) fd=1 ;; esac - echo "$line" >&$fd + echo "$command${rest:+ }$rest" >&$fd done <"$TODO" >"$TODO.new" 3>>"$DONE" && mv -f "$TODO".new "$TODO" && case "$(peek_next_command)" in -- 1.7.2.1 ``` ## Brandon Casey, 2010-08-13 20:47 Subject: [PATCH 2/2] git-rebase--interactive.sh: use printf instead of echo to print commit message Message-ID: URL: https://gitlist.dev/e/vFgTzGXLhalxcMpLoOFhqi1W6sU5I3lJ9CWjrrJjoRmkMjHSswmpLXU2vVL8PS5JJNEO727l9q8%40cipher.nrlssc.navy.mil In-Reply-To: ``` From: Brandon Casey Replace the echo statements that operate on $rest with printf's to restore what was lost from 938791cd. This avoids any mangling that XSI-conformant echo's may introduce. Signed-off-by: Brandon Casey --- Junio, Feel free to squash this into 1/2 if desired. -Brandon git-rebase--interactive.sh | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 2e5bed0..3419247 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -625,7 +625,7 @@ skip_unnecessary_picks () { case "$fd,$command" in 3,pick|3,p) # pick a commit whose parent is current $ONTO -> skip - sha1=$(echo "$rest" | cut -d ' ' -f 1) + sha1=$(printf '%s' "$rest" | cut -d ' ' -f 1) case "$(git rev-parse --verify --quiet "$sha1"^)" in "$ONTO"*) ONTO=$sha1 @@ -642,7 +642,7 @@ skip_unnecessary_picks () { fd=1 ;; esac - echo "$command${rest:+ }$rest" >&$fd + printf '%s\n' "$command${rest:+ }$rest" >&$fd done <"$TODO" >"$TODO.new" 3>>"$DONE" && mv -f "$TODO".new "$TODO" && case "$(peek_next_command)" in -- 1.7.2.1 ``` ## Brian Gernhardt, 2010-08-13 21:34 Subject: Re: [PATCH 2/2] git-rebase--interactive.sh: use printf instead of echo to print commit message Message-ID: <4BE6DADC-6846-4942-B361-639DCD308F09@silverinsanity.com> URL: https://gitlist.dev/e/4BE6DADC-6846-4942-B361-639DCD308F09%40silverinsanity.com In-Reply-To: ``` On Aug 13, 2010, at 4:47 PM, Brandon Casey wrote: > Replace the echo statements that operate on $rest with printf's to restore > what was lost from 938791cd. This avoids any mangling that XSI-conformant > echo's may introduce. ACK, this was exactly the problem. Thanks for the quick response! ~~ Brian ``` ## Matthieu Moy, 2010-08-15 08:56 Subject: Re: [PATCH 2/2] git-rebase--interactive.sh: use printf instead of echo to print commit message Message-ID: URL: https://gitlist.dev/e/vpqmxso45l6.fsf%40bauges.imag.fr In-Reply-To: <4BE6DADC-6846-4942-B361-639DCD308F09@silverinsanity.com> ``` Brian Gernhardt writes: > On Aug 13, 2010, at 4:47 PM, Brandon Casey wrote: > >> Replace the echo statements that operate on $rest with printf's to restore >> what was lost from 938791cd. This avoids any mangling that XSI-conformant >> echo's may introduce. > > ACK, this was exactly the problem. Thanks for the quick response! Both patches sound good, yes. Junio: I think you can either apply them on top of mine, or squash them all together. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Junio C Hamano, 2010-08-22 07:22 Subject: Re: [PATCH 2/2] git-rebase--interactive.sh: use printf instead of echo to print commit message Message-ID: <7vd3tb6rhp.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd3tb6rhp.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Both makes sense; thanks. ```