Re: [PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits

[Jeff King]

On Wed, Sep 07, 2016 at 03:28:10PM -0700, Jacob Keller wrote:

> On Wed, Sep 7, 2016 at 3:04 PM, Jeff King  wrote:
> > The patch-ids code which powers "log --cherry-pick" doesn't
> > look at whether each commit is a merge or not. It just feeds
> > the commit's first parent to the diff, and ignores any
> > additional parents.
> >
> 
> The subject here is misleading since it says you will use sha1 of the
> merge commit ,but instead just ignore merge commits and indicate they
> have no patch id. I suspect this is because you switched
> implementations part way through developing this.

Oops, yes, that's exactly what happened. I'll wait for discussion to
settle on the approach and fix it up in the re-roll.

-Peff

Re: [PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits

[Jacob Keller]

On Wed, Sep 7, 2016 at 3:04 PM, Jeff King  wrote:
> The patch-ids code which powers "log --cherry-pick" doesn't
> look at whether each commit is a merge or not. It just feeds
> the commit's first parent to the diff, and ignores any
> additional parents.
>

The subject here is misleading since it says you will use sha1 of the
merge commit ,but instead just ignore merge commits and indicate they
have no patch id. I suspect this is because you switched
implementations part way through developing this.

Thanks,
Jake

[PATCH 3/3] patch-ids: use commit sha1 as patch-id for merge commits

[Jeff King]

The patch-ids code which powers "log --cherry-pick" doesn't
look at whether each commit is a merge or not. It just feeds
the commit's first parent to the diff, and ignores any
additional parents.

In theory, this might be useful if you wanted to find
equivalence between, say, a merge commit and a squash-merge
that does the same thing.  But it also promotes a false
equivalence between distinct merges; for example, every
"merge -s ours" would look like an empty commit (which is
true in a sense, but presumably there was a value in merging
in the discarded history). Since patch-ids are meant for
throwing away duplicates, we should err on the side of _not_
matching such merges.

Moreover, we may spend a lot of extra time computing these
merge diffs. In the case that inspired this patch, a "git
format-patch --cherry-pick" dropped from over 3 minutes to
less than 4 seconds.

This seems pretty drastic, but is easily explained. The
command was invoked by a "git rebase" of an older topic
branch; there had been tens of thousands of commits on the
upstream branch in the meantime. In addition, this project
used a topic-branch workflow with occasional "back-merges"
from "master" to each topic (to resolve conflicts on the
topics rather than in the merge commits). So there were not
only extra merges, but the diffs for these back-merges were
generally quite large (because they represented _everything_
that had been merged to master since the topic branched).

This patch defines the patch-id of a merge commit as
essentially "null"; it has no patch-id. As a result,
merges cannot match patch-ids via "--cherry-pick", and
"format-patch --base" will not list merges in its list of
prerequisite patch ids.

We can signal the null patch-id by returning "-1" from
commit_patch_id(), as it no longer returns any meaningful
errors (and as a result, its callers are updated to handle
"-1" differently).

Helped-by: Johannes Schindelin 
Signed-off-by: Jeff King 
---
 builtin/log.c |  2 +-
 patch-ids.c   | 14 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 92dc34d..e660034 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1344,7 +1344,7 @@ static void prepare_bases(struct base_tree_info *bases,
if (commit->util)
continue;
if (commit_patch_id(commit, , sha1, 0))
-   die(_("cannot get patch id"));
+   continue;
ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, 
bases->alloc_patch_id);
patch_id = bases->patch_id + bases->nr_patch_id;
hashcpy(patch_id->hash, sha1);
diff --git a/patch-ids.c b/patch-ids.c
index 0e95220..197c70b 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -7,10 +7,12 @@
 int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1, int diff_header_only)
 {
-   if (commit->parents)
+   if (commit->parents) {
+   if (commit->parents->next)
+   return -1;
diff_tree_sha1(commit->parents->item->object.oid.hash,
   commit->object.oid.hash, "", options);
-   else
+   } else
diff_root_tree_sha1(commit->object.oid.hash, "", options);
diffcore_std(options);
diff_flush_patch_id(options, sha1, diff_header_only);
@@ -19,7 +21,7 @@ int commit_patch_id(struct commit *commit, struct 
diff_options *options,
 
 /*
  * When we cannot load the full patch-id for both commits for whatever
- * reason, the function returns -1 (i.e. return error(...)). Despite
+ * reason, the function returns -1. Despite
  * the "cmp" in the name of this function, the caller only cares about
  * the return value being zero (a and b are equivalent) or non-zero (a
  * and b are different), and returning non-zero would keep both in the
@@ -33,12 +35,10 @@ static int patch_id_cmp(struct patch_id *a,
 {
if (is_null_sha1(a->patch_id) &&
commit_patch_id(a->commit, opt, a->patch_id, 0))
-   return error("Could not get patch ID for %s",
-   oid_to_hex(>commit->object.oid));
+   return -1;
if (is_null_sha1(b->patch_id) &&
commit_patch_id(b->commit, opt, b->patch_id, 0))
-   return error("Could not get patch ID for %s",
-   oid_to_hex(>commit->object.oid));
+   return -1;
return hashcmp(a->patch_id, b->patch_id);
 }
 
-- 
2.10.0.rc2.154.gb4a4b8b