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