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
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
