Hi,
On Tue, 27 Jan 2009, Stephen Haberman wrote:
>
> Dscho wroteÖ
>
> > As for the design bug I want to fix: imagine this history:
> >
> > ------A
> > / /
> > / /
> > ---- B
> > \ \
> > \ \
> > C-----D-----E = HEAD
> >
> > A, C and D touch the same file, and A and D agree on the contents.
> >
> > Now, rebase -p A does the following at the moment:
> >
> > ------A-----E' = HEAD
> > / /
> > / /
> > ---- B
> >
> > In other words, C is truly forgotten, and it is pretended that D never
> > happened, either. That is exactly what test case 2 in t3410 tests for
> > [*1*].
> >
> > This is insane.
>
> Agreed.
Actually, I misread t3410 a great deal. The situation is as follows:
... UPSTREAM
\
... A - B - C -D
A is a patch the upstream does not have, B is a patch UPSTREAM has,
and "git diff C^!" (i.e. the diff of C to its first parent) is _also_
identical to a diff of a merge that is in UPSTREAM.
Basically, t3410 tests that after "git rebase -i -p UPSTREAM" and leaving
the rebase script as-is, essentially, A and D are cherry-picked on top of
UPSTREAM.
> Does this mean you're just getting rid of the code that calls "rev list
> --cherry-pick"?
Only now do I understand.
I misread the code for --cherry-pick. For merges, it adds the diff to the
first parent!
I do not know if it really is desirable to have --cherry-pick handle
merges at all; I tend to think it is not.
Unfortunately, a short blame session just points to 9c6efa36 done by a
sloppy programmer: yours truly.
So I adapted my code to find the "dropped" merges in
git-rebase--interactive, too, for now, but I guess the proper fix is
something like this:
-- snipsnap --
[PATCH] --cherry-pick: do not skip merges, ever
Currently, --cherry-pick has no problem getting a patch id for merge
commits: it calculated as the patch id of the patch between the first
parent and the merge commit.
Of course, this is bogus, as it completely misses the fact that the
merge commit has other parents, too, and therefore a single patch id
would be wrong.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
---
patch-ids.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/patch-ids.c b/patch-ids.c
index 3be5d31..808a7f0 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -6,9 +6,12 @@
static int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1)
{
- if (commit->parents)
+ if (commit->parents) {
+ if (commit->parents->next)
+ return 0; /* merges do not have a patch id */
diff_tree_sha1(commit->parents->item->object.sha1,
commit->object.sha1, "", options);
+ }
else
diff_root_tree_sha1(commit->object.sha1, "", options);
diffcore_std(options);
