On Fri, Sep 09, 2016 at 02:01:14PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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;
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);
@@ -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)) {
free(key);
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.
-Peff