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

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

[Jonas Fonseca]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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"
 

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

[Johannes Schindelin]

Hi,

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.

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?

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

> @@ -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

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

[Junio C Hamano]

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.

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

[Steffen Prohaska]

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.

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

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

[Benoit Sigoure]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

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

[Johannes Sixt]

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.

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

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

[Ralf Wildenhues]

* Johannes Sixt wrote on Fri, Nov 09, 2007 at 09:57:34AM CET:
> Benoit Sigoure schrieb:
>> Off topic question: why do you guys always do this instead of doing, say, 
>> this:
>>
>> INTERACTIVE=false
[...]
>> 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

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

[Benoit Sigoure]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On Nov 9, 2007, at 9:57 AM, Johannes Sixt wrote:

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

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

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

[Andreas Ericsson]

Johannes Schindelin wrote:
>> +		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

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

[Johannes Schindelin]

Hi,

On Fri, 9 Nov 2007, Benoit Sigoure wrote:

> 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

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

[Johannes Schindelin]

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.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Thu, 8 Nov 2007, Junio C Hamano wrote:

> 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

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

[Steffen Prohaska]

On Nov 9, 2007, at 3:18 PM, Johannes Schindelin wrote:

> 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

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

[Björn Steinbrink]

On 2007.11.08 18:37:37 -0800, Junio C Hamano wrote:
> 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