# [BUG] git-rebase fails when a commit message contains a diff 17 messages from 2007-11-09 to 2007-11-11. Participants: Jonas Fonseca, Junio C Hamano, Johannes Schindelin, Steffen Prohaska, Benoit Sigoure, Johannes Sixt, Ralf Wildenhues, Andreas Ericsson, Björn Steinbrink. Thread: https://gitlist.dev/t/10746 ## Jonas Fonseca, 2007-11-09 01:12 Subject: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <20071109011214.GA5903@diku.dk> URL: https://gitlist.dev/e/20071109011214.GA5903%40diku.dk ``` 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 < W && + git add W && + git commit -F commitdiff-message && + git rebase master +' + test_done -- Jonas Fonseca ``` ## Junio C Hamano, 2007-11-09 01:39 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <7vlk98xkm3.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vlk98xkm3.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071109011214.GA5903@diku.dk> ``` 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, 2007-11-09 01:51 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <7vhcjwxk1s.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhcjwxk1s.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7vlk98xkm3.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano 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, 2007-11-09 02:18 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <7v640cxitc.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v640cxitc.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7vhcjwxk1s.fsf@gitster.siamese.dyndns.org> ``` 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 ] [])' + [--interactive] [--onto ] [])' . 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, 2007-11-09 02:28 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711090225110.4362%40racer.site In-Reply-To: <7v640cxitc.fsf@gitster.siamese.dyndns.org> ``` 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 ``` ## Junio C Hamano, 2007-11-09 02:37 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <7v1wb0xhxq.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1wb0xhxq.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ``` ## Steffen Prohaska, 2007-11-09 07:03 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: URL: https://gitlist.dev/e/B2BAA0E1-8DA7-417D-AE25-53D8690810ED%40zib.de In-Reply-To: <7vhcjwxk1s.fsf@gitster.siamese.dyndns.org> ``` On Nov 9, 2007, at 2:51 AM, Junio C Hamano wrote: > Junio C Hamano 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 ``` ## Benoit Sigoure, 2007-11-09 07:57 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <6FCE17E3-9FAA-4676-B12A-369B31743DA6@lrde.epita.fr> URL: https://gitlist.dev/e/6FCE17E3-9FAA-4676-B12A-369B31743DA6%40lrde.epita.fr In-Reply-To: ``` 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, 2007-11-09 08:57 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <473420FE.7010807@viscovery.net> URL: https://gitlist.dev/e/473420FE.7010807%40viscovery.net In-Reply-To: <6FCE17E3-9FAA-4676-B12A-369B31743DA6@lrde.epita.fr> ``` 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 ``` ## Ralf Wildenhues, 2007-11-09 09:05 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <20071109090509.GA20121@ins.uni-bonn.de> URL: https://gitlist.dev/e/20071109090509.GA20121%40ins.uni-bonn.de In-Reply-To: <473420FE.7010807@viscovery.net> ``` * 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 ``` ## Benoit Sigoure, 2007-11-09 09:07 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <87747E02-0C25-4330-84F4-1C56C0196BD5@lrde.epita.fr> URL: https://gitlist.dev/e/87747E02-0C25-4330-84F4-1C56C0196BD5%40lrde.epita.fr In-Reply-To: <473420FE.7010807@viscovery.net> ``` 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 ``` ## Andreas Ericsson, 2007-11-09 10:29 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <47343683.7080904@op5.se> URL: https://gitlist.dev/e/47343683.7080904%40op5.se In-Reply-To: ``` 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 ``` ## Johannes Schindelin, 2007-11-09 13:20 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711091319420.4362%40racer.site In-Reply-To: <6FCE17E3-9FAA-4676-B12A-369B31743DA6@lrde.epita.fr> ``` 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 ``` ## Johannes Schindelin, 2007-11-09 14:18 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711091417500.4362%40racer.site In-Reply-To: ``` Hi, On Fri, 9 Nov 2007, Steffen Prohaska wrote: > On Nov 9, 2007, at 2:51 AM, Junio C Hamano wrote: > > > Junio C Hamano 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 ``` ## Johannes Schindelin, 2007-11-09 14:19 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711091418510.4362%40racer.site In-Reply-To: <7v1wb0xhxq.fsf@gitster.siamese.dyndns.org> ``` Hi, On Thu, 8 Nov 2007, Junio C Hamano wrote: > Johannes Schindelin 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 ``` ## Steffen Prohaska, 2007-11-09 14:29 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <04B901C5-E8A4-494D-A8D0-4D430F818803@zib.de> URL: https://gitlist.dev/e/04B901C5-E8A4-494D-A8D0-4D430F818803%40zib.de In-Reply-To: ``` 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 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 ``` ## Björn Steinbrink, 2007-11-11 22:31 Subject: Re: [BUG] git-rebase fails when a commit message contains a diff Message-ID: <20071111223134.GA12699@atjola.homenet> URL: https://gitlist.dev/e/20071111223134.GA12699%40atjola.homenet In-Reply-To: <7v1wb0xhxq.fsf@gitster.siamese.dyndns.org> ``` On 2007.11.08 18:37:37 -0800, Junio C Hamano wrote: > Johannes Schindelin 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 ```