Git Bug - diff in commit message.

Git Bug - diff in commit message.

[anikey]

Hi, peops. I'm pretty much sure that's a bug.

What I did was putting git diff (i needed to tell people that for my changes
to start working they needed to aplly message-inline patch to some code
which was not under git) in commit message. Like adding:

diff --git a/app/controllers/settings_controller.rb
b/app/controllers/settings_controller.rb
index 937da74..0e8440d 100644
--- a/app/controllers/settings_controller.rb
+++ b/app/controllers/settings_controller.rb
@@ -42,7 +42,7 @@ class SettingsController < ApplicationController
   end
 
   def snmp_mibs
-    render layout: 'ext3'
+    render layout: 'ext3_2'
   end
 
   def cfg_auth_keys(auth_type=:all)

though the commit itself didn't contain that change. So while `git rebase
some_branch_name` I started getting:

First, rewinding head to replay your work on top of it...
Applying: My cool patch.
fatal: sha1 information is lacking or useless
(app/controllers/settings_controller.rb).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 My cool patch.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

I wasn't able to figure out what was wrong for a very long time, when things
came to my mind.

Thanks.

--
View this message in context: http://git.661346.n2.nabble.com/Git-Bug-diff-in-commit-message-tp6772145p6772145.html
Sent from the git mailing list archive at Nabble.com.

Re: Git Bug - diff in commit message.

[Michael Witten]

On Thu, Sep 8, 2011 at 14:49, anikey <arty.anikey@gmail.com> wrote:
> Hi, peops. I'm pretty much sure that's a bug.
>
> What I did was putting git diff (i needed to tell people that for my changes
> to start working they needed to aplly message-inline patch to some code
> which was not under git) in commit message. Like adding:
>
> diff --git a/app/controllers/settings_controller.rb
> b/app/controllers/settings_controller.rb
> index 937da74..0e8440d 100644
> --- a/app/controllers/settings_controller.rb
> +++ b/app/controllers/settings_controller.rb
> @@ -42,7 +42,7 @@ class SettingsController < ApplicationController
>   end
>
>   def snmp_mibs
> -    render layout: 'ext3'
> +    render layout: 'ext3_2'
>   end
>
>   def cfg_auth_keys(auth_type=:all)
>
> though the commit itself didn't contain that change. So while `git rebase
> some_branch_name` I started getting:
>
> First, rewinding head to replay your work on top of it...
> Applying: My cool patch.
> fatal: sha1 information is lacking or useless
> (app/controllers/settings_controller.rb).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 My cool patch.
>
> When you have resolved this problem run "git rebase --continue".
> If you would prefer to skip this patch, instead run "git rebase --skip".
> To restore the original branch and stop rebasing run "git rebase --abort".

Ha!

It would appear that `git rebase' is in fact producing patches with
`git format-patch' and then applying the resulting patches with `git
am', which gets confused by your inline diff; this can be clearly seen
in the `git-rebase--am[.sh]' file.

Perhaps `git rebase' should be reimplemented to use `git cherry-pick',
or does that suffer from the same problem?

Re: Git Bug - diff in commit message.

[Junio C Hamano]

anikey <arty.anikey@gmail.com> writes:

> Hi, peops. I'm pretty much sure that's a bug.

I am not Peops, but will try to help ;-).

As "rebase" is essentially "format-patch" output piped into "am",
unindented diff in a random place inside the commit message will be
indistingushable from the beginning of the patch. You cannot have an
unadulterated "diff" output in your commit message for this reason, and
this is not likely to change.

Often what people do is to indent them, as the reason they quote patch in
the message is to make it serve as a supporting material. Incidentally,
that makes the resulting commit log message easier to read as well; e.g.

        Fix bloop bug

        An earlier commit had this patch that is quite bogus.

            diff --git a/bloop.php b/bloop.php
            index 937da74..0e8440d 100644
            --- a/bloop.php
            +++ b/bloop.php
            @@ -42,7 +42,7 @@
            ...

        The assignment to frotz in the above hunk should happen after
        nitfol variable is initialized.

        This patch fixes it.

        Signed-off-by: Cont Ributer <contributor@example.com>

Re: Git Bug - diff in commit message.

[Thomas Rast]

Michael Witten wrote:
> On Thu, Sep 8, 2011 at 14:49, anikey <arty.anikey@gmail.com> wrote:
> > Hi, peops. I'm pretty much sure that's a bug.
> >
> > What I did was putting git diff (i needed to tell people that for my changes
> > to start working they needed to aplly message-inline patch to some code
> > which was not under git) in commit message. Like adding:
> 
> It would appear that `git rebase' is in fact producing patches with
> `git format-patch' and then applying the resulting patches with `git
> am', which gets confused by your inline diff; this can be clearly seen
> in the `git-rebase--am[.sh]' file.
> 
> Perhaps `git rebase' should be reimplemented to use `git cherry-pick',
> or does that suffer from the same problem?

It doesn't, since it uses a three-way merge.  But there's no real
reason to reimplement anything; just use an interactive rebase, it
uses cherry-pick in a loop.


-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: Git Bug - diff in commit message.

[Johannes Sixt]

Am 9/9/2011 2:56, schrieb Thomas Rast:
> Michael Witten wrote:
>> Perhaps `git rebase' should be reimplemented to use `git cherry-pick',
>> or does that suffer from the same problem?
> 
> It doesn't, since it uses a three-way merge.  But there's no real
> reason to reimplement anything; just use an interactive rebase, it
> uses cherry-pick in a loop.

But you can just as well use 'git rebase -m', which uses cherry-pick (or
something equivalent, dunno).

-- Hannes

Re: Git Bug - diff in commit message.

[Junio C Hamano]

Michael Witten <mfwitten@gmail.com> writes:

> It would appear that `git rebase' is in fact producing patches with
> `git format-patch' and then applying the resulting patches with `git
> am', which gets confused by your inline diff; this can be clearly seen
> in the `git-rebase--am[.sh]' file.
>
> Perhaps `git rebase' should be reimplemented to use `git cherry-pick',
> or does that suffer from the same problem?

I think it just is a simple matter of this one-liner.  We were already
bending backwards to preserve the original message by not parsing and
running stripspace the message in the output from mailinfo, and instead
using the log message from the original, but were still using the patch
text that came from mailinfo that was split incorrectly because it was
fooled by the diff in the commit log message.

In the longer term, I think "git-rebase--am.sh" should be rewritten to
have format-patch avoid the cost of actually generating the patch text,
and the "mailinfo" call that comes above the context shown in this patch
should be made conditional---when using "am" for rebasing we do not really
care anything but the commit object names, and everything else is figured
out from the commit this codepath.

 git-am.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 016b505..9a4cb2d 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -632,6 +632,7 @@ To restore the original branch and stop patching run \"\$cmdline --abort\"."
 			sed -e '1,/^$/d' >"$dotest/msg-clean"
 			echo "$commit" > "$dotest/original-commit"
 			get_author_ident_from_commit "$commit" > "$dotest/author-script"
+			git diff-tree --root --binary -m --first-parent "$commit" >"$dotest/patch"
 		else
 			{
 				sed -n '/^Subject/ s/Subject: //p' "$dotest/info"

Re: Git Bug - diff in commit message.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In the longer term, I think "git-rebase--am.sh" should be rewritten to
> have format-patch avoid the cost of actually generating the patch text,
> and the "mailinfo" call that comes above the context shown in this patch
> should be made conditional---when using "am" for rebasing we do not really
> care anything but the commit object names, and everything else is figured
> out from the commit this codepath.

And here is a quick hack to do that using "log --cherry-pick --right-only".

 git-am.sh         |   44 ++++++++++++++++++++++++--------------------
 git-rebase--am.sh |   12 +++++++++---
 2 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 9042432..b79ccc5 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -641,32 +641,36 @@ do
 	# by the user, or the user can tell us to do so by --resolved flag.
 	case "$resume" in
 	'')
-		git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
-			<"$dotest/$msgnum" >"$dotest/info" ||
-			stop_here $this
-
-		# skip pine's internal folder data
-		sane_grep '^Author: Mail System Internal Data$' \
-			<"$dotest"/info >/dev/null &&
-			go_next && continue
-
-		test -s "$dotest/patch" || {
-			eval_gettextln "Patch is empty.  Was it split wrong?
-If you would prefer to skip this patch, instead run \"\$cmdline --skip\".
-To restore the original branch and stop patching run \"\$cmdline --abort\"."
-			stop_here $this
-		}
 		rm -f "$dotest/original-commit" "$dotest/author-script"
-		if test -f "$dotest/rebasing" &&
+
+		if test -f "$dotest/rebasing"
+		then
 			commit=$(sed -e 's/^From \([0-9a-f]*\) .*/\1/' \
 				-e q "$dotest/$msgnum") &&
-			test "$(git cat-file -t "$commit")" = commit
-		then
+			test "$(git cat-file -t "$commit")" = commit || {
+				stop_here $this
+			}
 			git cat-file commit "$commit" |
 			sed -e '1,/^$/d' >"$dotest/msg-clean"
-			echo "$commit" > "$dotest/original-commit"
-			get_author_ident_from_commit "$commit" > "$dotest/author-script"
+			echo "$commit" >"$dotest/original-commit"
+			get_author_ident_from_commit "$commit" >"$dotest/author-script"
+			git diff-tree -p --root "$commit" >"$dotest/patch"
 		else
+			git mailinfo $keep $no_inbody_headers \
+			$scissors $utf8 "$dotest/msg" "$dotest/patch" \
+			<"$dotest/$msgnum" >"$dotest/info" ||
+			stop_here $this
+
+			# skip pine's internal folder data
+			sane_grep '^Author: Mail System Internal Data$' \
+				<"$dotest"/info >/dev/null &&
+				go_next && continue
+			test -s "$dotest/patch" || {
+				eval_gettextln "Patch is empty.  Was it split wrong?
+If you would prefer to skip this patch, instead run \"\$cmdline --skip\".
+To restore the original branch and stop patching run \"\$cmdline --abort\"."
+				stop_here $this
+			}
 			{
 				sed -n '/^Subject/ s/Subject: //p' "$dotest/info"
 				echo
diff --git a/git-rebase--am.sh b/git-rebase--am.sh

[...]

Re: Git Bug - diff in commit message.

[Martin von Zweigbergk]

Hi,

On Tue, Oct 11, 2011 at 4:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> In the longer term, I think "git-rebase--am.sh" should be rewritten to
>> have format-patch avoid the cost of actually generating the patch text,

I'm (slowly) working on this.

> And here is a quick hack to do that using "log --cherry-pick --right-only".

Thanks. I had done something similar. I have now adopted most of your
changes into my patches, but  I have a few questions.

> +               if test -f "$dotest/rebasing"
> +               then
>                        commit=$(sed -e 's/^From \([0-9a-f]*\) .*/\1/' \
>                                -e q "$dotest/$msgnum") &&
> +                       test "$(git cat-file -t "$commit")" = commit || {
> +                               stop_here $this
> +                       }

Are these braces needed?

> +                       git diff-tree -p --root "$commit" >"$dotest/patch"

In your previous mail in this thread, this was

+                       git diff-tree --root --binary -m
--first-parent "$commit" >"$dotest/patch"

I see why you dropped "-m --first-parent"; we should simply never
receive such patches to "git-am --rebasing". But why isn't --binary
necessary?

> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
>
> [...]

Why was this part left out? Did you get it to work using 'git log
--cherry-pick --right-only'? I'm asking because I did not get it to
work with except for the case where $onto=upstream (basically when
--onto is not given).

Martin

Re: Git Bug - diff in commit message.

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

>> And here is a quick hack to do that using "log --cherry-pick --right-only".
>
> Thanks. I had done something similar. I have now adopted most of your
> changes into my patches, but  I have a few questions.

It was a quick hack and not necessarily well thought out.

> Are these braces needed?

Probably not.

> In your previous mail in this thread, this was
> ...
> I see why you dropped "-m --first-parent"; we should simply never
> receive such patches to "git-am --rebasing". But why isn't --binary
> necessary?

It may be; I somehow thought we made it a no-op but didn't check.

> Why was this part left out?

No particular reason other than I was not looking at the earlier "how
about this" patch closely. It may be needed, it may not be, I dunno, and I
didn't check when I wrote this second quick hack.

Thanks.