git rebase: unexpected conflict

git rebase: unexpected conflict

[Michael S. Tsirkin]

git rebase seems to see conflicts where there shouldn't be any.

Example: I have a tree here:

git://git/openfabrics.org/~vlad/ofed_1_2/.git

and have efedfe57a21a134a65d951bcca73af46da609c5e
checked out (I think it moved since, but you can create
a branch if you like).

This is tree from Linus + some extra drivers added
(it's large (as kernel trees often are), so use
 Linus' tree as a reference if you want to clone it).

Now, I decided I don't like 9b3bfe5696aa417d38ce903eb345a03d65743dd2,
and since no one is likely to have a clone of this tree,
it should be OK to remove it from history with git rebase.

So I did:

git-rebase --onto 9b3bfe5696aa417d38ce903eb345a03d65743dd2~1 \
		9b3bfe5696aa417d38ce903eb345a03d65743dd2

and it starts replaying the changes I pulled from Linus' tree:

--------------------------
First, rewinding head to replay your work on top of it...
HEAD is now at 6cc583d... IB/sdp: allow users via module parameter to bound SDP's memory use

Applying '[JFFS2] kill warning RE debug-only variables'

Wrote tree 3a03db80ed42fda246e5e8420699e22cdd6fff89
Committed: c0427b1c39aeb86ea7b97a25ed7b623ac563b516

Applying '[MTD NAND] Initial import of CAFÉ NAND driver.'

Adds trailing whitespace.
.dotest/patch:44:/*
Adds trailing whitespace.
.dotest/patch:93:
Adds trailing whitespace.
.dotest/patch:225:              /* Ignore the first command of a pair; the hardware
Adds trailing whitespace.
.dotest/patch:242:      dev_dbg(&cafe->pdev->dev, "dlen %x, ctl1 %x, ctl2 %x\n",
Adds trailing whitespace.
.dotest/patch:507:
warning: squelched 2 whitespace errors
warning: 7 lines add trailing whitespaces.
Wrote tree 18cdff711481cdb4c495a846a7776203198dfc87
Committed: 4cfaf4dd2573cd695603f31c404f3b66e1515487

Applying '[MTD] SSFDC must depend on BLOCK'

Wrote tree c91cb50928089c0968d40a14432085db58b12614
Committed: 60a9aa8868bab0821b8cdb05f711e7a812584800

Applying '[MTD NAND] OLPC CAFÉ driver update'

Adds trailing whitespace.
.dotest/patch:119:      cafe_dev_dbg(&cafe->pdev->dev, "dlen %x, ctl1 %x, ctl2 %x\n",
Adds trailing whitespace.
.dotest/patch:198:/* Ick. The BBT code really ought to be able to work this bit out
warning: 2 lines add trailing whitespaces.
Wrote tree 3c7c72650eea2c613569ca51bc47775beca984c9
Committed: fc6406731b3fe302d630317593483fbde9abe8d4

Applying '[MTD] MAPS: Add parameter to amd76xrom to override rom window size'

Wrote tree ae4567223d6b8ed5408ca0b63611b0f7962589aa
Committed: c4f773a7476520b5459ddbb83df5fe143eec4626

Applying '[MTD] CHIPS: Support for SST 49LF040B flash chip'

Wrote tree c5038d0ec22dbc4ba571c9a82697a43f8dd2bd9b
Committed: f19c8f26bc9849531b28d48430c09e0359976a3e

Applying '[MTD] MAPS: Support for BIOS flash chips on Intel ESB2 southbridge'

Wrote tree 169159ae4017bc3838a01ad737fae53efa150a09
Committed: c6ec7ab7ef424713c6bf62fa436b95912c18db3a

Applying '[JFFS2] Use rb_first() and rb_last() cleanup'

Wrote tree 53862b25b4bbd4c7e57cc893025545ce235eb1b8
Committed: 0f45697d933b9db1edae3cac0a59284cdae5656f

Applying '[MTD] JEDEC probe: fix comment typo (devic)'

Wrote tree 82173259dd1c133e7f9c5879fd096ba658f69cb4
Committed: 2119a3d1d4af4bfbcb72494b994ce52d0d6ad08b

Applying '[MTD] MAPS: esb2rom: use hotplug safe interfaces'

Wrote tree cab550abb9f4d577243bb6492706922c58cd9a44
Committed: 4d7f42ad97194199da7ad4ec0ae80ce5399b7554

Applying '[JFFS2] Fix jffs2_follow_link() typo'

Wrote tree fe9d6afb990115817aae90716640e5588c79c7c6
Committed: 29cd2f5afa2aa8a8ea7842437f6cdd534605a70e

Applying '[MTD] NAND: AT91 NAND driver'

error: patch failed: drivers/mtd/nand/Kconfig:232
error: drivers/mtd/nand/Kconfig: patch does not apply
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
 100% (22053/22053) done
Auto-merged drivers/mtd/nand/Kconfig
CONFLICT (content): Merge conflict in drivers/mtd/nand/Kconfig
Auto-merged drivers/mtd/nand/Makefile
Failed to merge in the changes.
Patch failed at 0012.

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

Sure enough, there are conflicts in drivers/mtd/nand/Kconfig:

<<<<<<< HEAD:drivers/mtd/nand/Kconfig
config MTD_NAND_CAFE
       tristate "NAND support for OLPC CAFÉ chip"
       depends on PCI
       help
         Use NAND flash attached to the CAFÉ chip designed for the $100
         laptop.
=======
config MTD_NAND_AT91
        bool "Support for NAND Flash / SmartMedia on AT91"
        depends on MTD_NAND && ARCH_AT91
        help
          Enables support for NAND Flash / Smart Media Card interface
          on Atmel AT91 processors.
>>>>>>> [MTD] NAND: AT91 NAND driver:drivers/mtd/nand/Kconfig

But I don't really understand where do the conflicts come from:
The patch I skipped (9b3bfe5696aa417d38ce903eb345a03d65743dd2)
only touched the file drivers/infiniband/core/addr.c,
so why can't rebase replay all of the changes?

-- 
MST

Re: git rebase: unexpected conflict

[Jakub Narebski]

Michael S. Tsirkin wrote:

> git rebase seems to see conflicts where there shouldn't be any.

Were you on a branch that you wanted to rebase? I had made this error once.
git rebase --onto <newbase> <upstream> <branch> moves <upstream>..<branch>
into <newbase>.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: git rebase: unexpected conflict

[Michael S. Tsirkin]

> Quoting Jakub Narebski <jnareb@gmail.com>:
> Subject: Re: git rebase: unexpected conflict
> 
> Michael S. Tsirkin wrote:
> 
> > git rebase seems to see conflicts where there shouldn't be any.
> 
> Were you on a branch that you wanted to rebase?

Yes: I did not specify the <branch> parameter at all.
However note that there were merges since the commit
that I wanted to skip.

> I had made this error once.
> git rebase --onto <newbase> <upstream> <branch> moves <upstream>..<branch>
> into <newbase>.

I really just did
git-rebase --onto $1~1 $1

and the commit I gave 9b3bfe5696aa417d38ce903eb345a03d65743dd2
actually as a parent of the current branch.


-- 
MST

Re: git rebase: unexpected conflict

[Junio C Hamano]

"Michael S. Tsirkin" <mst@mellanox.co.il> writes:

> 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".
> --------------------------
>
> Sure enough, there are conflicts in drivers/mtd/nand/Kconfig:
>
> <<<<<<< HEAD:drivers/mtd/nand/Kconfig
> config MTD_NAND_CAFE
>        tristate "NAND support for OLPC CAFÉ chip"
>        depends on PCI
>        help
>          Use NAND flash attached to the CAFÉ chip designed for the $100
>          laptop.
> =======
> config MTD_NAND_AT91
>         bool "Support for NAND Flash / SmartMedia on AT91"
>         depends on MTD_NAND && ARCH_AT91
>         help
>           Enables support for NAND Flash / Smart Media Card interface
>           on Atmel AT91 processors.
>>>>>>>> [MTD] NAND: AT91 NAND driver:drivers/mtd/nand/Kconfig
>
> But I don't really understand where do the conflicts come from:
> The patch I skipped (9b3bfe5696aa417d38ce903eb345a03d65743dd2)
> only touched the file drivers/infiniband/core/addr.c,
> so why can't rebase replay all of the changes?

That is because David Woodhouse has an evil merge at
c45aa055c32b488fc3fd73c760df372b09acf69a.

Running "git show" on that commit would tell you the story.

Re: git rebase: unexpected conflict

[Michael S. Tsirkin]

> Quoting Junio C Hamano <junkio@cox.net>:
> Subject: Re: git rebase: unexpected conflict
> 
> "Michael S. Tsirkin" <mst@mellanox.co.il> writes:
> 
> > 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".
> > --------------------------
> >
> > Sure enough, there are conflicts in drivers/mtd/nand/Kconfig:
> >
> > <<<<<<< HEAD:drivers/mtd/nand/Kconfig
> > config MTD_NAND_CAFE
> >        tristate "NAND support for OLPC CAFÉ chip"
> >        depends on PCI
> >        help
> >          Use NAND flash attached to the CAFÉ chip designed for the $100
> >          laptop.
> > =======
> > config MTD_NAND_AT91
> >         bool "Support for NAND Flash / SmartMedia on AT91"
> >         depends on MTD_NAND && ARCH_AT91
> >         help
> >           Enables support for NAND Flash / Smart Media Card interface
> >           on Atmel AT91 processors.
> >>>>>>>> [MTD] NAND: AT91 NAND driver:drivers/mtd/nand/Kconfig
> >
> > But I don't really understand where do the conflicts come from:
> > The patch I skipped (9b3bfe5696aa417d38ce903eb345a03d65743dd2)
> > only touched the file drivers/infiniband/core/addr.c,
> > so why can't rebase replay all of the changes?
> 
> That is because David Woodhouse has an evil merge at
> c45aa055c32b488fc3fd73c760df372b09acf69a.
> 
> Running "git show" on that commit would tell you the story.

Sorry about being dense. What's evil there?
There seems to have been a merge, and a conflict was resolved.

mst@mst-lt:~/scm/linux-2.6$ git show c45aa055c32b488fc3fd73c760df372b09acf69a
commit c45aa055c32b488fc3fd73c760df372b09acf69a
Merge: 784f4d5... 8dd851d...
Author: David Woodhouse <dwmw2@infradead.org>
Date:   Sun Oct 22 02:17:05 2006 +0100

    Merge git://git.infradead.org/~dwmw2/cafe-2.6

    Conflicts:

        drivers/mtd/nand/Kconfig

diff --cc drivers/mtd/nand/Kconfig
index b4b1656,5e97e63..564f79d
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@@ -219,6 -219,6 +219,13 @@@ config MTD_NAND_SHARPS
        tristate "Support for NAND Flash on Sharp SL Series (C7xx + others)"
        depends on MTD_NAND && ARCH_PXA

++config MTD_NAND_CAFE
++       tristate "NAND support for OLPC CAFÉ chip"
++       depends on PCI
++       help
++       Use NAND flash attached to the CAFÉ chip designed for the $100
++       laptop.
++
  config MTD_NAND_CS553X
        tristate "NAND support for CS5535/CS5536 (AMD Geode companion chip)"
        depends on MTD_NAND && X86_32 && (X86_PC || X86_GENERICARCH)

-- 
MST

Re: git rebase: unexpected conflict

[Junio C Hamano]

"Michael S. Tsirkin" <mst@mellanox.co.il> writes:

> Sorry about being dense. What's evil there?

Sorry for being too terse to be useful.

An evil merge is a merge that introduces changes that do not
appear in any parent.

> There seems to have been a merge, and a conflict was resolved.
>
> mst@mst-lt:~/scm/linux-2.6$ git show c45aa055c32b488fc3fd73c760df372b09acf69a
> commit c45aa055c32b488fc3fd73c760df372b09acf69a
> Merge: 784f4d5... 8dd851d...
> Author: David Woodhouse <dwmw2@infradead.org>
> Date:   Sun Oct 22 02:17:05 2006 +0100
>
>     Merge git://git.infradead.org/~dwmw2/cafe-2.6
>
>     Conflicts:
>
>         drivers/mtd/nand/Kconfig
>
> diff --cc drivers/mtd/nand/Kconfig
> index b4b1656,5e97e63..564f79d
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@@ -219,6 -219,6 +219,13 @@@ config MTD_NAND_SHARPS
>         tristate "Support for NAND Flash on Sharp SL Series (C7xx + others)"
>         depends on MTD_NAND && ARCH_PXA
>
> ++config MTD_NAND_CAFE
> ++       tristate "NAND support for OLPC CAFÉ chip"
> ++       depends on PCI
> ++       help
> ++       Use NAND flash attached to the CAFÉ chip designed for the $100
> ++       laptop.
> ++
>   config MTD_NAND_CS553X
>         tristate "NAND support for CS5535/CS5536 (AMD Geode companion chip)"
>         depends on MTD_NAND && X86_32 && (X86_PC || X86_GENERICARCH)
>

Notice two plus letters?  The copy of the file in either
parents of the merge did not have these 7 lines.  These were
literally _added_ by the merge.

Since git-rebase currently ignores merge commits, while you are
rebasing your branch, your branch does not have these lines
after finishing to rebase the commit just before one.  The
commit that comes on top of this one _expects_ to have NAND_CAFE
already added in the tree, but since you do not have one, you
get a conflict.

Re: git rebase: unexpected conflict

[Linus Torvalds]

On Wed, 7 Feb 2007, Junio C Hamano wrote:
> 
> That is because David Woodhouse has an evil merge at
> c45aa055c32b488fc3fd73c760df372b09acf69a.
> 
> Running "git show" on that commit would tell you the story.

It's not really "evil". It's just that our "--cc" strategy makes it look 
very odd. But if you then look at why it looks that odd, you can do:

	git diff 784f4d5..8dd851d drivers/mtd/nand/Kconfig

(those two sha1's are just the commits that got merged, as reported by 
"git show"), and you'll see what happened:

 - one branch added the MTD_NAND_CAFE entry
 - the other one had removed the MTD_NAND_AT91 entry
 - they were in the same location in the Kconfig file, so it was a real 
   data conflict
 - when David fixed it up, instead of just trivially selecting the config 
   entry that remained after the merge (MTD_NAND_CAFE), he *also* then 
   moved it to alphabetically the proper place in the file.

End result: doing a "git show" on the resulting commit does how something 
rather strange (the final result does indeed have lines that did not exist 
in *either* of the parents), but what it doesn't show is the cause of that 
strange thing, because that ended up being a non-conflict after the merge 
(ie remove the old entries from both files).

In general, the output of "--cc" for conflicting merges is really quite 
good, but it's can be a bit hard to read, and in cases like this, the 
"conflict" is shown in a way that doesn't exactly make the _original_ 
conflict obvious.

			Linus

Re: git rebase: unexpected conflict

[Linus Torvalds]

On Wed, 7 Feb 2007, Michael S. Tsirkin wrote:
> 
> Sorry about being dense. What's evil there?
> There seems to have been a merge, and a conflict was resolved.

The reason Junio marked it as "evil" is that the resolved result was also 
moved around a bit, making it really hard to see what the original 
conflict was.

If David had resolved things in place, "git show" would actually have 
shown a nicer conflict diff, and _explained_ the conflict better (most 
likely by not showing it at all - which is just a sign that it was 
resolved cleanly in one direction). As it is, the conflict diff doesn't 
tell anything about what the conflict actually was, or whether it resolved 
trivially.

[ Note! Conflict diffs do *not* necessarily show the "conflict" at all: 
  we'll happily ignore any conflict that was resolved completely in one 
  direction, and the conflict diff output is really more of a warning that 
  something bad could have happened, and somebody tried to *hide* some 
  changes by calling it a merge. ]

So what makes that an "evil merge" is that it triggers the conflict diff, 
and that one really looks like somebody tried to add code and hide it. 
There was no actual evil activity involved, it was just done in a way that 
raise the red flags of the git --cc format.

So for a "non-evil" merge, the --cc output (which is what "git show" shows 
by default) will either be empty to indicate a trivial merge, or will give 
a hint about how the problem was resolved. Because of the movement, in 
this case it did neither.

		Linus

Re: git rebase: unexpected conflict

[Michael S. Tsirkin]

> Since git-rebase currently ignores merge commits

Ugh. This is the bit I did not know about.

So the following script is kind of equivalent to rebase -
it applies commits in range $1..$2 that touch files in list on top of current tree:

from=$1
shift
to=$1
shift
git-show --pretty=email `git-rev-list --no-merges $from..$to -- $* | tac` > box
../git-am box

And here I was hoping rebase will be smarter and help me out.

OK.

Questions about the above line now:

1. What is annoying in the above is that
git-show can not limit its output to just the part of patch
that affects the list of files I give, the way git-diff can.
Would such an extension be a good idea?

2. It's unfortunate that I need a temp file here.
Can git-am get stdin somehow?


-- 
MST

Re: git rebase: unexpected conflict

[Junio C Hamano]

"Michael S. Tsirkin" <mst@mellanox.co.il> writes:

>> Since git-rebase currently ignores merge commits
>
> Ugh. This is the bit I did not know about.
>
> So the following script is kind of equivalent to rebase -
> it applies commits in range $1..$2 that touch files in list on top of current tree:
>
> from=$1
> shift
> to=$1
> shift
> git-show --pretty=email `git-rev-list --no-merges $from..$to -- $* | tac` > box
> ../git-am box
>
> And here I was hoping rebase will be smarter and help me out.
>
> OK.
>
> Questions about the above line now:
>
> 1. What is annoying in the above is that
> git-show can not limit its output to just the part of patch
> that affects the list of files I give, the way git-diff can.
> Would such an extension be a good idea?

It is a good idea and I think it is implemented.

	$ git-show <commit>... --- <paths>

would probably give what you want (although I am not absolutely
sure I understand what you want).

> 2. It's unfortunate that I need a temp file here.
> Can git-am get stdin somehow?

See "git-rebase.sh" around ll.325.

Re: git rebase: unexpected conflict

[Michael S. Tsirkin]

> > git-show can not limit its output to just the part of patch
> > that affects the list of files I give, the way git-diff can.
> > Would such an extension be a good idea?
> 
> It is a good idea and I think it is implemented.
> 
> 	$ git-show <commit>... --- <paths>

Yes, this works. But this seems undocumented. Right?

-- 
MST

Re: git rebase: unexpected conflict

[Junio C Hamano]

"Michael S. Tsirkin" <mst@mellanox.co.il> writes:

>> > git-show can not limit its output to just the part of patch
>> > that affects the list of files I give, the way git-diff can.
>> > Would such an extension be a good idea?
>> 
>> It is a good idea and I think it is implemented.
>> 
>> 	$ git-show <commit>... --- <paths>
>
> Yes, this works. But this seems undocumented. Right?

Somehow I think I saw Linus's answer to this exact topic a few
hours ago.

This manual page describes only the most frequently used options.

Re: git rebase: unexpected conflict

[Michael S. Tsirkin]

> Quoting Junio C Hamano <junkio@cox.net>:
> Subject: Re: git rebase: unexpected conflict
> 
> "Michael S. Tsirkin" <mst@mellanox.co.il> writes:
> 
> >> > git-show can not limit its output to just the part of patch
> >> > that affects the list of files I give, the way git-diff can.
> >> > Would such an extension be a good idea?
> >> 
> >> It is a good idea and I think it is implemented.
> >> 
> >> 	$ git-show <commit>... --- <paths>
> >
> > Yes, this works. But this seems undocumented. Right?
> 
> Somehow I think I saw Linus's answer to this exact topic a few
> hours ago.
> 
> This manual page describes only the most frequently used options.

Yes but Linus discussed git-log, which is both complete and correct.
Unlike git-log, the man page foor git-show is correct but incomplete:
It does say 
> This manual page describes only the most frequently used options.

However where git-log man page says:
-----
The  command  takes options applicable to the git-rev-list(1) command to
control what is shown and how, and options applicable to the
git-diff-tree(1)  commands to control how the change each commit introduces
are shown.
-----

man git-show says nothing of the kind.


-------------------------------------------------------------------

Junio, is the following patch correct?

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

---

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index b802946..361eaec 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -16,7 +16,7 @@ Shows the commit logs.
 
 The command takes options applicable to the gitlink:git-rev-list[1]
 command to control what is shown and how, and options applicable to
-the gitlink:git-diff-tree[1] commands to control how the change
+the gitlink:git-diff-tree[1] commands to control how the changes
 each commit introduces are shown.
 
 This manual page describes only the most frequently used options.
diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
index 9051951..f56f164 100644
--- a/Documentation/git-show.txt
+++ b/Documentation/git-show.txt
@@ -25,6 +25,9 @@ with \--name-only).
 
 For plain blobs, it shows the plain contents.
 
+The command takes options applicable to the gitlink:git-diff-tree[1] command to
+control how the changes the commit introduces are shown.
+
 This manual page describes only the most frequently used options.
 
 
-- 
MST

Re: git rebase: unexpected conflict

[Junio C Hamano]

"Michael S. Tsirkin" <mst@mellanox.co.il> writes:

> Junio, is the following patch correct?

Looks sane to me.

Re: git rebase: unexpected conflict

[Michael S. Tsirkin]

> Quoting Junio C Hamano <junkio@cox.net>:
> Subject: Re: git rebase: unexpected conflict
> 
> "Michael S. Tsirkin" <mst@mellanox.co.il> writes:
> 
> > Junio, is the following patch correct?
> 
> Looks sane to me.

You can take it then :)

-- 
MST

Re: git rebase: unexpected conflict

[Michael S. Tsirkin]

> > 2. It's unfortunate that I need a temp file here.
> > Can git-am get stdin somehow?
> 
> See "git-rebase.sh" around ll.325.

git-am can read standard input. Document this.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

--

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index f7d551e..77ef103 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -21,6 +21,10 @@ current branch.
 
 OPTIONS
 -------
+<mbox>...::
+	The list of mailbox files to read patches from. If you do not
+	supply this argument, reads from the standard input.
+
 --signoff::
 	Add `Signed-off-by:` line to the commit message, using
 	the committer identity of yourself.

-- 
MST