2023-03-21 07:25:56 +01:00
|
|
|
#include "git-compat-util.h"
|
2007-04-10 02:01:27 +02:00
|
|
|
#include "diff.h"
|
|
|
|
#include "commit.h"
|
2020-12-31 12:56:23 +01:00
|
|
|
#include "hash-lookup.h"
|
2023-02-24 01:09:27 +01:00
|
|
|
#include "hex.h"
|
2007-04-10 02:01:27 +02:00
|
|
|
#include "patch-ids.h"
|
|
|
|
|
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 <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-09-12 19:56:41 +02:00
|
|
|
static int patch_id_defined(struct commit *commit)
|
|
|
|
{
|
|
|
|
/* must be 0 or 1 parents */
|
|
|
|
return !commit->parents || !commit->parents->next;
|
|
|
|
}
|
|
|
|
|
2016-04-26 09:51:21 +02:00
|
|
|
int commit_patch_id(struct commit *commit, struct diff_options *options,
|
2022-10-24 22:07:40 +02:00
|
|
|
struct object_id *oid, int diff_header_only)
|
2007-04-10 02:01:27 +02:00
|
|
|
{
|
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 <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-09-12 19:56:41 +02:00
|
|
|
if (!patch_id_defined(commit))
|
|
|
|
return -1;
|
|
|
|
|
2007-04-10 02:01:27 +02:00
|
|
|
if (commit->parents)
|
2017-05-30 19:31:03 +02:00
|
|
|
diff_tree_oid(&commit->parents->item->object.oid,
|
|
|
|
&commit->object.oid, "", options);
|
2007-04-10 02:01:27 +02:00
|
|
|
else
|
2017-05-30 19:30:57 +02:00
|
|
|
diff_root_tree_oid(&commit->object.oid, "", options);
|
2007-04-10 02:01:27 +02:00
|
|
|
diffcore_std(options);
|
2022-10-24 22:07:40 +02:00
|
|
|
return diff_flush_patch_id(options, oid, diff_header_only);
|
2007-04-10 02:01:27 +02:00
|
|
|
}
|
|
|
|
|
rebase: avoid computing unnecessary patch IDs
The `rebase` family of Git commands avoid applying patches that were
already integrated upstream. They do that by using the revision walking
option that computes the patch IDs of the two sides of the rebase
(local-only patches vs upstream-only ones) and skipping those local
patches whose patch ID matches one of the upstream ones.
In many cases, this causes unnecessary churn, as already the set of
paths touched by a given commit would suffice to determine that an
upstream patch has no local equivalent.
This hurts performance in particular when there are a lot of upstream
patches, and/or large ones.
Therefore, let's introduce the concept of a "diff-header-only" patch ID,
compare those first, and only evaluate the "full" patch ID lazily.
Please note that in contrast to the "full" patch IDs, those
"diff-header-only" patch IDs are prone to collide with one another, as
adjacent commits frequently touch the very same files. Hence we now
have to be careful to allow multiple hash entries with the same hash.
We accomplish that by using the hashmap_add() function that does not even
test for hash collisions. This also allows us to evaluate the full patch ID
lazily, i.e. only when we found commits with matching diff-header-only
patch IDs.
We add a performance test that demonstrates ~1-6% improvement. In
practice this will depend on various factors such as how many upstream
changes and how big those changes are along with whether file system
caches are cold or warm. As Git's test suite has no way of catching
performance regressions, we also add a regression test that verifies
that the full patch ID computation is skipped when the diff-header-only
computation suffices.
Signed-off-by: Kevin Willford <kcwillford@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-29 18:19:20 +02:00
|
|
|
/*
|
|
|
|
* When we cannot load the full patch-id for both commits for whatever
|
|
|
|
* reason, the function returns -1 (i.e. return error(...)). Despite
|
2018-08-28 23:22:55 +02:00
|
|
|
* the "neq" in the name of this function, the caller only cares about
|
rebase: avoid computing unnecessary patch IDs
The `rebase` family of Git commands avoid applying patches that were
already integrated upstream. They do that by using the revision walking
option that computes the patch IDs of the two sides of the rebase
(local-only patches vs upstream-only ones) and skipping those local
patches whose patch ID matches one of the upstream ones.
In many cases, this causes unnecessary churn, as already the set of
paths touched by a given commit would suffice to determine that an
upstream patch has no local equivalent.
This hurts performance in particular when there are a lot of upstream
patches, and/or large ones.
Therefore, let's introduce the concept of a "diff-header-only" patch ID,
compare those first, and only evaluate the "full" patch ID lazily.
Please note that in contrast to the "full" patch IDs, those
"diff-header-only" patch IDs are prone to collide with one another, as
adjacent commits frequently touch the very same files. Hence we now
have to be careful to allow multiple hash entries with the same hash.
We accomplish that by using the hashmap_add() function that does not even
test for hash collisions. This also allows us to evaluate the full patch ID
lazily, i.e. only when we found commits with matching diff-header-only
patch IDs.
We add a performance test that demonstrates ~1-6% improvement. In
practice this will depend on various factors such as how many upstream
changes and how big those changes are along with whether file system
caches are cold or warm. As Git's test suite has no way of catching
performance regressions, we also add a regression test that verifies
that the full patch ID computation is skipped when the diff-header-only
computation suffices.
Signed-off-by: Kevin Willford <kcwillford@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-29 18:19:20 +02:00
|
|
|
* 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
|
|
|
|
* result, even if they actually were equivalent, in order to err on
|
|
|
|
* the side of safety. The actual value being negative does not have
|
|
|
|
* any significance; only that it is non-zero matters.
|
|
|
|
*/
|
2018-08-28 23:22:55 +02:00
|
|
|
static int patch_id_neq(const void *cmpfn_data,
|
2019-10-07 01:30:37 +02:00
|
|
|
const struct hashmap_entry *eptr,
|
|
|
|
const struct hashmap_entry *entry_or_key,
|
2022-08-25 19:09:48 +02:00
|
|
|
const void *keydata UNUSED)
|
2007-04-10 02:01:27 +02:00
|
|
|
{
|
2017-07-01 02:28:34 +02:00
|
|
|
/* NEEDSWORK: const correctness? */
|
|
|
|
struct diff_options *opt = (void *)cmpfn_data;
|
2019-10-07 01:30:37 +02:00
|
|
|
struct patch_id *a, *b;
|
|
|
|
|
|
|
|
a = container_of(eptr, struct patch_id, ent);
|
|
|
|
b = container_of(entry_or_key, struct patch_id, ent);
|
2017-07-01 02:28:34 +02:00
|
|
|
|
2017-05-30 19:30:53 +02:00
|
|
|
if (is_null_oid(&a->patch_id) &&
|
2022-10-24 22:07:40 +02:00
|
|
|
commit_patch_id(a->commit, opt, &a->patch_id, 0))
|
rebase: avoid computing unnecessary patch IDs
The `rebase` family of Git commands avoid applying patches that were
already integrated upstream. They do that by using the revision walking
option that computes the patch IDs of the two sides of the rebase
(local-only patches vs upstream-only ones) and skipping those local
patches whose patch ID matches one of the upstream ones.
In many cases, this causes unnecessary churn, as already the set of
paths touched by a given commit would suffice to determine that an
upstream patch has no local equivalent.
This hurts performance in particular when there are a lot of upstream
patches, and/or large ones.
Therefore, let's introduce the concept of a "diff-header-only" patch ID,
compare those first, and only evaluate the "full" patch ID lazily.
Please note that in contrast to the "full" patch IDs, those
"diff-header-only" patch IDs are prone to collide with one another, as
adjacent commits frequently touch the very same files. Hence we now
have to be careful to allow multiple hash entries with the same hash.
We accomplish that by using the hashmap_add() function that does not even
test for hash collisions. This also allows us to evaluate the full patch ID
lazily, i.e. only when we found commits with matching diff-header-only
patch IDs.
We add a performance test that demonstrates ~1-6% improvement. In
practice this will depend on various factors such as how many upstream
changes and how big those changes are along with whether file system
caches are cold or warm. As Git's test suite has no way of catching
performance regressions, we also add a regression test that verifies
that the full patch ID computation is skipped when the diff-header-only
computation suffices.
Signed-off-by: Kevin Willford <kcwillford@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-29 18:19:20 +02:00
|
|
|
return error("Could not get patch ID for %s",
|
|
|
|
oid_to_hex(&a->commit->object.oid));
|
2017-05-30 19:30:53 +02:00
|
|
|
if (is_null_oid(&b->patch_id) &&
|
2022-10-24 22:07:40 +02:00
|
|
|
commit_patch_id(b->commit, opt, &b->patch_id, 0))
|
rebase: avoid computing unnecessary patch IDs
The `rebase` family of Git commands avoid applying patches that were
already integrated upstream. They do that by using the revision walking
option that computes the patch IDs of the two sides of the rebase
(local-only patches vs upstream-only ones) and skipping those local
patches whose patch ID matches one of the upstream ones.
In many cases, this causes unnecessary churn, as already the set of
paths touched by a given commit would suffice to determine that an
upstream patch has no local equivalent.
This hurts performance in particular when there are a lot of upstream
patches, and/or large ones.
Therefore, let's introduce the concept of a "diff-header-only" patch ID,
compare those first, and only evaluate the "full" patch ID lazily.
Please note that in contrast to the "full" patch IDs, those
"diff-header-only" patch IDs are prone to collide with one another, as
adjacent commits frequently touch the very same files. Hence we now
have to be careful to allow multiple hash entries with the same hash.
We accomplish that by using the hashmap_add() function that does not even
test for hash collisions. This also allows us to evaluate the full patch ID
lazily, i.e. only when we found commits with matching diff-header-only
patch IDs.
We add a performance test that demonstrates ~1-6% improvement. In
practice this will depend on various factors such as how many upstream
changes and how big those changes are along with whether file system
caches are cold or warm. As Git's test suite has no way of catching
performance regressions, we also add a regression test that verifies
that the full patch ID computation is skipped when the diff-header-only
computation suffices.
Signed-off-by: Kevin Willford <kcwillford@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-29 18:19:20 +02:00
|
|
|
return error("Could not get patch ID for %s",
|
|
|
|
oid_to_hex(&b->commit->object.oid));
|
2018-08-28 23:22:55 +02:00
|
|
|
return !oideq(&a->patch_id, &b->patch_id);
|
2007-04-10 02:01:27 +02:00
|
|
|
}
|
|
|
|
|
2018-09-21 17:57:30 +02:00
|
|
|
int init_patch_ids(struct repository *r, struct patch_ids *ids)
|
2007-04-10 02:01:27 +02:00
|
|
|
{
|
|
|
|
memset(ids, 0, sizeof(*ids));
|
2018-09-21 17:57:30 +02:00
|
|
|
repo_diff_setup(r, &ids->diffopts);
|
patch-ids: turn off rename detection
The patch-id code may be running inside another porcelain
like "git log" or "git format-patch", and therefore may have
set diff_detect_rename_default, either via the diff-ui
config, or by default since 5404c11 (diff: activate
diff.renames by default, 2016-02-25). This is the case even
if a command is run with `--no-renames`, as that is applied
only to the diff-options used by the command itself.
Rename detection doesn't help the patch-id results. It
_may_ actually hurt, as minor differences in the files that
would be overlooked by patch-id's canonicalization might
result in different renames (though I'd doubt that it ever
comes up in practice).
But mostly it is just a waste of CPU to compute these
renames.
Note that this does have one user-visible impact: the
prerequisite patches listed by "format-patch --base". There
may be some confusion between different versions of git as
older ones will enable renames, but newer ones will not.
However, this was already a problem, as people with
different settings for the "diff.renames" config would get
different results. After this patch, everyone should get the
same results, regardless of their config.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-09-09 22:34:34 +02:00
|
|
|
ids->diffopts.detect_rename = 0;
|
2017-10-31 19:19:11 +01:00
|
|
|
ids->diffopts.flags.recursive = 1;
|
2012-08-03 14:16:24 +02:00
|
|
|
diff_setup_done(&ids->diffopts);
|
2018-08-28 23:22:55 +02:00
|
|
|
hashmap_init(&ids->patches, patch_id_neq, &ids->diffopts, 256);
|
2007-04-10 02:01:27 +02:00
|
|
|
return 0;
|
|
|
|
}
|
|
|
|
|
|
|
|
int free_patch_ids(struct patch_ids *ids)
|
|
|
|
{
|
2020-11-02 19:55:05 +01:00
|
|
|
hashmap_clear_and_free(&ids->patches, struct patch_id, ent);
|
2007-04-10 02:01:27 +02:00
|
|
|
return 0;
|
|
|
|
}
|
|
|
|
|
2016-07-29 18:19:17 +02:00
|
|
|
static int init_patch_id_entry(struct patch_id *patch,
|
|
|
|
struct commit *commit,
|
|
|
|
struct patch_ids *ids)
|
2007-04-10 02:01:27 +02:00
|
|
|
{
|
2017-05-30 19:30:53 +02:00
|
|
|
struct object_id header_only_patch_id;
|
rebase: avoid computing unnecessary patch IDs
The `rebase` family of Git commands avoid applying patches that were
already integrated upstream. They do that by using the revision walking
option that computes the patch IDs of the two sides of the rebase
(local-only patches vs upstream-only ones) and skipping those local
patches whose patch ID matches one of the upstream ones.
In many cases, this causes unnecessary churn, as already the set of
paths touched by a given commit would suffice to determine that an
upstream patch has no local equivalent.
This hurts performance in particular when there are a lot of upstream
patches, and/or large ones.
Therefore, let's introduce the concept of a "diff-header-only" patch ID,
compare those first, and only evaluate the "full" patch ID lazily.
Please note that in contrast to the "full" patch IDs, those
"diff-header-only" patch IDs are prone to collide with one another, as
adjacent commits frequently touch the very same files. Hence we now
have to be careful to allow multiple hash entries with the same hash.
We accomplish that by using the hashmap_add() function that does not even
test for hash collisions. This also allows us to evaluate the full patch ID
lazily, i.e. only when we found commits with matching diff-header-only
patch IDs.
We add a performance test that demonstrates ~1-6% improvement. In
practice this will depend on various factors such as how many upstream
changes and how big those changes are along with whether file system
caches are cold or warm. As Git's test suite has no way of catching
performance regressions, we also add a regression test that verifies
that the full patch ID computation is skipped when the diff-header-only
computation suffices.
Signed-off-by: Kevin Willford <kcwillford@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-29 18:19:20 +02:00
|
|
|
|
2016-07-29 18:19:18 +02:00
|
|
|
patch->commit = commit;
|
2022-10-24 22:07:40 +02:00
|
|
|
if (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))
|
2016-07-29 18:19:17 +02:00
|
|
|
return -1;
|
2007-04-10 02:01:27 +02:00
|
|
|
|
2019-10-07 01:30:27 +02:00
|
|
|
hashmap_entry_init(&patch->ent, oidhash(&header_only_patch_id));
|
2016-07-29 18:19:17 +02:00
|
|
|
return 0;
|
2007-04-10 02:01:27 +02:00
|
|
|
}
|
|
|
|
|
patch-ids: handle duplicate hashmap entries
This fixes a bug introduced in dfb7a1b4d0 (patch-ids: stop using a
hand-rolled hashmap implementation, 2016-07-29) in which
git rev-list --cherry-pick A...B
will fail to suppress commits reachable from A even if a commit with
matching patch-id appears in B.
Around the time of that commit, the algorithm for "--cherry-pick" looked
something like this:
0. Traverse all of the commits, marking them as being on the left or
right side of the symmetric difference.
1. Iterate over the left-hand commits, inserting a patch-id struct for
each into a hashmap, and pointing commit->util to the patch-id
struct.
2. Iterate over the right-hand commits, checking which are present in
the hashmap. If so, we exclude the commit from the output _and_ we
mark the patch-id as "seen".
3. Iterate again over the left-hand commits, checking whether
commit->util->seen is set; if so, exclude them from the output.
At the end, we'll have eliminated commits from both sides that have a
matching patch-id on the other side. But there's a subtle assumption
here: for any given patch-id, we must have exactly one struct
representing it. If two commits from A both have the same patch-id and
we allow duplicates in the hashmap, then we run into a problem:
a. In step 1, we insert two patch-id structs into the hashmap.
b. In step 2, our lookups will find only one of these structs, so only
one "seen" flag is marked.
c. In step 3, one of the commits in A will have its commit->util->seen
set, but the other will not. We'll erroneously output the latter.
Prior to dfb7a1b4d0, our hashmap did not allow duplicates. Afterwards,
it used hashmap_add(), which explicitly does allow duplicates.
At that point, the solution would have been easy: when we are about to
add a duplicate, skip doing so and return the existing entry which
matches. But it gets more complicated.
In 683f17ec44 (patch-ids: replace the seen indicator with a commit
pointer, 2016-07-29), our step 3 goes away entirely. Instead, in step 2,
when the right-hand side finds a matching patch_id from the left-hand
side, we can directly mark the left-hand patch_id->commit to be omitted.
Solving that would be easy, too; there's a one-to-many relationship of
patch-ids to commits, so we just need to keep a list.
But there's more. Commit b3dfeebb92 (rebase: avoid computing unnecessary
patch IDs, 2016-07-29) built on that by lazily computing the full
patch-ids. So we don't even know when adding to the hashmap whether two
commits truly have the same id. We'd have to tentatively assign them a
list, and then possibly split them apart (possibly into N new structs)
at the moment we compute the real patch-ids. This could work, but it's
complicated and error-prone.
Instead, let's accept that we may store duplicates, and teach the lookup
side to be more clever. Rather than asking for a single matching
patch-id, it will need to iterate over all matching patch-ids. This does
mean examining every entry in a single hash bucket, but the worst-case
for a hash lookup was already doing that.
We'll keep the hashmap details out of the caller by providing a simple
iteration interface. We can retain the simple has_commit_patch_id()
interface for the other callers, but we'll simplify its return value
into an integer, rather than returning the patch_id struct. That way
they won't be tempted to look at the "commit" field of the return value
without iterating.
Reported-by: Arnaud Morin <arnaud.morin@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-01-12 16:52:32 +01:00
|
|
|
struct patch_id *patch_id_iter_first(struct commit *commit,
|
2007-04-10 02:01:27 +02:00
|
|
|
struct patch_ids *ids)
|
|
|
|
{
|
2016-07-29 18:19:17 +02:00
|
|
|
struct patch_id 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 <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-09-12 19:56:41 +02:00
|
|
|
if (!patch_id_defined(commit))
|
|
|
|
return NULL;
|
|
|
|
|
2016-07-29 18:19:17 +02:00
|
|
|
memset(&patch, 0, sizeof(patch));
|
|
|
|
if (init_patch_id_entry(&patch, commit, ids))
|
|
|
|
return NULL;
|
|
|
|
|
2019-10-07 01:30:42 +02:00
|
|
|
return hashmap_get_entry(&ids->patches, &patch, ent, NULL);
|
2007-04-10 02:01:27 +02:00
|
|
|
}
|
|
|
|
|
patch-ids: handle duplicate hashmap entries
This fixes a bug introduced in dfb7a1b4d0 (patch-ids: stop using a
hand-rolled hashmap implementation, 2016-07-29) in which
git rev-list --cherry-pick A...B
will fail to suppress commits reachable from A even if a commit with
matching patch-id appears in B.
Around the time of that commit, the algorithm for "--cherry-pick" looked
something like this:
0. Traverse all of the commits, marking them as being on the left or
right side of the symmetric difference.
1. Iterate over the left-hand commits, inserting a patch-id struct for
each into a hashmap, and pointing commit->util to the patch-id
struct.
2. Iterate over the right-hand commits, checking which are present in
the hashmap. If so, we exclude the commit from the output _and_ we
mark the patch-id as "seen".
3. Iterate again over the left-hand commits, checking whether
commit->util->seen is set; if so, exclude them from the output.
At the end, we'll have eliminated commits from both sides that have a
matching patch-id on the other side. But there's a subtle assumption
here: for any given patch-id, we must have exactly one struct
representing it. If two commits from A both have the same patch-id and
we allow duplicates in the hashmap, then we run into a problem:
a. In step 1, we insert two patch-id structs into the hashmap.
b. In step 2, our lookups will find only one of these structs, so only
one "seen" flag is marked.
c. In step 3, one of the commits in A will have its commit->util->seen
set, but the other will not. We'll erroneously output the latter.
Prior to dfb7a1b4d0, our hashmap did not allow duplicates. Afterwards,
it used hashmap_add(), which explicitly does allow duplicates.
At that point, the solution would have been easy: when we are about to
add a duplicate, skip doing so and return the existing entry which
matches. But it gets more complicated.
In 683f17ec44 (patch-ids: replace the seen indicator with a commit
pointer, 2016-07-29), our step 3 goes away entirely. Instead, in step 2,
when the right-hand side finds a matching patch_id from the left-hand
side, we can directly mark the left-hand patch_id->commit to be omitted.
Solving that would be easy, too; there's a one-to-many relationship of
patch-ids to commits, so we just need to keep a list.
But there's more. Commit b3dfeebb92 (rebase: avoid computing unnecessary
patch IDs, 2016-07-29) built on that by lazily computing the full
patch-ids. So we don't even know when adding to the hashmap whether two
commits truly have the same id. We'd have to tentatively assign them a
list, and then possibly split them apart (possibly into N new structs)
at the moment we compute the real patch-ids. This could work, but it's
complicated and error-prone.
Instead, let's accept that we may store duplicates, and teach the lookup
side to be more clever. Rather than asking for a single matching
patch-id, it will need to iterate over all matching patch-ids. This does
mean examining every entry in a single hash bucket, but the worst-case
for a hash lookup was already doing that.
We'll keep the hashmap details out of the caller by providing a simple
iteration interface. We can retain the simple has_commit_patch_id()
interface for the other callers, but we'll simplify its return value
into an integer, rather than returning the patch_id struct. That way
they won't be tempted to look at the "commit" field of the return value
without iterating.
Reported-by: Arnaud Morin <arnaud.morin@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-01-12 16:52:32 +01:00
|
|
|
struct patch_id *patch_id_iter_next(struct patch_id *cur,
|
|
|
|
struct patch_ids *ids)
|
|
|
|
{
|
|
|
|
return hashmap_get_next_entry(&ids->patches, cur, ent);
|
|
|
|
}
|
|
|
|
|
|
|
|
int has_commit_patch_id(struct commit *commit,
|
|
|
|
struct patch_ids *ids)
|
|
|
|
{
|
|
|
|
return !!patch_id_iter_first(commit, ids);
|
|
|
|
}
|
|
|
|
|
2007-04-10 02:01:27 +02:00
|
|
|
struct patch_id *add_commit_patch_id(struct commit *commit,
|
|
|
|
struct patch_ids *ids)
|
|
|
|
{
|
2017-05-04 15:55:38 +02:00
|
|
|
struct patch_id *key;
|
2016-07-29 18:19:17 +02:00
|
|
|
|
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 <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-09-12 19:56:41 +02:00
|
|
|
if (!patch_id_defined(commit))
|
|
|
|
return NULL;
|
|
|
|
|
2021-03-13 17:17:22 +01:00
|
|
|
CALLOC_ARRAY(key, 1);
|
2016-07-29 18:19:17 +02:00
|
|
|
if (init_patch_id_entry(key, commit, ids)) {
|
|
|
|
free(key);
|
|
|
|
return NULL;
|
|
|
|
}
|
|
|
|
|
2019-10-07 01:30:29 +02:00
|
|
|
hashmap_add(&ids->patches, &key->ent);
|
2016-07-29 18:19:17 +02:00
|
|
|
return key;
|
2007-04-10 02:01:27 +02:00
|
|
|
}
|