Re: [PATCH v3 2/2] patch-ids: define patch-id of merge commits as "null"

[Junio C Hamano]

Jeff King  writes:

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

At first I wondered if such a change would make all merges look the
same, but the patch-ids.c comparison is not for ordering/sorting but
only for equality, so as long as the comparison function knows that
a comparison of anything with "null" yields "They are different", we
are OK.

> diff --git a/patch-ids.c b/patch-ids.c
> index 77e4663..8d06099 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -7,18 +7,40 @@
>  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 PATCH_ID_NONE;
>   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);
> - return diff_flush_patch_id(options, sha1, diff_header_only);
> + if (diff_flush_patch_id(options, sha1, diff_header_only))
> + return PATCH_ID_ERROR;
> + return PATCH_ID_OK;
> +}

Looks sensible.  Thanks.

Re: [PATCH v3 2/2] patch-ids: define patch-id of merge commits as "null"

[Jeff King]

On Fri, Sep 09, 2016 at 04:34:47PM -0400, Jeff King wrote:

> 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.
> 
> To distinguish between real errors and "null", we have to
> expand the semantics of commit_patch_id()'s return value,
> and callers need to distinguish these cases.

One alternative would be to add an out-parameter that is set in the
success case saying "yes, we have a real patch-id". And then the callers
could look like:

  if (commit_patch_id(commit, , sha1, 0, _one))
die("error!");
  if (!got_one)
continue; /* silently skip */

We could even use the null sha1 to signal that rather than an extra
parameter, I suppose.

I dunno. It would make the callers less clunky, I think, but it does
feel a bit magical.

-Peff

[PATCH v3 2/2] patch-ids: define patch-id of merge commits as "null"

[Jeff King]

The patch-id 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 identical to 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.

To distinguish between real errors and "null", we have to
expand the semantics of commit_patch_id()'s return value,
and callers need to distinguish these cases.

Helped-by: Johannes Schindelin 
Signed-off-by: Jeff King 
---
 builtin/log.c | 10 +-
 patch-ids.c   | 40 
 patch-ids.h   | 11 +--
 3 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 92dc34d..ced1ea7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1343,8 +1343,16 @@ static void prepare_bases(struct base_tree_info *bases,
struct object_id *patch_id;
if (commit->util)
continue;
-   if (commit_patch_id(commit, , sha1, 0))
+
+   switch (commit_patch_id(commit, , sha1, 0)) {
+   case PATCH_ID_OK:
+   break;
+   case PATCH_ID_NONE:
+   continue;
+   case PATCH_ID_ERROR:
die(_("cannot get patch id"));
+   }
+
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 77e4663..8d06099 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -7,18 +7,40 @@
 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 PATCH_ID_NONE;
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);
-   return diff_flush_patch_id(options, sha1, diff_header_only);
+   if (diff_flush_patch_id(options, sha1, diff_header_only))
+   return PATCH_ID_ERROR;
+   return PATCH_ID_OK;
+}
+
+/* avoid repeating ourselves in patch_id_cmp */
+static int cmp_setup(struct patch_id *p, struct diff_options *opt)
+{
+   if (!is_null_sha1(p->patch_id))
+   return 0; /* OK, already computed id */
+
+   switch (commit_patch_id(p->commit, opt, p->patch_id, 0)) {
+   case PATCH_ID_OK:
+   return 0;
+   case PATCH_ID_ERROR:
+   return error("Could not get patch ID for %s",
+oid_to_hex(>commit->object.oid));
+   case PATCH_ID_NONE:
+   return -1; /* not an error, but nothing to compare */
+   }
+   die("BUG: unhandled patch_result");
 }
 
 /*
  * 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
@@