On Fri, Sep 09, 2016 at 02:01:14PM -0700, Junio C Hamano wrote:

> Jeff King <p...@peff.net> writes:
> > And here is v3. Besides commit-message fixups, it drops patch 2, and
> > instead the third patch teaches commit_patch_id() to distinguish between
> > errors and "no patch id".
> >
> > Frankly, I still like v2 better, but I do not feel like arguing with
> > Johannes about it anymore.
> FWIW, I too like the simplicity of v2, as all the error-to-die
> conversion is for cases in which there is no sane recovery path.
> I'll have to take a bit deeper look at [v3 2/2] that had to become
> more involved to decide if the additional flexibility is really
> worth it.

One other option I didn't really look at: commit_patch_id() could
consider feeding it a merge as an error, and it would be come the
caller's responsibility to avoid doing so. That should already be the
case for "format-patch --base".

We'd probably have to change add_commit_patch_id() and
has_commit_patch_id() to return NULL early when fed a merge, but that is
not too bad.

The reason I didn't pursue this is that I didn't want the definition of
"what constitutes something with no patch-id" to cross too many
abstraction layers. But it's not like we expect a multitude of
conditions; it will probably remain just "we don't handle merges" for
the foreseeable future.

That looks like the patch below (as a replacement for patch 2), which is
even less invasive. It also performs a little better on my example case,
because we avoid adding merges to the hashmap entirely.

diff --git a/patch-ids.c b/patch-ids.c
index 77e4663..5d2d96a 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;
                               commit->object.oid.hash, "", options);
-       else
+       } else
                diff_root_tree_sha1(commit->object.oid.hash, "", options);
        return diff_flush_patch_id(options, sha1, diff_header_only);
@@ -72,11 +74,20 @@ static int init_patch_id_entry(struct patch_id *patch,
        return 0;
+static int patch_id_defined(struct commit *commit)
+       /* must be 0 or 1 parents */
+       return !commit->parents || !commit->parents->next;
 struct patch_id *has_commit_patch_id(struct commit *commit,
                                     struct patch_ids *ids)
        struct patch_id patch;
+       if (!patch_id_defined(commit))
+               return NULL;
        memset(&patch, 0, sizeof(patch));
        if (init_patch_id_entry(&patch, commit, ids))
                return NULL;
@@ -89,6 +100,9 @@ struct patch_id *add_commit_patch_id(struct commit *commit,
        struct patch_id *key = xcalloc(1, sizeof(*key));
+       if (!patch_id_defined(commit))
+               return NULL;
        if (init_patch_id_entry(key, commit, ids)) {
                return NULL;

I'd probably do a preparatory patch to drop the return value from
add_commit_patch_id(). No callers actually look at it.


Reply via email to