threads / bug / 10746

[BUG] git-rebase fails when a commit message contains a diff

Subject: [BUG] git-rebase fails when a commit message contains a diff

## tl;dr

17 messages between Nov 9, 2007 and Nov 11, 2007.

replies: 16people: 9as markdown or json

Jonas Fonseca· Nov 9, 2007, 01:12 UTC · lore

I have concocted a test that illustrates the problem: patch below. The problem is that the patches in .dotest are not properly formatted to make it easy to distinguish commit message from commit diff.

When running the test git-rebase fails with:
	Applying Test message with diff in it
	error: config: does not exist in index
	fatal: sha1 information is lacking or useless (config).
	Repository lacks necessary blobs to fall back on 3-way merge.
	Cannot fall back to three-way merge.
	Patch failed at 0004.

By modifying the test below, a similar problem related with handling of the end-of-message marker (---\n) can be exposed. A commit message that contains this marker and text afterwards ends up only having the text preceeding the marker.

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 95e33b5..75babdb 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -83,4 +83,25 @@ test_expect_success 'rebase a single mode change' '
      GIT_TRACE=1 git rebase master
 '
 
+cat >commitdiff-message <<EOF
+Test message with diff in it
+
+--- .git/config	2007-11-02 22:44:55.000000000 +0100
++++ .git/config+	2007-11-09 01:40:30.000000000 +0100
+@@ -1,3 +1,4 @@
++# Broken
+ [core]
+ 	repositoryformatversion = 0
+ 	filemode = true
+EOF
+
+test_expect_success 'rebase commit with diff in the commit message' \
+'
+     git checkout -b diff-msg upstream-merged-nonlinear &&
+     echo 1 > W &&
+     git add W &&
+     git commit -F commitdiff-message &&
+     git rebase master
+'
+
 test_done

-- 
Jonas Fonseca
Junio C Hamano· Nov 9, 2007, 01:39 UTC · re: Jonas Fonseca · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

That's a known design limitation of applymbox/mailinfo. Any line that looks like a beginning of a patch in e-mail ("^--- ", "^---$", "^diff -", and "^Index: ") terminates the commit log.

Junio C Hamano· Nov 9, 2007, 01:51 UTC · re: Junio C Hamano · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Junio C Hamano <gitster@pobox.com> writes:
> That's a known design limitation of applymbox/mailinfo.  Any
> line that looks like a beginning of a patch in e-mail ("^--- ",
> "^---$", "^diff -", and "^Index: ") terminates the commit log.
Ok, so that explains the symptom.  What's the next step?
 * The applymbox/mailinfo pair should continue to split the
   commit log message at the first such line.  There is no point
   breaking established workflow, and people in communities that
   exchange patches via e-mail already know to avoid this issue
   by indenting quoted diff snippet in the log message,
   e.g. 5be507fc.
 * There is no fundamental reason for rebase to use e-mail
   format to express what "format-patch | am -3" pipeline does.
   We do it currently because (1) it was expedient to reuse what
   was already there, and because (2) the original target
   audience of git are e-mail oriented communities, so there was
   not strong incentive to make rebase independent of the
   applymbox/mailinfo limitation (that is, even if you make
   rebase able to handle such a patch, you cannot send out the
   result over e-mail *anyway*).
   This however does not mean we should always use merging
   rebase.  patch based approach "format-patch | am -3" pipeline
   uses is often much faster.  Instead of using "format-patch |
   am -3", we could use more careful patch and message
   generation, like git-rebase--interactive.sh:make_patch()
   does.
Junio C Hamano· Nov 9, 2007, 02:18 UTC · re: Junio C Hamano · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

I wonder if this is a sensible thing to do, regardless of the issue of commit log message that contains anything.

The patch replaces git-rebase with git-rebase--interactive. The only difference from the existing "git-rebase -i" is if the command is called without "-i" the initial "here is the to-do list. please rearrange the lines, modify 'pick' to 'edit' or whatever as appropriate" step is done without letting the user edit the list.

---
 Makefile                                    |    2 +-
 git-rebase--interactive.sh => git-rebase.sh |   14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index 0d5590f..f0e5e38 100644
--- a/Makefile
+++ b/Makefile
@@ -214,7 +214,7 @@ SCRIPT_SH = \
 	git-clean.sh git-clone.sh git-commit.sh \
 	git-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
-	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
+	git-pull.sh git-rebase.sh \
 	git-repack.sh git-request-pull.sh \
 	git-sh-setup.sh \
 	git-am.sh \
diff --git a/git-rebase--interactive.sh b/git-rebase.sh
similarity index 98%
rename from git-rebase--interactive.sh
rename to git-rebase.sh
index 76dc679..1dd6f6d 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase.sh
@@ -11,7 +11,7 @@
 # http://article.gmane.org/gmane.comp.version-control.git/22407
 
 USAGE='(--continue | --abort | --skip | [--preserve-merges] [--verbose]
-	[--onto <branch>] <upstream> [<branch>])'
+	[--interactive] [--onto <branch>] <upstream> [<branch>])'
 
 . git-sh-setup
 require_work_tree
@@ -25,6 +25,7 @@ REWRITTEN="$DOTEST"/rewritten
 PRESERVE_MERGES=
 STRATEGY=
 VERBOSE=
+INTERACTIVE=
 test -d "$REWRITTEN" && PRESERVE_MERGES=t
 test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 test -f "$DOTEST"/verbose && VERBOSE=t
@@ -346,6 +347,9 @@ do_rest () {
 while test $# != 0
 do
 	case "$1" in
+	--interactive|-i)
+		INTERACTIVE=t
+		;;
 	--continue)
 		comment_for_reflog continue
 
@@ -504,9 +508,11 @@ EOF
 			die_abort "Nothing to do"
 
 		cp "$TODO" "$TODO".backup
-		git_editor "$TODO" ||
-			die "Could not execute editor"
-
+		case "$INTERACTIVE" in
+		t)
+			git_editor "$TODO" || die "Could not execute editor"
+			;;
+		esac
 		has_action "$TODO" ||
 			die_abort "Nothing to do"
Johannes Schindelin· Nov 9, 2007, 02:28 UTC · re: Junio C Hamano · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Hi,
On Thu, 8 Nov 2007, Junio C Hamano wrote:
Show 8 quoted lines
> I wonder if this is a sensible thing to do, regardless of the issue of 
> commit log message that contains anything.
> 
> The patch replaces git-rebase with git-rebase--interactive.  The only 
> difference from the existing "git-rebase -i" is if the command is called 
> without "-i" the initial "here is the to-do list. please rearrange the 
> lines, modify 'pick' to 'edit' or whatever as appropriate" step is done 
> without letting the user edit the list.

Hmm. I don't know, really. I had the impression that the "git format-patch | git am" pipeline would be faster.

But if we were to do this, we'd get a progress meter for free. And bugs exposed, no doubt.

>  Makefile                                    |    2 +-
>  git-rebase--interactive.sh => git-rebase.sh |   14 ++++++++++----
>  2 files changed, 11 insertions(+), 5 deletions(-)
What about the existing git-rebase.sh?
Show 14 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase.sh
> similarity index 98%
> rename from git-rebase--interactive.sh
> rename to git-rebase.sh
> index 76dc679..1dd6f6d 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase.sh
> @@ -346,6 +347,9 @@ do_rest () {
>  while test $# != 0
>  do
>  	case "$1" in
> +	--interactive|-i)
> +		INTERACTIVE=t
> +		;;
There is already a case for that, further down.
Show 12 quoted lines
> @@ -504,9 +508,11 @@ EOF
>  			die_abort "Nothing to do"
>  
>  		cp "$TODO" "$TODO".backup
> -		git_editor "$TODO" ||
> -			die "Could not execute editor"
> -
> +		case "$INTERACTIVE" in
> +		t)
> +			git_editor "$TODO" || die "Could not execute editor"
> +			;;
> +		esac
Would that not be easier to read as
		test t = "$INTERACTIVE" &&
			git_editor "$TODO" || die "Could not execute editor"
Hmm?

Ciao, Dscho

Junio C Hamano· Nov 9, 2007, 02:37 UTC · re: Johannes Schindelin · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 13 quoted lines
> On Thu, 8 Nov 2007, Junio C Hamano wrote:
>
>> I wonder if this is a sensible thing to do, regardless of the issue of 
>> commit log message that contains anything.
>> 
>> The patch replaces git-rebase with git-rebase--interactive.  The only 
>> difference from the existing "git-rebase -i" is if the command is called 
>> without "-i" the initial "here is the to-do list. please rearrange the 
>> lines, modify 'pick' to 'edit' or whatever as appropriate" step is done 
>> without letting the user edit the list.
>
> Hmm.  I don't know, really.  I had the impression that the "git 
> format-patch | git am" pipeline would be faster.
Heh, I did not read rebase--interactive carefully enough.

Unless told to use merge with "rebase -m", rebase replays the change by extracting and applying patches, and speed comparison was about that vs merge based replaying; I thought make_patch was done in order to avoid using cherry-pick (which is based on merge-recursive) and doing patch application with three-way fallback. Apparently that is not what "interactive" does.

Perhaps pick_one () could be taught to perform the 3-way fallback dance git-am plays correctly. The patch I sent to make git-rebase--interactive take over git-rebase would then become quite reasonable, I would think.

Johannes Schindelin· Nov 9, 2007, 14:19 UTC · re: Junio C Hamano · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Hi,
On Thu, 8 Nov 2007, Junio C Hamano wrote:
Show 29 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Thu, 8 Nov 2007, Junio C Hamano wrote:
> >
> >> I wonder if this is a sensible thing to do, regardless of the issue 
> >> of commit log message that contains anything.
> >> 
> >> The patch replaces git-rebase with git-rebase--interactive.  The only 
> >> difference from the existing "git-rebase -i" is if the command is 
> >> called without "-i" the initial "here is the to-do list. please 
> >> rearrange the lines, modify 'pick' to 'edit' or whatever as 
> >> appropriate" step is done without letting the user edit the list.
> >
> > Hmm.  I don't know, really.  I had the impression that the "git 
> > format-patch | git am" pipeline would be faster.
> 
> Heh, I did not read rebase--interactive carefully enough.
> 
> Unless told to use merge with "rebase -m", rebase replays the change by 
> extracting and applying patches, and speed comparison was about that vs 
> merge based replaying; I thought make_patch was done in order to avoid 
> using cherry-pick (which is based on merge-recursive) and doing patch 
> application with three-way fallback.  Apparently that is not what 
> "interactive" does.
> 
> Perhaps pick_one () could be taught to perform the 3-way fallback dance 
> git-am plays correctly.  The patch I sent to make 
> git-rebase--interactive take over git-rebase would then become quite 
> reasonable, I would think.

I have a different idea: How about changing cherry-pick to try a simple patch first? If that fails, we can always go back to merge-recursive (or merge-nu once that is ready).

Ciao, Dscho

Björn Steinbrink· Nov 11, 2007, 22:31 UTC · re: Junio C Hamano · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

On 2007.11.08 18:37:37 -0800, Junio C Hamano wrote:
Show 29 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Thu, 8 Nov 2007, Junio C Hamano wrote:
> >
> >> I wonder if this is a sensible thing to do, regardless of the issue of 
> >> commit log message that contains anything.
> >> 
> >> The patch replaces git-rebase with git-rebase--interactive.  The only 
> >> difference from the existing "git-rebase -i" is if the command is called 
> >> without "-i" the initial "here is the to-do list. please rearrange the 
> >> lines, modify 'pick' to 'edit' or whatever as appropriate" step is done 
> >> without letting the user edit the list.
> >
> > Hmm.  I don't know, really.  I had the impression that the "git 
> > format-patch | git am" pipeline would be faster.
> 
> Heh, I did not read rebase--interactive carefully enough.
> 
> Unless told to use merge with "rebase -m", rebase replays the
> change by extracting and applying patches, and speed comparison
> was about that vs merge based replaying; I thought make_patch
> was done in order to avoid using cherry-pick (which is based on
> merge-recursive) and doing patch application with three-way
> fallback.  Apparently that is not what "interactive" does.
> 
> Perhaps pick_one () could be taught to perform the 3-way
> fallback dance git-am plays correctly.  The patch I sent to make
> git-rebase--interactive take over git-rebase would then become
> quite reasonable, I would think.

Note that git-rebase--interactive also doesn't really support the --strategy parameter which git-rebase handles using git-merge-* instead of git-am. Only merge commits during a -i -p run actually apply the strategy.

Björn
Benoit Sigoure· Nov 9, 2007, 07:57 UTC · re: Johannes Schindelin · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

On Nov 9, 2007, at 3:28 AM, Johannes Schindelin wrote:
> Would that not be easier to read as
>
> 		test t = "$INTERACTIVE" &&
> 			git_editor "$TODO" || die "Could not execute editor"
Hmm this will `die' if you're not running interactively.

Off topic question: why do you guys always do this instead of doing, say, this:

INTERACTIVE=false
case $1 in
.
.
.
   --interactive|-i)
     INTERACTIVE=:
     ... ;;
esac
.
.
.
if $INTERACTIVE; then
   git_editor "$TODO" || die ...
fi
?
-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory
Johannes Sixt· Nov 9, 2007, 08:57 UTC · re: Benoit Sigoure · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Benoit Sigoure schrieb:
Show 13 quoted lines
> Off topic question: why do you guys always do this instead of doing, 
> say, this:
> 
> INTERACTIVE=false
> 
> case $1 in
>   --interactive|-i)
>     INTERACTIVE=:
>     ... ;;
> esac
> if $INTERACTIVE; then
>   git_editor "$TODO" || die ...
> fi
Because in some shells 'false' is not a built-in.
But then this might do it without the extra process:
	INTERACTIVE="! :"	# false
	case $1 in
	--interactive|-i)
	    INTERACTIVE=:
	    ... ;;
	esac
	if $INTERACTIVE; then
	  git_editor "$TODO" || die ...
	fi
-- Hannes
Ralf Wildenhues· Nov 9, 2007, 09:05 UTC · re: Johannes Sixt · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

* Johannes Sixt wrote on Fri, Nov 09, 2007 at 09:57:34AM CET:
Show 5 quoted lines
> Benoit Sigoure schrieb:
>> Off topic question: why do you guys always do this instead of doing, say, 
>> this:
>>
>> INTERACTIVE=false
[...]
Show 5 quoted lines
>> if $INTERACTIVE; then
>>   git_editor "$TODO" || die ...
>> fi
>
> Because in some shells 'false' is not a built-in.
[...]
>         INTERACTIVE="! :"       # false

Which shells do not have 'false' but do support '!' as process exit status negation? For them, is it important to save another fork? Solaris sh has neither (it will error on the '!'), but it's ruled out for git anyway.

Cheers, Ralf

Benoit Sigoure· Nov 9, 2007, 09:07 UTC · re: Johannes Sixt · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

On Nov 9, 2007, at 9:57 AM, Johannes Sixt wrote:
Show 14 quoted lines
> Benoit Sigoure schrieb:
>> Off topic question: why do you guys always do this instead of  
>> doing, say, this:
>> INTERACTIVE=false
>> case $1 in
>>   --interactive|-i)
>>     INTERACTIVE=:
>>     ... ;;
>> esac
>> if $INTERACTIVE; then
>>   git_editor "$TODO" || die ...
>> fi
>
> Because in some shells 'false' is not a built-in.

Can you name such a shell? (besides Solaris' brain-damaged, b0rken and foobared /bin/sh which will most likely not work with Git anyway)

> But then this might do it without the extra process:
>
> 	INTERACTIVE="! :"	# false

`!' is not portable either. In particular, I highly doubt `!' will work on shells that don't have `false' as a builtin (Hello Mr Solaris).

Show 9 quoted lines
>
> 	case $1 in
> 	--interactive|-i)
> 	    INTERACTIVE=:
> 	    ... ;;
> 	esac
> 	if $INTERACTIVE; then
> 	  git_editor "$TODO" || die ...
> 	fi

Correct me if I'm wrong but I think that some shells don't have `test' as a builtin either.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory
Johannes Schindelin· Nov 9, 2007, 13:20 UTC · re: Benoit Sigoure · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Hi,
On Fri, 9 Nov 2007, Benoit Sigoure wrote:
Show 8 quoted lines
> On Nov 9, 2007, at 3:28 AM, Johannes Schindelin wrote:
> 
> > Would that not be easier to read as
> > 
> > 		test t = "$INTERACTIVE" &&
> > 			git_editor "$TODO" || die "Could not execute editor"
> 
> Hmm this will `die' if you're not running interactively.
D'oh.  Of course I meant
		test -z "$INTERACTIVE" || git_editor "$TODO" || die ...

Ciao, Dscho

Andreas Ericsson· Nov 9, 2007, 10:29 UTC · re: Johannes Schindelin · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Johannes Schindelin wrote:
Show 12 quoted lines
>> +		case "$INTERACTIVE" in
>> +		t)
>> +			git_editor "$TODO" || die "Could not execute editor"
>> +			;;
>> +		esac
> 
> 
> Would that not be easier to read as
> 
> 		test t = "$INTERACTIVE" &&
> 			git_editor "$TODO" || die "Could not execute editor"
>

Written like that, it would die every time $INTERACTIVE isn't "t". You'd need curly braces around 

	git_editor "$TODO" || die "Could not execute editor"
-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
Steffen Prohaska· Nov 9, 2007, 07:03 UTC · re: Junio C Hamano · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

On Nov 9, 2007, at 2:51 AM, Junio C Hamano wrote:
Show 14 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> That's a known design limitation of applymbox/mailinfo.  Any
>> line that looks like a beginning of a patch in e-mail ("^--- ",
>> "^---$", "^diff -", and "^Index: ") terminates the commit log.
>
> Ok, so that explains the symptom.  What's the next step?
>
>  * The applymbox/mailinfo pair should continue to split the
>    commit log message at the first such line.  There is no point
>    breaking established workflow, and people in communities that
>    exchange patches via e-mail already know to avoid this issue
>    by indenting quoted diff snippet in the log message,
>    e.g. 5be507fc.
I wasn't aware of this.

Maybe git-commit should validate commit messages? If people have a notion of a well-formed commit message this should be verified. The message should not contain strings that indicate termination of the commit message when sent by email. If commit did this the evil strings would never enter a commit message in the first place.

Actually I sometimes use '---' as a separator in text. I'm sure I'll forget at some point that I must not use it in a commit message. I'd be happy if git saved me from this.

If I understand correctly, the problem will remain when sending patches by emails; even if git-rebase was changed to use merge. Or does git-format-patch quote evil strings in commit messages?

	Steffen
Johannes Schindelin· Nov 9, 2007, 14:18 UTC · re: Steffen Prohaska · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

Hi,
On Fri, 9 Nov 2007, Steffen Prohaska wrote:
Show 18 quoted lines
> On Nov 9, 2007, at 2:51 AM, Junio C Hamano wrote:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> > 
> > > That's a known design limitation of applymbox/mailinfo.  Any
> > > line that looks like a beginning of a patch in e-mail ("^--- ",
> > > "^---$", "^diff -", and "^Index: ") terminates the commit log.
> > 
> > Ok, so that explains the symptom.  What's the next step?
> > 
> >  * The applymbox/mailinfo pair should continue to split the
> >    commit log message at the first such line.  There is no point
> >    breaking established workflow, and people in communities that
> >    exchange patches via e-mail already know to avoid this issue
> >    by indenting quoted diff snippet in the log message,
> >    e.g. 5be507fc.
> 
> I wasn't aware of this.
But there's a really easy workaround: use --merge with git rebase.

Ciao, Dscho

Steffen Prohaska· Nov 9, 2007, 14:29 UTC · re: Johannes Schindelin · lore

Re: [BUG] git-rebase fails when a commit message contains a diff

On Nov 9, 2007, at 3:18 PM, Johannes Schindelin wrote:
Show 24 quoted lines
> Hi,
>
> On Fri, 9 Nov 2007, Steffen Prohaska wrote:
>
>> On Nov 9, 2007, at 2:51 AM, Junio C Hamano wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> That's a known design limitation of applymbox/mailinfo.  Any
>>>> line that looks like a beginning of a patch in e-mail ("^--- ",
>>>> "^---$", "^diff -", and "^Index: ") terminates the commit log.
>>>
>>> Ok, so that explains the symptom.  What's the next step?
>>>
>>>  * The applymbox/mailinfo pair should continue to split the
>>>    commit log message at the first such line.  There is no point
>>>    breaking established workflow, and people in communities that
>>>    exchange patches via e-mail already know to avoid this issue
>>>    by indenting quoted diff snippet in the log message,
>>>    e.g. 5be507fc.
>>
>> I wasn't aware of this.
>
> But there's a really easy workaround: use --merge with git rebase.

It's no longer a problem for me. I now know the limitation and the work around. But others could get bitten, too. We could save them.

	Steffen

← back to recent threads