threads / patch / 24357

patch, 2 partslib-rebase.sh: fake-editor.sh: Allow checking of commit header(s) in $GIT_EDITOR

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

## tl;dr

12 messages between Jul 10, 2010 and Jul 13, 2010. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Nazri Ramliy· Jul 10, 2010, 12:27 UTC · lore
Signed-off-by: Nazri Ramliy <ayiehere@gmail.com>
---
 t/lib-rebase.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
Show changes to t/lib-rebase.sh +8 −0
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
Nazri Ramliy· Jul 10, 2010, 12:27 UTC · re: Nazri Ramliy · lore

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

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(-)
Show changes to 2 files +18 −8

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

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
Nazri Ramliy· Jul 10, 2010, 16:35 UTC · re: Nazri Ramliy · lore

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

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.
Junio C Hamano· Jul 12, 2010, 06:05 UTC · re: Nazri Ramliy · lore

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

Nazri Ramliy <ayiehere@gmail.com> writes:
Show 6 quoted lines
> 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?

Nazri Ramliy· Jul 13, 2010, 01:46 UTC · re: Junio C Hamano · lore

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

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?
Show 5 quoted lines
> 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
Junio C Hamano· Jul 13, 2010, 05:06 UTC · re: Nazri Ramliy · lore

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

Nazri Ramliy <ayiehere@gmail.com> writes:
Show 5 quoted lines
> 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)?

Nazri Ramliy· Jul 13, 2010, 05:19 UTC · re: Junio C Hamano · lore

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

On Tue, Jul 13, 2010 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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.
Junio C Hamano· Jul 12, 2010, 06:08 UTC · re: Nazri Ramliy · lore

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

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
Nazri Ramliy· Jul 13, 2010, 00:36 UTC · re: Junio C Hamano · lore

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

On Mon, Jul 12, 2010 at 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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:
Show 6 quoted lines
> 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.
Jay Soffian· Jul 13, 2010, 01:39 UTC · re: Nazri Ramliy · lore

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

On Mon, Jul 12, 2010 at 8:36 PM, Nazri Ramliy <ayiehere@gmail.com> wrote:
Show 15 quoted lines
> 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]* //'
Show 9 quoted lines
>>> +             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.
Nazri Ramliy· Jul 13, 2010, 01:58 UTC · re: Jay Soffian · lore

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

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:
Show 5 quoted lines
> 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.
Junio C Hamano· Jul 13, 2010, 05:13 UTC · re: Nazri Ramliy · lore

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

Nazri Ramliy <ayiehere@gmail.com> writes:
Show 9 quoted lines
> 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.

← back to recent threads