[PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[Nazri Ramliy]

Signed-off-by: Nazri Ramliy <ayiehere@gmail.com>
---
 t/lib-rebase.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6aefe27..21d2fef 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -4,6 +4,8 @@
 #
 # - override the commit message with $FAKE_COMMIT_MESSAGE
 # - amend the commit message with $FAKE_COMMIT_AMEND
+# - check that the commit header(s) text shown in the editor matches that in the
+#   file $EXPECT_HEADER.
 # - check that non-commit messages have a certain line count with $EXPECT_COUNT
 # - check the commit count in the commit message header with $EXPECT_HEADER_COUNT
 # - rewrite a rebase -i script as directed by $FAKE_LINES.
@@ -34,6 +36,12 @@ case "$1" in
 	exit
 	;;
 esac
+test -z "$EXPECT_HEADER" ||
+	(
+		grep '^pick' < "$1" | cut -d' ' -f3- > commit_headers.$$ &&
+		diff "$EXPECT_HEADER" commit_headers.$$ > /dev/null
+	) ||
+	exit
 test -z "$EXPECT_COUNT" ||
 	test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
 	exit
-- 
1.7.2.rc2

[PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[Nazri Ramliy]

Signed-off-by: Nazri Ramliy <ayiehere@gmail.com>
---
 t/lib-rebase.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6aefe27..5804d23 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -4,6 +4,8 @@
 #
 # - override the commit message with $FAKE_COMMIT_MESSAGE
 # - amend the commit message with $FAKE_COMMIT_AMEND
+# - check that commit header(s) in the editor matches that in the
+#   file $EXPECT_HEADER.
 # - check that non-commit messages have a certain line count with $EXPECT_COUNT
 # - check the commit count in the commit message header with $EXPECT_HEADER_COUNT
 # - rewrite a rebase -i script as directed by $FAKE_LINES.
@@ -34,6 +36,12 @@ case "$1" in
 	exit
 	;;
 esac
+test -z "$EXPECT_HEADER" ||
+	(
+		grep '^pick' < "$1" | cut -d' ' -f3- > commit_headers.$$ &&
+		diff "$EXPECT_HEADER" commit_headers.$$ > /dev/null
+	) ||
+	exit
 test -z "$EXPECT_COUNT" ||
 	test "$EXPECT_COUNT" = $(sed -e '/^#/d' -e '/^$/d' < "$1" | wc -l) ||
 	exit
-- 
1.7.2.rc2

[PATCH 2/2] rebase -i: Preserve whitespace at beginning of commit header in $GIT_EDITOR

[Nazri Ramliy]

During "git rebase -i", when the commit headers are shown in the editor
for action (pick/squash/etc), the whitespace (if any) at the beginning
of commit headers are stripped out due to the use of "read shortsha1 rest"
for reading the output of "git rev-list".

The missing beginning whitespace do not pose any harm but this could be
annoying when you want to identify these commits to perform actions on,
for example, rewording them to remove the beginning whitespace.

Not having the whitespace makes them hard to spot, and you have to rely
on something like "git log --oneline" to identify them.

This patch:

	1. Ensures that the commit header shown in $GIT_EDITOR is
	   identical to the actual commit header
	2. Add a test for it

Signed-off-by: Nazri Ramliy <ayiehere@gmail.com>
---
This series is based on maint.

Running all test shows no breakage. 

 git-rebase--interactive.sh    |   17 ++++++++++-------
 t/t3404-rebase-interactive.sh |   12 ++++++++++++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6b86abc..80991a8 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -683,6 +683,10 @@ parse_onto () {
 	git rev-parse --verify "$1^0"
 }
 
+pick () {
+	git log -1 --abbrev-commit --abbrev=7 --format="pick %h %s" $sha1
+}
+
 while test $# != 0
 do
 	case "$1" in
@@ -887,16 +891,15 @@ first and then run 'git rebase --continue' again."
 			REVISIONS=$ONTO...$HEAD
 			SHORTREVISIONS=$SHORTHEAD
 		fi
-		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
-			--abbrev=7 --reverse --left-right --topo-order \
-			$REVISIONS | \
-			sed -n "s/^>//p" | while read shortsha1 rest
+
+		git rev-list $MERGES_OPTION --reverse --left-right \
+			--topo-order $REVISIONS | sed -n "s/^>//p" |
+		while read sha1
 		do
 			if test t != "$PRESERVE_MERGES"
 			then
-				echo "pick $shortsha1 $rest" >> "$TODO"
+				pick $sha1 >> "$TODO"
 			else
-				sha1=$(git rev-parse $shortsha1)
 				if test -z "$REBASE_ROOT"
 				then
 					preserve=t
@@ -913,7 +916,7 @@ first and then run 'git rebase --continue' again."
 				if test f = "$preserve"
 				then
 					touch "$REWRITTEN"/$sha1
-					echo "pick $shortsha1 $rest" >> "$TODO"
+					pick $sha1 >> "$TODO"
 				fi
 			fi
 		done
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ee9a1b2..e346174 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -630,4 +630,16 @@ test_expect_success 'always cherry-pick with --no-ff' '
 	test_cmp empty out
 '
 
+cat >expect_header <<EOF
+  Commit header with beginning whitespace
+EOF
+
+test_expect_success 'preserve whitespace in commit summary' '
+	git checkout -b preserve-whitespace master &&
+	echo a >whitespace_test &&
+	git add whitespace_test &&
+	git commit -m"  Commit header with beginning whitespace" &&
+	EXPECT_HEADER="expect_header" git rebase -i HEAD~1
+'
+
 test_done
-- 
1.7.2.rc2

Re: [PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[Junio C Hamano]

Nazri Ramliy <ayiehere@gmail.com> writes:

> +test -z "$EXPECT_HEADER" ||
> +	(
> +		grep '^pick' < "$1" | cut -d' ' -f3- > commit_headers.$$ &&

Sending output from grep to cut does not sound very cool; wouldn't a
single "sed" invocation more appropriate?

> +		diff "$EXPECT_HEADER" commit_headers.$$ > /dev/null

Is "test_cmp" inappropriate here for some reason?

> +	) ||

Do you need a subshell for this, or just a grouping {} sufficient?

> +	exit

Re: [PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[Nazri Ramliy]

On Mon, Jul 12, 2010 at 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nazri Ramliy <ayiehere@gmail.com> writes:
>
>> +test -z "$EXPECT_HEADER" ||
>> +     (
>> +             grep '^pick' < "$1" | cut -d' ' -f3- > commit_headers.$$ &&
>
> Sending output from grep to cut does not sound very cool; wouldn't a
> single "sed" invocation more appropriate?

If I were to use a single "sed" invocation here's how it's going to look like:

	sed -e "s/^pick [0-9a-f]\+//" < "$1" > commit_headers.$$

But doing so reminds me of this:

On Sat, Jul 10, 2010 at 6:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nazri Ramliy <ayiehere@gmail.com> writes:
>
>> +                     sed -i "s/^\([a-z]\+\) [0-9a-f]\+ /\1 $REPLACE_COMMIT_ID /" \
>
> This is not portable. Escaping an ERE element with a backslash does not
> make it suitable for use in BRE that sed uses.

Aren't we back to square one? Or am I missing something?

>> +             diff "$EXPECT_HEADER" commit_headers.$$ > /dev/null
>
> Is "test_cmp" inappropriate here for some reason?

It seems appropriate, but this will require a ". ./test-lib.sh" from inside
lib-rebase.sh, which fails because at that point we are already in the trash
directory for the test, the solution is to do a ". $TEST_DIRECTORY/test-lib.sh"
instead but that gives more errors due to test-lib.sh assumes that it is always
being "sourced" when $CWD is $TEST_DIRECTORY, i think.

>
>> +     ) ||
>
> Do you need a subshell for this, or just a grouping {} sufficient?

Yes a grouping {} is sufficient.

Note: The only user of the feature that this patch provides is my patch to
t3404-rebase-interactive.sh
(1278896663-3922-2-git-send-email-ayiehere@gmail.com),
the rationale of which is discussed in 7vbpadfd4r.fsf@alter.siamese.dyndns.org.

I'll send a reply to that one in a bit.


nazri.

Re: [PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[Jay Soffian]

On Mon, Jul 12, 2010 at 8:36 PM, Nazri Ramliy <ayiehere@gmail.com> wrote:
> If I were to use a single "sed" invocation here's how it's going to look like:
>
>        sed -e "s/^pick [0-9a-f]\+//" < "$1" > commit_headers.$$
>
> But doing so reminds me of this:
>
> On Sat, Jul 10, 2010 at 6:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nazri Ramliy <ayiehere@gmail.com> writes:
>>
>>> +                     sed -i "s/^\([a-z]\+\) [0-9a-f]\+ /\1 $REPLACE_COMMIT_ID /" \
>>
>> This is not portable. Escaping an ERE element with a backslash does not
>> make it suitable for use in BRE that sed uses.
>
> Aren't we back to square one? Or am I missing something?

sed 's/^pick [0-9a-f]\{1,\} //'

is a valid posix BRE. Alternately + can be expressed as:

sed 's/^pick [0-9a-f][0-9a-f]* //'


>>> +             diff "$EXPECT_HEADER" commit_headers.$$ > /dev/null
>>
>> Is "test_cmp" inappropriate here for some reason?
>
> It seems appropriate, but this will require a ". ./test-lib.sh" from inside
> lib-rebase.sh, which fails because at that point we are already in the trash
> directory for the test, the solution is to do a ". $TEST_DIRECTORY/test-lib.sh"
> instead but that gives more errors due to test-lib.sh assumes that it is always
> being "sourced" when $CWD is $TEST_DIRECTORY, i think.

Don't the scripts that source lib-rebase.sh all source test-lib.sh
ahead of it? lib-rebase.sh shouldn't need to re-source test-lib.sh.
No?

j.

Re: [PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[Nazri Ramliy]

On Tue, Jul 13, 2010 at 8:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Come on ;-) The most portable, traditional and straightforward way to
> spell that would be "[a-z][a-z]*", no?

On Tue, Jul 13, 2010 at 9:39 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> sed 's/^pick [0-9a-f]\{1,\} //'
>
> is a valid posix BRE. Alternately + can be expressed as:
>
> sed 's/^pick [0-9a-f][0-9a-f]* //'

Thanks for the cluestick guys.

On Tue, Jul 13, 2010 at 8:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Somebody is already using test_set_editor inside lib-rebase.sh --- where
> else does it come from other than test-lib?  Don't the scripts that
> include lib-rebase already include test-lib.sh to make test_cmp available
> to you?

On Tue, Jul 13, 2010 at 9:39 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> Don't the scripts that source lib-rebase.sh all source test-lib.sh
> ahead of it? lib-rebase.sh shouldn't need to re-source test-lib.sh.
> No?

Ah, sorry for not reporting the exact cause of the problem, it is
actually due to the fact that test_cmp will be called (later) by
"fake-editor.sh", which does not source test-lib.sh, and sourcing
test-lib.sh will not be a correct solution in that case yes?

nazri.

Re: [PATCH 1/2] lib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

[Junio C Hamano]

Nazri Ramliy <ayiehere@gmail.com> writes:

> On Tue, Jul 13, 2010 at 9:39 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
>> Don't the scripts that source lib-rebase.sh all source test-lib.sh
>> ahead of it? lib-rebase.sh shouldn't need to re-source test-lib.sh.
>> No?
>
> Ah, sorry for not reporting the exact cause of the problem, it is
> actually due to the fact that test_cmp will be called (later) by
> "fake-editor.sh", which does not source test-lib.sh, and sourcing
> test-lib.sh will not be a correct solution in that case yes?

Ahh, I see.

As we _only_ care about the files being equal (and we do not want any
input from that context even when we are running in verbose mode), I think
it is probably Ok to use bare "diff" like your patch did.  Thanks for an
explanation.

People with platforms with a broken "diff" implementations, PLEASE
complain LOUDLY if your "diff" is not even capable of comparing two
regular files and report if they are the same with its exit status.