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

[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    |   25 ++++++++++++++-----------
 t/t3404-rebase-interactive.sh |   13 +++++++++++++
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 31e6860..726cb6a 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -889,14 +889,15 @@ first and then run 'git rebase --continue' again."
 		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
-		do
-			if test t != "$PRESERVE_MERGES"
-			then
-				echo "pick $shortsha1 $rest" >> "$TODO"
-			else
+			$REVISIONS | sed -n "s/^>//p" > "$TODO.tmp"
+
+		if test t != "$PRESERVE_MERGES"
+		then
+			cat "$TODO.tmp" | sed "s/^/pick /" > "$TODO"
+		else
+			cat "$TODO.tmp" |
+			while read -r shortsha1 rest
+			do
 				sha1=$(git rev-parse $shortsha1)
 				if test -z "$REBASE_ROOT"
 				then
@@ -914,10 +915,12 @@ first and then run 'git rebase --continue' again."
 				if test f = "$preserve"
 				then
 					touch "$REWRITTEN"/$sha1
-					echo "pick $shortsha1 $rest" >> "$TODO"
+					grep "^$shortsha1" "$TODO.tmp" | sed "s/^/pick /" >> "$TODO"
 				fi
-			fi
-		done
+			done
+
+		fi
+		rm -f "$TODO.tmp"
 
 		# Watch for commits that been dropped by --cherry-pick
 		if test t = "$PRESERVE_MERGES"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 47ca88f..2c3a0b9 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -648,4 +648,17 @@ test_expect_success 'rebase-i history with funny messages' '
 	test_cmp expect actual
 '
 
+cat >expect <<EOF
+pick COMMIT_ID   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" &&
+	REPLACE_COMMIT_ID=COMMIT_ID EXPECT_NON_COMMENT_LINES="expect" 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

[Junio C Hamano]

Nazri Ramliy <ayiehere@gmail.com> writes:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 31e6860..726cb6a 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -889,14 +889,15 @@ first and then run 'git rebase --continue' again."
>  		fi
>  		git rev-list $MERGES_OPTION --pretty=oneline --abbrev-commit \
>  			--abbrev=7 --reverse --left-right --topo-order \
> +			$REVISIONS | sed -n "s/^>//p" > "$TODO.tmp"
> +
> +		if test t != "$PRESERVE_MERGES"
> +		then
> +			cat "$TODO.tmp" | sed "s/^/pick /" > "$TODO"

Do not cat a single file into a pipeline.

	sed "s/^/pick /" <"$TODO.tmp" >"$TODO"

> +		else
> +			cat "$TODO.tmp" |

Likewise.

> +			while read -r shortsha1 rest
> +			do
>  				sha1=$(git rev-parse $shortsha1)
>  				if test -z "$REBASE_ROOT"
>  				then
> @@ -914,10 +915,12 @@ first and then run 'git rebase --continue' again."
>  				if test f = "$preserve"
>  				then
>  					touch "$REWRITTEN"/$sha1
> +					grep "^$shortsha1" "$TODO.tmp" | sed "s/^/pick /" >> "$TODO"

Don't pipe output of grep into sed.

	sed -ne "/^$shortsha1 /s/^/pick /p" <"$TODO.tmp" >>"$TODO"

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

[Nazri Ramliy]

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.
>
> Do we use in-place replacement anywhere else with sed?  I don't think it
> is portable, either.
>

And later ...

On Sat, Jul 10, 2010 at 6:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +                     cat "$TODO.tmp" | sed "s/^/pick /" > "$TODO"
>
> Do not cat a single file into a pipeline.
>
>        sed "s/^/pick /" <"$TODO.tmp" >"$TODO"
>
>> +             else
>> +                     cat "$TODO.tmp" |
>
> Likewise.
>
[snip]
> Don't pipe output of grep into sed.
>
>        sed -ne "/^$shortsha1 /s/^/pick /p" <"$TODO.tmp" >>"$TODO"

I'll send out a v2 of this series that addresses the above issues in
better ways (i think).

nazri

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

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