* [PATCH] fix adbhid mismerge
@ 2007-10-17 0:02 Al Viro
2007-10-17 1:21 ` Linus Torvalds
0 siblings, 1 reply; 5+ messages in thread
From: Al Viro @ 2007-10-17 0:02 UTC (permalink / raw
To: Linus Torvalds; +Cc: linux-kernel
Something really odd has happened: the last couple of changesets
have
- int up_flag;
+ int keycode, up_flag;
and
- int up_flag;
+ int up_flag, key;
in another, both in adb_input_keycode(). Even with -m passed to
git-whatchanged there's no sign of anything in that area.
Aside of trivial conflict resolution (see below), what's the right
way to trace the things like that? Linus?
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/macintosh/adbhid.c b/drivers/macintosh/adbhid.c
index 8cce016..2766e4f 100644
--- a/drivers/macintosh/adbhid.c
+++ b/drivers/macintosh/adbhid.c
@@ -282,7 +282,7 @@ static void
adbhid_input_keycode(int id, int scancode, int repeat)
{
struct adbhid *ahid = adbhid[id];
- int keycode, up_flag;
+ int keycode, up_flag, key;
keycode = scancode & 0x7f;
up_flag = scancode & 0x80;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fix adbhid mismerge
2007-10-17 0:02 [PATCH] fix adbhid mismerge Al Viro
@ 2007-10-17 1:21 ` Linus Torvalds
2007-10-17 2:21 ` Linus Torvalds
0 siblings, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2007-10-17 1:21 UTC (permalink / raw
To: Al Viro; +Cc: linux-kernel
On Wed, 17 Oct 2007, Al Viro wrote:
>
> Something really odd has happened: the last couple of changesets
> have
> - int up_flag;
> + int keycode, up_flag;
> and
> - int up_flag;
> + int up_flag, key;
> in another, both in adb_input_keycode(). Even with -m passed to
> git-whatchanged there's no sign of anything in that area.
I don't think you did anything wrong. You used both --full-history
(implicitly: git-whatchanged) and you made sure to see the diffs for both
sides of any merge (-m), and that means that you should see every single
diff involved.
Looking into it, the "key" variable was declared in the commit that
introduced the new line
int up_flag, key;
..
key = adbhid[id]->keycode[keycode];
which is commit 555ddbb4e2191c8823df2d61525218ac39481385. But then that
declaration of "int key" goes away at some later time.
And doing a
git whatchanged -p -m 555ddbb4e2191c8823df2d61525218ac39481385.. drivers/macintosh/adbhid.c
does actually show the culprit. It's just that the "-p -m" format is so
damn unreadable that it's almost impossible to see.
Anyway, it's b981d8b3f5e008ff10d993be633ad00564fc22cd, which had a
conflict in that file, and Dmitry apparently mis-merged it and edited the
result down so that it didn't have 'key' declared any more.
So the way I found it was to just search for the line in the diffs that
makes that thing go away, ie just look for the line in the diffs that says
- int up_flag, key;
and then you need to look at which of those are totally bogus and are just
because it shows the diff against one of the earlier trees that also don't
have that "key = adbhid[...]" line!
(And that is actually *much* less obvious than it should be, since a lot
of the case of those lines going away is becuase I had merged Dmitry's
tree in the first place)
You can make git help you narrow it down a bit more by using -S, ie some
horrible command line from hell like this:
git whatchanged -S'int up_flag, key;' -m -p \
555ddbb4e2191c8823df2d61525218ac39481385.. \
drivers/macintosh/adbhid.c
will actually show only those commits that add/remove a line like the one
you are wondering where it went. That can cut down on the noise a bit, but
you'll get all the same false alarms, so no, it's probably not worth it.
In general, I'm afraid that merge errors are simply not very easy to find.
The problem in this case is in that b981d8b3f5 merge, but if you actually
then do a "git show b981d8b3f5" you won't even see the problem spot in the
default "--cc" format output, because Dmitry had resolved that problmatic
place to be the same as one of the parent branches, and that makes git not
show the diff for it (since it's "uninteresting")
Linus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fix adbhid mismerge
2007-10-17 1:21 ` Linus Torvalds
@ 2007-10-17 2:21 ` Linus Torvalds
2007-10-17 16:18 ` Björn Steinbrink
0 siblings, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2007-10-17 2:21 UTC (permalink / raw
To: Al Viro; +Cc: linux-kernel
On Tue, 16 Oct 2007, Linus Torvalds wrote:
>
> I don't think you did anything wrong. You used both --full-history
> (implicitly: git-whatchanged) and you made sure to see the diffs for both
> sides of any merge (-m), and that means that you should see every single
> diff involved.
Btw, if anybody can come up with a better way to find these kinds of
mis-merges, I'd love to hear about it.
In *this* particular case, the -c flag ("combined" merge diff) probably
comes closest, and is certainly a lot better than passing in -m (which
shows each merge against both parents separately), and in fact, I think
you would have found the mis-merge immediately if you had used
git whatchanged -p -c v2.6.23.. drivers/macintosh/adbhid.c
but I'm not going to guarantee that -c always gives you what you want.
In general, the rules are:
- the default for merge diffs is to show "condensed combined" merge, ie
the diff of only those parts where the result actively differs from
*both* parents.
This is very terse, and it has the wonderful property of showing merges
where you actually ended up doing "real work" and not just picking one
side or the other, but in this case the very fact that the mis-merge
had picked one side (and it really would have _needed_ a correct manual
merge) also meant that the default "--cc" format didn't show anything
at all.
- "-c" is for regular combined merges: any file that was modified in both
parents will show up as a combination of the diffs of both sides, while
a file that was taken in its *entirety* is ignored.
In this case that's exactly what you wanted. It's just too noisy to
necessarily be the default, and you can still have a silent mis-merge
if the merger picked *only* one side.
But in general, I suspect that "-c" is often a good thing to try if you
cannot find the cause of some change in a regular commit, and suspect a
merge error.
- "-m" shows each side totally independently. Quite frankly, I've never
found it useful. It is essentially guaranteed to show all changes,
since it shows the patches against all parents individually, so even if
we took only one side, we'll still show the patch against the *other*
side, but quite frankly, while it's thus useful in theory, in practice
the end result is just too noisy to likely ever really be useful as
anything but a "yes, the information is there (..but it's practically
impossible to find for all the other noise that is also there)"
The main reason "-m" exists is historical: before Junio implemented the
combined formats, -m was the easy way to show *any* information. I bet -m
can be useful in some case where you have some pattern you can search for
(ie I used -m in this case to find the mis-merge, but realized only later
that I would have been better off using -c), but it's not something I'd
recommend unless you were really desperate.
What I'd actually really like would be something that shows the original
conflict, but that's really expensive to compute (it basically involves
re-doing the merge from scratch - finding the proper base commit(s) etc).
So we never did that.
Linus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fix adbhid mismerge
2007-10-17 2:21 ` Linus Torvalds
@ 2007-10-17 16:18 ` Björn Steinbrink
2007-10-17 18:28 ` Björn Steinbrink
0 siblings, 1 reply; 5+ messages in thread
From: Björn Steinbrink @ 2007-10-17 16:18 UTC (permalink / raw
To: Linus Torvalds; +Cc: Al Viro, linux-kernel
On 2007.10.16 19:21:53 -0700, Linus Torvalds wrote:
>
>
> On Tue, 16 Oct 2007, Linus Torvalds wrote:
> >
> > I don't think you did anything wrong. You used both --full-history
> > (implicitly: git-whatchanged) and you made sure to see the diffs for both
> > sides of any merge (-m), and that means that you should see every single
> > diff involved.
>
> Btw, if anybody can come up with a better way to find these kinds of
> mis-merges, I'd love to hear about it.
>
[...]
>
> What I'd actually really like would be something that shows the original
> conflict, but that's really expensive to compute (it basically involves
> re-doing the merge from scratch - finding the proper base commit(s) etc).
> So we never did that.
So here's what I came up with:
git grep -l "int keycode, up_flag" \
$(git-rev-list HEAD --parents -- drivers/macintosh/adbhid.c | \
egrep '(.{40} ?){3}' | cut -d' ' -f1) \
-- drivers/macintosh/adbhid.c | grep -o '^[^:]*'
Which gives: b981d8b3f5e008ff10d993be633ad00564fc22cd
Then:
git checkout b981d8b3f5e008ff10d993be633ad00564fc22cd^1
git merge b981d8b3f5e008ff10d993be633ad00564fc22cd^2
And you got your merge conflict.
The idea is, that the above ugliness searches for the last commit that
produced the bad line. The inner git-rev-list call searches for merge
commits (thanks to Ilari in #git for the egrep trick), then git-grep
looks which of these have the "bad line" and the final grep just filters
the filename out.
If the bash thing spits out more than one commit hash, you probably want
to use the last one... I guess... And if the given result doesn't
produce the request merge conflict, well, I guess you could replace HEAD
in the git-rev-list call with the sha1 you got in the first run, but I'm
not entirely sure about that.
Is that helpful?
Björn
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fix adbhid mismerge
2007-10-17 16:18 ` Björn Steinbrink
@ 2007-10-17 18:28 ` Björn Steinbrink
0 siblings, 0 replies; 5+ messages in thread
From: Björn Steinbrink @ 2007-10-17 18:28 UTC (permalink / raw
To: Linus Torvalds; +Cc: Al Viro, linux-kernel
On 2007.10.17 18:18:21 +0200, Björn Steinbrink wrote:
> On 2007.10.16 19:21:53 -0700, Linus Torvalds wrote:
> >
> >
> > On Tue, 16 Oct 2007, Linus Torvalds wrote:
> > >
> > > I don't think you did anything wrong. You used both --full-history
> > > (implicitly: git-whatchanged) and you made sure to see the diffs for both
> > > sides of any merge (-m), and that means that you should see every single
> > > diff involved.
> >
> > Btw, if anybody can come up with a better way to find these kinds of
> > mis-merges, I'd love to hear about it.
> >
> [...]
> >
> > What I'd actually really like would be something that shows the original
> > conflict, but that's really expensive to compute (it basically involves
> > re-doing the merge from scratch - finding the proper base commit(s) etc).
> > So we never did that.
>
> So here's what I came up with:
>
> git grep -l "int keycode, up_flag" \
> $(git-rev-list HEAD --parents -- drivers/macintosh/adbhid.c | \
> egrep '(.{40} ?){3}' | cut -d' ' -f1) \
> -- drivers/macintosh/adbhid.c | grep -o '^[^:]*'
>
> Which gives: b981d8b3f5e008ff10d993be633ad00564fc22cd
>
> Then:
> git checkout b981d8b3f5e008ff10d993be633ad00564fc22cd^1
> git merge b981d8b3f5e008ff10d993be633ad00564fc22cd^2
>
> And you got your merge conflict.
>
> The idea is, that the above ugliness searches for the last commit that
Oops!
Obviously I meant to do s/last commit/merge commit/ before sending that
email.
> produced the bad line. The inner git-rev-list call searches for merge
> commits (thanks to Ilari in #git for the egrep trick), then git-grep
> looks which of these have the "bad line" and the final grep just filters
> the filename out.
>
> If the bash thing spits out more than one commit hash, you probably want
> to use the last one... I guess... And if the given result doesn't
> produce the request merge conflict, well, I guess you could replace HEAD
> in the git-rev-list call with the sha1 you got in the first run, but I'm
> not entirely sure about that.
>
> Is that helpful?
>
> Björn
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-10-17 18:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-17 0:02 [PATCH] fix adbhid mismerge Al Viro
2007-10-17 1:21 ` Linus Torvalds
2007-10-17 2:21 ` Linus Torvalds
2007-10-17 16:18 ` Björn Steinbrink
2007-10-17 18:28 ` Björn Steinbrink
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).