Re: [PATCH v2] rebase -i: respect core.commentchar
On Mon, Feb 11, 2013 at 04:13:31PM -0800, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: @@ -179,7 +182,9 @@ die_abort () { } has_action () { - sane_grep '^[^#]' $1 /dev/null + echo space stripped actions: 2 + git stripspace --strip-comments $1 2 + test -n $(git stripspace --strip-comments $1) } I'll remove the debugging remnants while queuing. Thanks. I don't think I was fully awake when I finished this last night - the following fixup is also needed to avoid relying on the shell emitting a literal backslash when a backslash isn't followed by a known escape character. -- 8 -- diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index cbe36bf..84bd525 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -947,7 +947,7 @@ test_expect_success 'rebase -i respects core.commentchar' ' test_when_finished git config --unset core.commentchar cat comment-lines.sh EOF #!$SHELL_PATH -sed -e 2,\$ s/^/\\/ \$1 \$1.tmp +sed -e 2,\$ s/^// \$1 \$1.tmp mv \$1.tmp \$1 EOF chmod a+x comment-lines.sh -- 8 -- @@ -942,20 +948,18 @@ test -s $todo || echo noop $todo test -n $autosquash rearrange_squash $todo test -n $cmd add_exec_commands $todo -cat $todo EOF - -# Rebase $shortrevisions onto $shortonto -EOF +echo $todo +printf '%s\n' $comment_char Rebase $shortrevisions onto $shortonto $todo I think you can still do cat $todo EOF $comment_char Rebase $shortrevisions onto... EOF here with any funny comment character. Doing this with two separate I/O does not hurt very much, but the resulting code may be easier to scan if left as here-text with a single cat. Please eyeball what is in 'pu' (I have a separate squashable fixup on top of your patch) and let me know if I made mistakes. The fixup commit looks good to me. John -- 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 v2] rebase -i: respect core.commentchar
John Keeping j...@keeping.me.uk writes: ... the following fixup is also needed to avoid relying on the shell emitting a literal backslash when a backslash isn't followed by a known escape character. -- 8 -- diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index cbe36bf..84bd525 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -947,7 +947,7 @@ test_expect_success 'rebase -i respects core.commentchar' ' test_when_finished git config --unset core.commentchar cat comment-lines.sh EOF #!$SHELL_PATH -sed -e 2,\$ s/^/\\/ \$1 \$1.tmp +sed -e 2,\$ s/^// \$1 \$1.tmp mv \$1.tmp \$1 EOF chmod a+x comment-lines.sh Yeek. If you used write_script with here-text that does not interpolate, write_script remove-all-but-the-first.sh \EOF sed -e '2,$s/^/\\/' $1 $1.tmp mv $1.tmp $1 EOF the above would be much more readable. I am not sure if I understand what you meant by literal backslash blah blah, though. -- 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 v2] rebase -i: respect core.commentchar
On Tue, Feb 12, 2013 at 09:29:26AM -0800, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: ... the following fixup is also needed to avoid relying on the shell emitting a literal backslash when a backslash isn't followed by a known escape character. -- 8 -- diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index cbe36bf..84bd525 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -947,7 +947,7 @@ test_expect_success 'rebase -i respects core.commentchar' ' test_when_finished git config --unset core.commentchar cat comment-lines.sh EOF #!$SHELL_PATH -sed -e 2,\$ s/^/\\/ \$1 \$1.tmp +sed -e 2,\$ s/^// \$1 \$1.tmp mv \$1.tmp \$1 EOF chmod a+x comment-lines.sh Yeek. If you used write_script with here-text that does not interpolate, write_script remove-all-but-the-first.sh \EOF sed -e '2,$s/^/\\/' $1 $1.tmp mv $1.tmp $1 EOF the above would be much more readable. Yet another thing for me to learn about ;-) Do you mean to use that outside the test case, so that the single quotes work? Or do I still need some level of escaping? I am not sure if I understand what you meant by literal backslash blah blah, though. It turns out that having this in the script works (in bash and dash although I haven't checked what Posix has to say about it): sed -e 2,$ s/^/\\\/ and is equivalent to: sed -e '2,$ s/^/\\/' because backslashes that aren't recognised as part of an escape sequence are not treated specially. John -- 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 v2] rebase -i: respect core.commentchar
Junio C Hamano gits...@pobox.com writes: cat comment-lines.sh EOF #!$SHELL_PATH -sed -e 2,\$ s/^/\\/ \$1 \$1.tmp +sed -e 2,\$ s/^// \$1 \$1.tmp mv \$1.tmp \$1 EOF chmod a+x comment-lines.sh Yeek. If you used write_script with here-text that does not interpolate, write_script remove-all-but-the-first.sh \EOF sed -e '2,$s/^/\\/' $1 $1.tmp mv $1.tmp $1 EOF the above would be much more readable. As this is already inside a pair of '', we cannot use single quote around sed expression without doing the ugly '\''. So it needs to be more like this, and I think it still is more readable. diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index cbe36bf..8b3e2cd 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -945,13 +945,11 @@ test_expect_success 'rebase -i respects core.commentchar' ' git checkout E^0 git config core.commentchar \\ test_when_finished git config --unset core.commentchar - cat comment-lines.sh EOF -#!$SHELL_PATH -sed -e 2,\$ s/^/\\/ \$1 \$1.tmp -mv \$1.tmp \$1 -EOF - chmod a+x comment-lines.sh - test_set_editor $(pwd)/comment-lines.sh + write_script remove-all-but-first.sh -\EOF + sed -e 2,\$s/^// $1 $1.tmp + mv $1.tmp $1 + EOF + test_set_editor $(pwd)/remove-all-but-first.sh git rebase -i B test B = $(git cat-file commit HEAD^ | sed -ne \$p) ' -- 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 v2] rebase -i: respect core.commentchar
On Tue, Feb 12, 2013 at 10:00:26AM -0800, Junio C Hamano wrote: So it needs to be more like this, and I think it still is more readable. Agreed. Will you squash this in or do you want a re-roll? diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index cbe36bf..8b3e2cd 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -945,13 +945,11 @@ test_expect_success 'rebase -i respects core.commentchar' ' git checkout E^0 git config core.commentchar \\ test_when_finished git config --unset core.commentchar - cat comment-lines.sh EOF -#!$SHELL_PATH -sed -e 2,\$ s/^/\\/ \$1 \$1.tmp -mv \$1.tmp \$1 -EOF - chmod a+x comment-lines.sh - test_set_editor $(pwd)/comment-lines.sh + write_script remove-all-but-first.sh -\EOF + sed -e 2,\$s/^// $1 $1.tmp + mv $1.tmp $1 + EOF + test_set_editor $(pwd)/remove-all-but-first.sh git rebase -i B test B = $(git cat-file commit HEAD^ | sed -ne \$p) ' -- 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 v2] rebase -i: respect core.commentchar
John Keeping j...@keeping.me.uk writes: I am not sure if I understand what you meant by literal backslash blah blah, though. It turns out that having this in the script works (in bash and dash although I haven't checked what Posix has to say about it): sed -e 2,$ s/^/\\\/ and is equivalent to: sed -e '2,$ s/^/\\/' because backslashes that aren't recognised as part of an escape sequence are not treated specially. That's POSIX. Inside a dq pair: \ The backslash shall retain its special meaning as an escape character (see Escape Character (Backslash)) only when followed by one of the following characters when considered special: $ ` \ newline So in your example \\\/, the first backslash escapes the second backslash and together they produce a single backslash, the third backslash is followed by a slash that is not special at all, so it produces a second backslash, and the slash stands for itself, resulting in \\/. -- 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 v2] rebase -i: respect core.commentchar
John Keeping j...@keeping.me.uk writes: On Tue, Feb 12, 2013 at 10:00:26AM -0800, Junio C Hamano wrote: So it needs to be more like this, and I think it still is more readable. Agreed. Will you squash this in or do you want a re-roll? I can squash this and the previous one into your original to a single commit. Thanks. diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index cbe36bf..8b3e2cd 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -945,13 +945,11 @@ test_expect_success 'rebase -i respects core.commentchar' ' git checkout E^0 git config core.commentchar \\ test_when_finished git config --unset core.commentchar -cat comment-lines.sh EOF -#!$SHELL_PATH -sed -e 2,\$ s/^/\\/ \$1 \$1.tmp -mv \$1.tmp \$1 -EOF -chmod a+x comment-lines.sh -test_set_editor $(pwd)/comment-lines.sh +write_script remove-all-but-first.sh -\EOF +sed -e 2,\$s/^// $1 $1.tmp +mv $1.tmp $1 +EOF +test_set_editor $(pwd)/remove-all-but-first.sh git rebase -i B test B = $(git cat-file commit HEAD^ | sed -ne \$p) ' -- 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
[PATCH v2] rebase -i: respect core.commentchar
Commit eff80a9 (Allow custom comment char) introduced a custom comment character for commit messages but did not teach git-rebase--interactive to use it. Change git-rebase--interactive to read core.commentchar and use its value when generating commit messages and for the command list. Signed-off-by: John Keeping j...@keeping.me.uk --- Changes since v1: - use '\' as the comment character in tests - use git stripspace --strip-comments where appropriate - use git stripspace --comment-lines where appropriate - use printf instead of echo when the string contains $comment_char - quote $comment_char in case label On Mon, Feb 11, 2013 at 01:49:27PM -0800, Junio C Hamano wrote: ... Perhaps git stripspace --comment-lines EOF ... EOF is a better option that that loop. Yes. I was confusing myself with out-of-date documentation. --- git-rebase--interactive.sh| 88 ++- t/t3404-rebase-interactive.sh | 16 2 files changed, 62 insertions(+), 42 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 8ed7fcc..b5ce3f2 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -80,6 +80,9 @@ rewritten_pending=$state_dir/rewritten-pending GIT_CHERRY_PICK_HELP=$resolvemsg export GIT_CHERRY_PICK_HELP +comment_char=$(git config --get core.commentchar 2/dev/null | cut -c1) +: ${comment_char:=#} + warn () { printf '%s\n' $* 2 } @@ -105,8 +108,8 @@ mark_action_done () { sed -e 1q $todo $done sed -e 1d $todo $todo.new mv -f $todo.new $todo - new_count=$(sane_grep -c '^[^#]' $done) - total=$(($new_count+$(sane_grep -c '^[^#]' $todo))) + new_count=$(git stripspace --strip-comments $done | wc -l) + total=$(($new_count+$(git stripspace --strip-comments $todo | wc -l))) if test $last_count != $new_count then last_count=$new_count @@ -116,19 +119,19 @@ mark_action_done () { } append_todo_help () { - cat $todo EOF -# -# Commands: -# p, pick = use commit -# r, reword = use commit, but edit the commit message -# e, edit = use commit, but stop for amending -# s, squash = use commit, but meld into previous commit -# f, fixup = like squash, but discard this commit's log message -# x, exec = run command (the rest of the line) using shell -# -# These lines can be re-ordered; they are executed from top to bottom. -# -# If you remove a line here THAT COMMIT WILL BE LOST. + git stripspace --comment-lines $todo EOF + +Commands: + p, pick = use commit + r, reword = use commit, but edit the commit message + e, edit = use commit, but stop for amending + s, squash = use commit, but meld into previous commit + f, fixup = like squash, but discard this commit's log message + x, exec = run command (the rest of the line) using shell + +These lines can be re-ordered; they are executed from top to bottom. + +If you remove a line here THAT COMMIT WILL BE LOST. EOF } @@ -179,7 +182,9 @@ die_abort () { } has_action () { - sane_grep '^[^#]' $1 /dev/null + echo space stripped actions: 2 + git stripspace --strip-comments $1 2 + test -n $(git stripspace --strip-comments $1) } is_empty_commit() { @@ -363,10 +368,10 @@ update_squash_messages () { if test -f $squash_msg; then mv $squash_msg $squash_msg.bak || exit count=$(($(sed -n \ - -e 1s/^# This is a combination of \(.*\) commits\./\1/p \ + -e 1s/^. This is a combination of \(.*\) commits\./\1/p \ -e q $squash_msg.bak)+1)) { - echo # This is a combination of $count commits. + printf '%s\n' $comment_char This is a combination of $count commits. sed -e 1d -e '2,/^./{ /^$/d }' $squash_msg.bak @@ -375,8 +380,8 @@ update_squash_messages () { commit_message HEAD $fixup_msg || die Cannot write $fixup_msg count=2 { - echo # This is a combination of 2 commits. - echo # The first commit's message is: + printf '%s\n' $comment_char This is a combination of 2 commits. + printf '%s\n' $comment_char The first commit's message is: echo cat $fixup_msg } $squash_msg @@ -385,21 +390,22 @@ update_squash_messages () { squash) rm -f $fixup_msg echo - echo # This is the $(nth_string $count) commit message: + printf '%s\n' $comment_char This is the $(nth_string $count) commit message: echo commit_message $2 ;; fixup) echo - echo # The
Re: [PATCH v2] rebase -i: respect core.commentchar
John Keeping j...@keeping.me.uk writes: @@ -179,7 +182,9 @@ die_abort () { } has_action () { - sane_grep '^[^#]' $1 /dev/null + echo space stripped actions: 2 + git stripspace --strip-comments $1 2 + test -n $(git stripspace --strip-comments $1) } I'll remove the debugging remnants while queuing. fixup) echo - echo # The $(nth_string $count) commit message will be skipped: + printf '%s\n' $comment_char The $(nth_string $count) commit message will be skipped: echo - commit_message $2 | sed -e 's/^/# /' + # Change the space after the comment character to TAB: + commit_message $2 | git stripspace --comment-lines | sed -e 's/ / /' I think this changes the behaviour but in a good way. It used to show an empty line in the incoming commit message to a hash followed by a trailing HT before the end of line, but now it only emits the comment char immediately followed by the end of line. @@ -942,20 +948,18 @@ test -s $todo || echo noop $todo test -n $autosquash rearrange_squash $todo test -n $cmd add_exec_commands $todo -cat $todo EOF - -# Rebase $shortrevisions onto $shortonto -EOF +echo $todo +printf '%s\n' $comment_char Rebase $shortrevisions onto $shortonto $todo I think you can still do cat $todo EOF $comment_char Rebase $shortrevisions onto... EOF here with any funny comment character. Doing this with two separate I/O does not hurt very much, but the resulting code may be easier to scan if left as here-text with a single cat. Please eyeball what is in 'pu' (I have a separate squashable fixup on top of your patch) and let me know if I made mistakes. Thanks for working on this. -- 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