bogus merges

bogus merges

[Wayne Scott]

A recent commit in linux-2.6 looks like this:

commit b129a8ccd53f74c43e4c83c8e0031a4990040830
Merge: 6b39374a27eb4be7e9d82145ae270ba02ea90dc8
194d0710e1a7fe92dcf860ddd31fded8c3103b7a
Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
Date:   Wed Aug 31 10:12:14 2005 +0100

    [SERIAL] Clean up and fix tty transmission start/stoping
    
The problem is that two parents of this merge are separate.  One is an
ancestor of the other.  This means the merge is really pointless and
probably accidental.

Really 'git commit' should detect problems like this automatically and
prevent them from getting in the tree.

-Wayne

Re: bogus merges

[Linus Torvalds]

On Mon, 5 Sep 2005, Wayne Scott wrote:
>
> A recent commit in linux-2.6 looks like this:

It hopefully shouldn't happen any more with the improved and fixed
git-merge-base.

> Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
> Date:   Wed Aug 31 10:12:14 2005 +0100

I suspect rmk is using cogito-0.13, and that it still has the older 
git-merge-base that can get confused by the date-ordering problem. And 
when git-merge-base gives the wrong result (not either of the commit 
objects it was given as an argument), then "git resolve" will do the wrong 
thing.

I just checked, and the _current_ git-merge-base definitely gives the 
right result:

	linux$ git-merge-base -a \
		6b39374a27eb4be7e9d82145ae270ba02ea90dc8 \
		194d0710e1a7fe92dcf860ddd31fded8c3103b7a

results in

	194d0710e1a7fe92dcf860ddd31fded8c3103b7a

ie it correctly notices that the second commit is the parent of the first
one.

> Really 'git commit' should detect problems like this automatically and
> prevent them from getting in the tree.

Well, that would depend on having the fixed git-merge-base in the first 
place, which in turn would mean that such a commit wouldn't happen at all, 
so it's kind of circular. It's not worth fixing anywhere else, since once 
you fix it in git-merge-base, it just becomes a non-issue.

Hopefully we'll have a new cogito release soon, and this particular bug
will be a thing of the past.

		Linus

Re: bogus merges

[Wayne Scott]

On 9/5/05, Linus Torvalds <torvalds@osdl.org> wrote:
> > Really 'git commit' should detect problems like this automatically and
> > prevent them from getting in the tree.
> 
> Well, that would depend on having the fixed git-merge-base in the first
> place, which in turn would mean that such a commit wouldn't happen at all,
> so it's kind of circular. It's not worth fixing anywhere else, since once
> you fix it in git-merge-base, it just becomes a non-issue.

This just error checking to prevent future bugs from getting committed
to the tree.  These kinds of things are very hard to repair after the
fact.

-Wayne

Re: bogus merges

[Russell King]

On Tue, Sep 06, 2005 at 08:08:57AM -0500, Wayne Scott wrote:
> On 9/5/05, Linus Torvalds <torvalds@osdl.org> wrote:
> > > Really 'git commit' should detect problems like this automatically and
> > > prevent them from getting in the tree.
> > 
> > Well, that would depend on having the fixed git-merge-base in the first
> > place, which in turn would mean that such a commit wouldn't happen at all,
> > so it's kind of circular. It's not worth fixing anywhere else, since once
> > you fix it in git-merge-base, it just becomes a non-issue.
> 
> This just error checking to prevent future bugs from getting committed
> to the tree.  These kinds of things are very hard to repair after the
> fact.

They don't get repaired.

The real problem is that there needs to be a way for cogito to remain
more up to date with git, so that when problems are fixed in git they
can propagate into cogito faster.

I'm not sure separating cogito from git is a good solution though -
I've no real view on how stable the interface between those two bits
are, but I'd hate for a git upgrade to break cogito in some subtle
way.

-- 
Russell King

Re: bogus merges

[Daniel Barkalow]

On Mon, 5 Sep 2005, Linus Torvalds wrote:

> On Mon, 5 Sep 2005, Wayne Scott wrote:
> >
> > A recent commit in linux-2.6 looks like this:
> 
> It hopefully shouldn't happen any more with the improved and fixed
> git-merge-base.

Couldn't it also happen if there's stale data in MERGE_HEAD when you 
commit a normal patch? The description doesn't look like a merge at all, 
but rather like a normal patch that inappropriately picked up an extra 
head. I'd guess he tried to merge something, got a conflict, decided that 
he didn't really want to do that anyway, switched to a different branch, 
applied a patch, and committed without noticing the note that he seemed to 
be committing a merge.

Probably the right thing is actually to clean up more when switching 
tasks, but it would probably also be worth checking that merges make sense 
as well.

	-Daniel
*This .sig left intentionally blank*

Re: bogus merges

[Russell King]

On Tue, Sep 06, 2005 at 02:28:39PM -0400, Daniel Barkalow wrote:
> On Mon, 5 Sep 2005, Linus Torvalds wrote:
> > On Mon, 5 Sep 2005, Wayne Scott wrote:
> > >
> > > A recent commit in linux-2.6 looks like this:
> > 
> > It hopefully shouldn't happen any more with the improved and fixed
> > git-merge-base.
> 
> Couldn't it also happen if there's stale data in MERGE_HEAD when you 
> commit a normal patch?

I don't think Cogito has the idea of a MERGE_HEAD file.

> The description doesn't look like a merge at all, 
> but rather like a normal patch that inappropriately picked up an extra 
> head. I'd guess he tried to merge something, got a conflict, decided that 
> he didn't really want to do that anyway, switched to a different branch, 
> applied a patch, and committed without noticing the note that he seemed to 
> be committing a merge.

"HE'd" hadn't (please don't use the third person when you're talking about
someone who is being copied on the thread, thanks.)  Look closer at the
two heads (I've cut out the author info):

commit b129a8ccd53f74c43e4c83c8e0031a4990040830
tree 4c40afd836be87166d6d014380262f1baa19694f
parent 6b39374a27eb4be7e9d82145ae270ba02ea90dc8
parent 194d0710e1a7fe92dcf860ddd31fded8c3103b7a
committer Russell King <rmk+kernel@arm.linux.org.uk> Wed, 31 Aug 2005 10:12:14 +0100
    [SERIAL] Clean up and fix tty transmission start/stoping

which was apparantly (but not really) a merge between:

commit 6b39374a27eb4be7e9d82145ae270ba02ea90dc8
tree 09933113cf28f253db1dd539463bdab741d67139
parent 62c592edead3c3a045662595f7ade3c12f133373
parent ed735ccbefaf7e5e3ef61418f7e209b8c59308a7
committer Linus Torvalds <torvalds@g5.osdl.org> Tue, 30 Aug 2005 11:16:30 -0700
    Merge refs/heads/upstream from master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git

commit 194d0710e1a7fe92dcf860ddd31fded8c3103b7a
tree da03b56fa4dee221c53af5770492d391f0d09459
parent a68d2ebc1581a3aec57bd032651e013fa609f530
parent 9bbd03758945858c9303f3258b418b94c4ffd735
committer Linus Torvalds <torvalds@g5.osdl.org> Wed, 03 Aug 2005 13:09:43 -0700
    Merge master.kernel.org:/home/rmk/linux-2.6-arm

However, the serial tree (which commit b129a8ccd53f74c43e4c83c8e0031a4990040830
was created in) was last pulled in commit 975f957dc408925805dd8f5aa4217b7eeea2d005.
Following the commit trail, you'll end up at commit
6b39374a27eb4be7e9d82145ae270ba02ea90dc8 above.

commit 975f957dc408925805dd8f5aa4217b7eeea2d005
tree 2198bb72323a016d93c7440c9240bac94a5c0016
parent 2321fbd2b87539edc1fbfc2e186528a1ef93835f
parent 661299d9d0437a0ff72240f3d60016ac3a361a6e
committer Linus Torvalds <torvalds@g5.osdl.org> Mon, 29 Aug 2005 10:34:59 -0700
    Merge HEAD from master.kernel.org:/home/rmk/linux-2.6-serial.git

which was the result of the following merge between the head of the
serial tree and Linus' tree:

commit 661299d9d0437a0ff72240f3d60016ac3a361a6e
tree 765512576314fc3612b503f182b9ae4e60fcf849
parent 05caac585f8abd6c0113856bc8858e3ef214d8a6
parent 41c018b7ecb60b1c2c4d5dee0cd37d32a94c45af
committer Russell King <rmk+kernel@arm.linux.org.uk> Thu, 28 Jul 2005 09:30:20 +0100
    Merge with Linus' 2.6 tree

This was at the head of the serial tree at the time:

commit 05caac585f8abd6c0113856bc8858e3ef214d8a6
tree ac9f8f2cc032281af09200da514257d120510906
parent 241fc4367b3ca5d407b043599ed980304a70b91f
committer Russell King <rmk+kernel@arm.linux.org.uk> Wed, 27 Jul 2005 11:41:18 +0100
    [SERIAL] Convert parport_serial to use new 8250_pci interfaces

So, the question is, if the serial git tree was all happy in the
merge on 29th August (975f957dc408925805dd8f5aa4217b7eeea2d005)
why did the next commit to that tree (being
b129a8ccd53f74c43e4c83c8e0031a4990040830) go wrong when the
previous merge would have been a standard fast-forward operation.

There is _no_ reason why the serial tree would have been wound back
to 3rd August.

> Probably the right thing is actually to clean up more when switching 
> tasks, but it would probably also be worth checking that merges make sense 
> as well.

You might be right except for one small detail.  I don't "switch tasks"
with a single git tree.  I have a set of individual git trees, one per
"task".  They always remain in a clean state due to the way I work:

	- update tree to Linus if Linus tree is a superset of the tree
	- apply changes in patch form to tree and commit each in turn
	- send linus a request to merge

However, occasionally, I update the tree when Linus' tree is not a
superset, as was the case in 661299d9d0437a0ff72240f3d60016ac3a361a6e
above.

-- 
Russell King

Re: bogus merges

[Daniel Barkalow]

On Sun, 11 Sep 2005, Russell King wrote:

> Look closer at the two heads (I've cut out the author info):
> 
> commit b129a8ccd53f74c43e4c83c8e0031a4990040830
> tree 4c40afd836be87166d6d014380262f1baa19694f
> parent 6b39374a27eb4be7e9d82145ae270ba02ea90dc8
> parent 194d0710e1a7fe92dcf860ddd31fded8c3103b7a
> committer Russell King <rmk+kernel@arm.linux.org.uk> Wed, 31 Aug 2005 10:12:14 +0100
>     [SERIAL] Clean up and fix tty transmission start/stoping
> 
> which was apparantly (but not really) a merge between:
> 
> commit 6b39374a27eb4be7e9d82145ae270ba02ea90dc8
> tree 09933113cf28f253db1dd539463bdab741d67139
> parent 62c592edead3c3a045662595f7ade3c12f133373
> parent ed735ccbefaf7e5e3ef61418f7e209b8c59308a7
> committer Linus Torvalds <torvalds@g5.osdl.org> Tue, 30 Aug 2005 11:16:30 -0700
>     Merge refs/heads/upstream from master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git
> 
> commit 194d0710e1a7fe92dcf860ddd31fded8c3103b7a
> tree da03b56fa4dee221c53af5770492d391f0d09459
> parent a68d2ebc1581a3aec57bd032651e013fa609f530
> parent 9bbd03758945858c9303f3258b418b94c4ffd735
> committer Linus Torvalds <torvalds@g5.osdl.org> Wed, 03 Aug 2005 13:09:43 -0700
>     Merge master.kernel.org:/home/rmk/linux-2.6-arm
> 
> However, the serial tree (which commit b129a8ccd53f74c43e4c83c8e0031a4990040830
> was created in) was last pulled in commit 975f957dc408925805dd8f5aa4217b7eeea2d005.
> Following the commit trail, you'll end up at commit
> 6b39374a27eb4be7e9d82145ae270ba02ea90dc8 above.
> 
> commit 975f957dc408925805dd8f5aa4217b7eeea2d005
> tree 2198bb72323a016d93c7440c9240bac94a5c0016
> parent 2321fbd2b87539edc1fbfc2e186528a1ef93835f
> parent 661299d9d0437a0ff72240f3d60016ac3a361a6e
> committer Linus Torvalds <torvalds@g5.osdl.org> Mon, 29 Aug 2005 10:34:59 -0700
>     Merge HEAD from master.kernel.org:/home/rmk/linux-2.6-serial.git
> 
> which was the result of the following merge between the head of the
> serial tree and Linus' tree:
> 
> commit 661299d9d0437a0ff72240f3d60016ac3a361a6e
> tree 765512576314fc3612b503f182b9ae4e60fcf849
> parent 05caac585f8abd6c0113856bc8858e3ef214d8a6
> parent 41c018b7ecb60b1c2c4d5dee0cd37d32a94c45af
> committer Russell King <rmk+kernel@arm.linux.org.uk> Thu, 28 Jul 2005 09:30:20 +0100
>     Merge with Linus' 2.6 tree
> 
> This was at the head of the serial tree at the time:
> 
> commit 05caac585f8abd6c0113856bc8858e3ef214d8a6
> tree ac9f8f2cc032281af09200da514257d120510906
> parent 241fc4367b3ca5d407b043599ed980304a70b91f
> committer Russell King <rmk+kernel@arm.linux.org.uk> Wed, 27 Jul 2005 11:41:18 +0100
>     [SERIAL] Convert parport_serial to use new 8250_pci interfaces
> 
> You might be right except for one small detail.  I don't "switch tasks"
> with a single git tree.

I think I was right, but "switching tasks" has to be interpretted as 
switching from a merge you don't care to complete to doing further 
development on that tree, and the "switch" is done by a fast-forward (to 
the result of someone else completing the merge).

>  I have a set of individual git trees, one per
> "task".  They always remain in a clean state due to the way I work:
> 
> 	- update tree to Linus if Linus tree is a superset of the tree
> 	- apply changes in patch form to tree and commit each in turn
> 	- send linus a request to merge
> 
> However, occasionally, I update the tree when Linus' tree is not a
> superset, as was the case in 661299d9d0437a0ff72240f3d60016ac3a361a6e
> above.

Ah, okay. So I think the chain of events is:

 28 Jul: You merge with Linus, when he hasn't pulled your tree. This goes 
         fine.
 03 Aug: You pull from Linus again, but decide that you don't actually 
         care about tracking mainline in this tree so tightly. This leaves
         194d... in .git/merging, because Cogito doesn't realize you're 
         not actually planning to complete the merge.
 29 Aug: Linus pulls from you. This goes fine.
 30 Aug: You update the tree again from Linus. This is a fast-forward and 
         goes fine, and doesn't touch .git/merging because it completes
         the process without requiring input or a commit.
 31 Aug: You make a different change and commit. This is the first time 
         you committed there since 03 Aug, and it thinks that you're still 
         merging your current head (now from 30 Aug) with 194d..., which 
         it marked down explicitly to track this when you started the 
         merge on the 3rd, and you don't get sufficient information to 
         realize that it's messed up.

I think that the fundamental bug is that tree_timewrap() doesn't clear 
.git/merging, which means that it can still think you're doing a merge 
when you've started doing something else (such as doing further 
development after updating to someone else's merge of your tree).

But I still think that it would be worth checking for the parents in merge 
commits making sense, because the fact that stale data is likely to be 
from abandoned operations on ancestors.

	-Daniel
*This .sig left intentionally blank*

Re: bogus merges

[Petr Baudis]

Dear diary, on Sun, Sep 11, 2005 at 08:08:14PM CEST, I got a letter
where Daniel Barkalow <barkalow@iabervon.org> told me that...
> I think that the fundamental bug is that tree_timewrap() doesn't clear 
> .git/merging, which means that it can still think you're doing a merge 
> when you've started doing something else (such as doing further 
> development after updating to someone else's merge of your tree).

Oh, nice catch. Thanks, fixed.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
If you want the holes in your knowledge showing up try teaching
someone.  -- Alan Cox

Re: bogus merges

[Russell King]

On Mon, Sep 05, 2005 at 09:01:32AM -0700, Linus Torvalds wrote:
> On Mon, 5 Sep 2005, Wayne Scott wrote:
> > A recent commit in linux-2.6 looks like this:
> 
> It hopefully shouldn't happen any more with the improved and fixed
> git-merge-base.
> 
> > Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
> > Date:   Wed Aug 31 10:12:14 2005 +0100
> 
> I suspect rmk is using cogito-0.13

Correct, and rmk will probably be extremely nervous about upgrading when
0.14 appears.

-- 
Russell King

Re: bogus merges

[Martin Langhoff]

On 9/11/05, Russell King <rmk@arm.linux.org.uk> wrote:
> On Mon, Sep 05, 2005 at 09:01:32AM -0700, Linus Torvalds wrote:
> > I suspect rmk is using cogito-0.13
> 
> Correct, and rmk will probably be extremely nervous about upgrading when
> 0.14 appears.

Well, *actually* cogito-0.13 didn't include git-core, so we have to
look for the reasons elsewhere. Could be the leftover MERGE_HEAD
Daniel mentions.

Russel, can you confirm what git-core version you are/were running?

cheers,.


martin

Re: bogus merges

[A Large Angry SCM]

Martin Langhoff wrote:
> On 9/11/05, Russell King <rmk@arm.linux.org.uk> wrote:
>>On Mon, Sep 05, 2005 at 09:01:32AM -0700, Linus Torvalds wrote:
>>>I suspect rmk is using cogito-0.13
>>Correct, and rmk will probably be extremely nervous about upgrading when
>>0.14 appears.
> 
> Well, *actually* cogito-0.13 didn't include git-core, so we have to
> look for the reasons elsewhere. Could be the leftover MERGE_HEAD
> Daniel mentions.
> 
> Russel, can you confirm what git-core version you are/were running?

Russel,

How are you updating your tree to Linus'? If you are rsyncing from 
kernel.org, you're probably getting a MERGE_HEAD with the rsync. A while 
ago I got annoyed enough add the equivalent of:

	rm -f ${LOCAL_REPOS}/.git/MERGE_HEAD

to my (very stupid) git rsync script.

Re: bogus merges

[Russell King]

On Sun, Sep 11, 2005 at 11:11:49AM -0400, A Large Angry SCM wrote:
> Martin Langhoff wrote:
> > On 9/11/05, Russell King <rmk@arm.linux.org.uk> wrote:
> >>On Mon, Sep 05, 2005 at 09:01:32AM -0700, Linus Torvalds wrote:
> >>>I suspect rmk is using cogito-0.13
> >>Correct, and rmk will probably be extremely nervous about upgrading when
> >>0.14 appears.
> > 
> > Well, *actually* cogito-0.13 didn't include git-core, so we have to
> > look for the reasons elsewhere. Could be the leftover MERGE_HEAD
> > Daniel mentions.
> > 
> > Russel, can you confirm what git-core version you are/were running?
> 
> Russel,
> 
> How are you updating your tree to Linus'? If you are rsyncing from 
> kernel.org, you're probably getting a MERGE_HEAD with the rsync. A while 
> ago I got annoyed enough add the equivalent of:
> 
> 	rm -f ${LOCAL_REPOS}/.git/MERGE_HEAD
> 
> to my (very stupid) git rsync script.

I think you can forget MERGE_HEAD.  Why?  I use cogito for pulling.

cg-pull gets the head, and then rsyncs the objects found in the object
directory.  It doesn't touch MERGE_HEAD.  None of the cogito scripts
that I have know anything about MERGE_HEAD itself.

(Plus, it's actually a two stage pull - I have a cron-based cg-pull
into a master repository on one box, which I then cg-pull to the
development box which is only powered when I'm working.)

-- 
Russell King

Re: bogus merges

[Russell King]

On Sun, Sep 11, 2005 at 11:48:10PM +1200, Martin Langhoff wrote:
> On 9/11/05, Russell King <rmk@arm.linux.org.uk> wrote:
> > On Mon, Sep 05, 2005 at 09:01:32AM -0700, Linus Torvalds wrote:
> > > I suspect rmk is using cogito-0.13
> > 
> > Correct, and rmk will probably be extremely nervous about upgrading when
> > 0.14 appears.
> 
> Well, *actually* cogito-0.13 didn't include git-core, so we have to
> look for the reasons elsewhere. Could be the leftover MERGE_HEAD
> Daniel mentions.
> 
> Russel, can you confirm what git-core version you are/were running?

Nope, because now that I look I'm actually using cogito 0.12.1
not 0.13 as I previously mentioned.  Oops.

-- 
Russell King