Re: [PATCH v3 0/2] patch-id for merges
Hi Peff, On Fri, 9 Sep 2016, Jeff King wrote: > Frankly, I still like v2 better, but I do not feel like arguing with > Johannes about it anymore. Sorry that I was a pest. I do care deeply about making our code cleaner, though, which in my mind means less of this "let exit() clean everything up and give the callers no chance to do anything anymore" approach, which to me always felt like we're basically teaching Git to shrug instead of being useful when something bad happens. Ciao, Dscho
Re: [PATCH v3 0/2] patch-id for merges
Jeff King writes:
> On Mon, Sep 12, 2016 at 10:18:33AM -0700, Junio C Hamano wrote:
>
>> > +static int patch_id_defined(struct commit *commit)
>> > +{
>> > + /* must be 0 or 1 parents */
>> > + return !commit->parents || !commit->parents->next;
>> > +}
>>
>> If we make the first hunk begin like so:
>>
>> > + if (commit->parents) {
>> > + if (!patch_id_defined(commit))
>> > + return -1;
>>
>> I wonder if the compiler gives us the same code.
>
> Good idea. I actually put the "patch_id_defined" check outside the "if"
> block you've quoted (otherwise we're making assumptions about the
> contents of patch_id_defined).
Facepalm. I was mis-reading the condition in the helper function.
Of course, guarding up-front makes more sense.
Re: [PATCH v3 0/2] patch-id for merges
On Mon, Sep 12, 2016 at 10:18:33AM -0700, Junio C Hamano wrote:
> > +static int patch_id_defined(struct commit *commit)
> > +{
> > + /* must be 0 or 1 parents */
> > + return !commit->parents || !commit->parents->next;
> > +}
>
> If we make the first hunk begin like so:
>
> > + if (commit->parents) {
> > + if (!patch_id_defined(commit))
> > + return -1;
>
> I wonder if the compiler gives us the same code.
Good idea. I actually put the "patch_id_defined" check outside the "if"
block you've quoted (otherwise we're making assumptions about the
contents of patch_id_defined).
I didn't check that the compiler generates the same code, but I'm
willing to blindly put faith in it. Either it will inline
patch_id_defined and optimize out the double-conditional, or it probably
doesn't matter in practice. Either way, the compiler is probably smarter
than me, and we should shoot for readability and not repeating
ourselves.
> > I'd probably do a preparatory patch to drop the return value from
> > add_commit_patch_id(). No callers actually look at it.
I decided against this. Technically add_commit_patch_id() can return an
error via the header-only diff_flush_patch_id(), and we'd be shutting
that down. Of course no callers actually _care_ about that right now, so
it doesn't matter at this point. But I'd prefer to punt it down the line
for when somebody does (and the solution may be to distinguish between
those two return codes, or it may be for the caller to have access to
patch_id_defined(); we won't know until we see the code).
So here's the replacement for 2/2:
-- >8 --
Subject: [PATCH] patch-ids: refuse to compute patch-id for merge commit
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 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 3 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 treats a merge fed to commit_patch_id() or
add_commit_patch_id() as an error, and a lookup for such a
merge via has_commit_patch_id() will always return NULL.
An earlier version of the patch tried to distinguish between
"error" and "patch id for merges not defined", but that
becomes unnecessarily complicated. The only callers are:
1. revision traversals which want to do --cherry-pick;
they call add_commit_patch_id(), but do not care if it
fails. They only want to add what we can, look it up
later with has_commit_patch_id(), and err on the side
of not-matching.
2. format-patch --base, which calls commit_patch_id().
This _does_ notice errors, but should never feed a
merge in the first place (and if it were to do so
accidentally, then this patch is a strict improvement;
we notice the bug rather than generating a bogus
patch-id).
So in both cases, this does the right thing.
Helped-by: Johannes Schindelin
Signed-off-by: Jeff King
---
patch-ids.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/patch-ids.c b/patch-ids.c
index 77e4663..ce285c2 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -4,9 +4,18 @@
#include "sha1-lookup.h"
#include "patch-ids.h"
+static int patch_id_defined(struct commit *commit)
+{
+ /* must be 0 or 1 parents */
+ return !commit->parents || !commit->parents->next;
+}
+
int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1, int diff_header_only)
{
+ if (!patch_id_defined(commit))
+ return -1;
+
if (commit->parents)
diff_tree_sha1(commit->parents->item->object.oid.hash,
commit->object.oid.hash, "", options);
@@ -77,6 +86,9 @@ struct patch_id *has_c
Re: [PATCH v3 0/2] patch-id for merges
Jeff King writes:
> 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.
After reading it, I did not find [v3 2/2] is too bad either, but
this is even better ;-)
> 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);
This looks familiar ;-)
> @@ -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;
> +}
If we make the first hunk begin like so:
> + if (commit->parents) {
> + if (!patch_id_defined(commit))
> + return -1;
I wonder if the compiler gives us the same code.
> 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;
Yup, these two hunks look a lot nicer.
> I'd probably do a preparatory patch to drop the return value from
> add_commit_patch_id(). No callers actually look at it.
Thanks.
Re: [PATCH v3 0/2] patch-id for merges
On Fri, Sep 09, 2016 at 02:01:14PM -0700, Junio C Hamano wrote:
> Jeff King 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
Re: [PATCH v3 0/2] patch-id for merges
Jeff King 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. Thanks.
[PATCH v3 0/2] patch-id for merges
On Wed, Sep 07, 2016 at 06:01:01PM -0400, Jeff King wrote: > Here's a re-roll of the series I posted at: > > > http://public-inbox.org/git/20160907075346.z6wtmqnfc6bsu...@sigill.intra.peff.net/ > > Basically, it drops the time for "format-patch --cherry-pick" on a > particular case from 3 minutes down to 3 seconds, by avoiding diffs > on merge commits. Compared to v1, it fixes the totally-broken handling > of commit_patch_id() pointed out by Johannes. > > [1/3]: patch-ids: turn off rename detection > [2/3]: diff_flush_patch_id: stop returning error result > [3/3]: patch-ids: use commit sha1 as patch-id for merge commits 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. [1/2]: patch-ids: turn off rename detection [2/2]: patch-ids: define patch-id of merge commits as "null" -Peff
