[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 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 actual commit header
	2. Add a test for it

Signed-off-by: Nazri Ramliy <ayiehere@gmail.com>
---
 git-rebase--interactive.sh    |   14 ++++++--------
 t/t3404-rebase-interactive.sh |   12 ++++++++++++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 31e6860..b13f42e 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -887,17 +887,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 -r 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"
+				git log -1 --abbrev-commit --abbrev=7 --format="pick %h %s" $sha1 > "$TODO"
 			else
-				sha1=$(git rev-parse $shortsha1)
 				if test -z "$REBASE_ROOT"
 				then
 					preserve=t
@@ -914,7 +912,7 @@ first and then run 'git rebase --continue' again."
 				if test f = "$preserve"
 				then
 					touch "$REWRITTEN"/$sha1
-					echo "pick $shortsha1 $rest" >> "$TODO"
+					git log -1 --abbrev-commit --abbrev=7 --format="pick %h %s" $sha1 > "$TODO"
 				fi
 			fi
 		done
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 47ca88f..78d0041 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -648,4 +648,16 @@ test_expect_success 'rebase-i history with funny messages' '
 	test_cmp expect actual
 '
 
+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 2/2] rebase -i: Preserve whitespace at beginning of commit header in $GIT_EDITOR

[Nazri Ramliy]

On Sat, Jul 10, 2010 at 8:27 PM, Nazri Ramliy <ayiehere@gmail.com> wrote:
> -                               echo "pick $shortsha1 $rest" >> "$TODO"
> +                               git log -1 --abbrev-commit --abbrev=7 --format="pick %h %s" $sha1 > "$TODO"
[snip]
> -                                       echo "pick $shortsha1 $rest" >> "$TODO"
> +                                       git log -1 --abbrev-commit --abbrev=7 --format="pick %h %s" $sha1 > "$TODO"

Notice the change from ">>" to ">"? I'm stupid :)

Please drop this patch. I'll resend the corrected one.

nazri.

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

[Junio C Hamano]

Nazri Ramliy <ayiehere@gmail.com> writes:

> 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

If the current code removes whitespaces at the beginning, I would actually
say that it is cleaning up the mess while preparing the instruction sheet
for you to edit, i.e. it is a good thing, and the patch might be making
things worse.

I find it difficult to come up with good reasons to convince myself that I
should be interested in what this patch tries to do.  Here are some of the
things that came to my mind while doing so.

What happens if you have trailing whitespaces, excess whitespaces in the
middle, etc. with or without this patch?  What _should_ happen in an ideal
world?

What happens if you have a malformed commit object whose first line is
blank (i.e. no "Subject" line), or there is _no_ commit log message
whatsoever with or without this patch?  What _should_ happen in an ideal
world?

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

[Nazri Ramliy]

On Mon, Jul 12, 2010 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If the current code removes whitespaces at the beginning, I would actually
> say that it is cleaning up the mess while preparing the instruction sheet
> for you to edit, i.e. it is a good thing, and the patch might be making
> things worse.

Consider this situation:

	$ git log --reverse --oneline HEAD~3..
	cafebabe a commit header
	deadbeef   badly formatted commit header
	falaafe1 another commit header

Without the patch (let's call this "exhibit 1"):

	$ GIT_EDITOR=cat git rebase -i HEAD~3
	cafebabe a commit header
	deadbeef badly formatted commit header
	fa1aafe1 another commit header

With the patch (and let's call this "exhibit 2"):

	$ GIT_EDITOR=cat git rebase -i HEAD~3
	cafebabe a commit header
	deadbeef    badly formatted commit header
	fa1aafe1 another commit header

> I find it difficult to come up with good reasons to convince myself that I
> should be interested in what this patch tries to do.

I guess it boils down to your answer to this:

When you want to reword the badly formatted commit header, would you
prefer the see the output like the one shown in exhibit 1 over that of
exhibit 2 in your $GIT_EDITOR?

The example I gave has only one commit header with beginning whitespace.
What if for some reason you end up with more than a couple of commits
headers that have whitespaces at the beginning and you would like to
rebase them to fix it and what you see is the one similar to exhibit 1?

Isn't it harder to distinguish those badly formatted commits in that case?

> Here are some of the things that came to my mind while doing so.
>
> What happens if you have trailing whitespaces, excess whitespaces in the
> middle, etc. with or without this patch?  What _should_ happen in an ideal
> world?

Trailing/excess whitespaces are non issue. The intention of this patch is to
present the commit headers "as-is".

> What happens if you have a malformed commit object whose first line is
> blank (i.e. no "Subject" line),or there is _no_ commit log message
> whatsoever with or without this patch?  What _should_ happen in an ideal
> world?

When there is no commit log message the behavior is the same with or
without this patch.

I can't test for when the commit has no "Subject" line because I don't
know how to make one.  One thing for sure that in this case, with this
patch, what is shown in $GIT_EDITOR will be exactly the same as what

        $ git log $sha1_for_that_commit --format="pick %h %s"

shows.

If you agree with my arguments here then I'll proceed with modifying the
first patch in this series as per the discussion in the thread started by
1278764821-32647-1-git-send-email-ayiehere@gmail.com.

nazri

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

[Junio C Hamano]

Nazri Ramliy <ayiehere@gmail.com> writes:

> I guess it boils down to your answer to this:
>
> When you want to reword the badly formatted commit header, would you
> prefer the see the output like the one shown in exhibit 1 over that of
> exhibit 2 in your $GIT_EDITOR?

What I was getting at was if you want to add a special case _only_ for
commits that have indented subject line, and do not care about ones that
are missing the subject line, excess whitespace in the middle or at the
end, or no commit log message.  That is why I asked the "What happens"
questions.  For example, if neither the current nor the updated code
removes excess whitespace in the middle or at the end, then that becomes a
non-issue.

I just checked.  You can create "funny commits" of various shape by
misusing hash-object (and presumably importing from foreign SCMs with
custom importers).  Since the topic here is about _fixing_ such mistakes,
seeing how we handle commits that have different but similar breakages is
very relevant before going forward.

 - We do keep excess whitespaces in the middle, so they are easy to spot.

 - A commit without any message is shown without _any_ message in the
   list, so presumably we can identify them (if there are many, you would
   want to fix them all anyway).

 - We drop trailing whitespaces, so it is impossible to spot a commit with
   such a subject line.  The user who wants to fix these breakages in the
   commit log messages presumably is intelligent enough to check with "git
   cat-file commit $that_commit | cat -e" to find such a commit, make a
   mental note of what the log message of the broken commit said, and then
   identify the one with the same message when editing the instruction
   sheet for the rebase-i, so it is not the end of the world, though.

 - A commit with an extra blank line is not shown any differently, so it
   is impossible to identify.  The "remember the message to identify"
   trick would work very well, though.

 - A commit with a space at the begining gets that excess space stripped,
   so it is impossible to identify.  Again, the "remember the message to
   identify" trick would work very well.

So the bigger picture laid out here shows that there are five cases,
including your "SP at the beginning" which is the last one.

Among these five, two are non-issues, and the remaining three share the
same "impossible to spot if you _only_ look at the insn sheet, but it is
easy to remember which ones to edit" characteristics.  Is the change that
only keeps the excess whitespaces at the beginning such an improvement
worth the code churn?

To put it differently, if we really do perceive this "impossible to spot
if you _only_ look at the insn sheet, but it is easy to remember which
ones to edit" as an issue, shouldn't the patch at least mention that it
attempts to solve only one and punts on the other two that are still to be
fixed later (so that other people can come back to help)?

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

[Nazri Ramliy]

On Tue, Jul 13, 2010 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> To put it differently, if we really do perceive this "impossible to spot
> if you _only_ look at the insn sheet, but it is easy to remember which
> ones to edit" as an issue, shouldn't the patch at least mention that it
> attempts to solve only one and punts on the other two that are still to be
> fixed later (so that other people can come back to help)?

Thanks for spending your time to clarify the issues.  I'll see if I
can improve the commit
message and/or the patch to handle those issues.

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:

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

[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