* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Jeff King @ 2024-02-13 8:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Maarten Bosmans, git, Teng Long, Maarten Bosmans
In-Reply-To: <xmqqy1bxiiop.fsf@gitster.g>
On Tue, Feb 06, 2024 at 09:52:38AM -0800, Junio C Hamano wrote:
> > That is also a cool idea. That would probably use the functionality of
> > the cat-file batch mode, right?
>
> Not really. I was hoping that "git show" that can take multiple
> objects from its command line would directly be used, or with a new
> option that gives a separator between these objects.
How about:
cat some_commit_ids |
git show --stdin -s -z --format='%H%n%N'
?
-Peff
^ permalink raw reply
* Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids
From: Linus Arver @ 2024-02-13 8:16 UTC (permalink / raw)
To: Eric W. Biederman, Junio C Hamano
Cc: git, brian m. carlson, Eric Sunshine, Eric W. Biederman
In-Reply-To: <20231002024034.2611-2-ebiederm@gmail.com>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
>
> While looking at how to handle input of both SHA-1 and SHA-256 oids in
> get_oid_with_context, I realized that the oid_array in
> repo_for_each_abbrev might have more than one kind of oid stored in it
> simultaneously.
>
> Update to oid_array_append to ensure that oids added to an oid array
s/Update to/Update
> always have an algorithm set.
>
> Update void_hashcmp to first verify two oids use the same hash algorithm
> before comparing them to each other.
>
> With that oid-array should be safe to use with different kinds of
s/oid-array/oid_array
> oids simultaneously.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> oid-array.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/oid-array.c b/oid-array.c
> index 8e4717746c31..1f36651754ed 100644
> --- a/oid-array.c
> +++ b/oid-array.c
> @@ -6,12 +6,20 @@ void oid_array_append(struct oid_array *array, const struct object_id *oid)
> {
> ALLOC_GROW(array->oid, array->nr + 1, array->alloc);
> oidcpy(&array->oid[array->nr++], oid);
> + if (!oid->algo)
> + oid_set_algo(&array->oid[array->nr - 1], the_hash_algo);
How come we can't set oid->algo _before_ we call oidcpy()? It seems odd
that we do the copy first and then modify what we just copied after the
fact, instead of making sure that the thing we want to copy is correct
before doing the copy.
But also, if we are going to make the oid object "correct" before
invoking oidcpy(), we might as well do it when the oid is first
created/used (in the caller(s) of this function). I don't demand that
you find/demonstrate where all these places are in this series (maybe
that's a hairy problem to tackle?), but it seems cleaner in principle to
fix the creation of oid objects instead of having to make oid users
clean up their act like this after using them.
> array->sorted = 0;
> }
>
> -static int void_hashcmp(const void *a, const void *b)
> +static int void_hashcmp(const void *va, const void *vb)
> {
> - return oidcmp(a, b);
> + const struct object_id *a = va, *b = vb;
> + int ret;
> + if (a->algo == b->algo)
> + ret = oidcmp(a, b);
This makes sense (per the commit message description) ...
> + else
> + ret = a->algo > b->algo ? 1 : -1;
... but this seems to go against it? I thought you wanted to only ever
compare hashes if they were of the same algo? It would be good to add a
comment explaining why this is OK (we are no longer doing a byte-by-byte
comparison of these oids any more here like we do for oidcmp() above
which boils down to calling memcmp()).
> + return ret;
Also, in terms of style I think the "early return for errors" style
would be simpler to read. I.e.
if (a->algo > b->algo)
return 1;
if (a->algo < b->algo)
return -1;
return oidcmd(a, b);
> }
>
> void oid_array_sort(struct oid_array *array)
> --
> 2.41.0
^ permalink raw reply
* Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids
From: Kristoffer Haugsbakk @ 2024-02-13 8:31 UTC (permalink / raw)
To: Eric W. Biederman
Cc: git, brian m. carlson, Eric Sunshine, Eric W. Biederman,
Junio C Hamano
In-Reply-To: <20231002024034.2611-2-ebiederm@gmail.com>
On Mon, Oct 2, 2023, at 04:40, Eric W. Biederman wrote:
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Most of your patches have this sign-off line with your name quoted.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Jean-Noël Avila @ 2024-02-13 8:33 UTC (permalink / raw)
To: Philippe Blain, git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Michael Lohmann, Junio C Hamano
In-Reply-To: <20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v4-2-3bc9e62808f4@gmail.com>
Le 11/02/2024 à 00:35, Philippe Blain a écrit :
> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>
> 'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...],
> 2006-07-03) to show commits touching conflicted files in the range
> HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document
> rev-list's option --merge, 2006-08-04).
>
> It can be useful to look at the commit history to understand what lead
> to merge conflicts also for other mergy operations besides merges, like
> cherry-pick, revert and rebase.
>
> For rebases, an interesting range to look at is HEAD...REBASE_HEAD,
> since the conflicts are usually caused by how the code changed
> differently on HEAD since REBASE_HEAD forked from it.
>
> For cherry-picks and revert, it is less clear that
> HEAD...CHERRY_PICK_HEAD and HEAD...REVERT_HEAD are indeed interesting
> ranges, since these commands are about applying or unapplying a single
> (or a few, for cherry-pick) commit(s) on top of HEAD. However, conflicts
> encountered during these operations can indeed be caused by changes
> introduced in preceding commits on both sides of the history.
>
> Adjust the code in prepare_show_merge so it constructs the range
> HEAD...$OTHER for each of OTHER={MERGE_HEAD, CHERRY_PICK_HEAD,
> REVERT_HEAD or REBASE_HEAD}. Note that we try these pseudorefs in order,
> so keep REBASE_HEAD last since the three other operations can be
> performed during a rebase. Note also that in the uncommon case where
> $OTHER and HEAD do not share a common ancestor, this will show the
> complete histories of both sides since their root commits, which is the
> same behaviour as currently happens in that case for HEAD and
> MERGE_HEAD.
>
> Adjust the documentation of this option accordingly.
>
> Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Co-authored-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/gitk.txt | 8 ++++----
> Documentation/rev-list-options.txt | 6 ++++--
> revision.c | 31 +++++++++++++++++++++++--------
> 3 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
> index c2213bb77b..80ff4e149a 100644
> --- a/Documentation/gitk.txt
> +++ b/Documentation/gitk.txt
> @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list.
>
> --merge::
>
> - After an attempt to merge stops with conflicts, show the commits on
> - the history between two branches (i.e. the HEAD and the MERGE_HEAD)
> - that modify the conflicted files and do not exist on all the heads
> - being merged.
> + Show commits touching conflicted paths in the range `HEAD...$OTHER`,
if $OTHER is a placeholder, why not use the placeholder notation <other>
instead of a notation that could deceive the reader into thinking that
this is an actual environment variable?
> + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`,
> + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
> + when the index has unmerged entries.
>
Thanks
^ permalink raw reply
* [PATCH 01/12] paint_down_to_common: plug a memory leak
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When a commit is missing, we return early (currently pretending that no
merge basis could be found in that case). At that stage, it is possible
that a merge base could have been found already, and added to the
`result`, which is now leaked.
Let's release it, to address that memory leak.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/commit-reach.c b/commit-reach.c
index a868a575ea1..b2b102926c9 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -105,8 +105,10 @@ static struct commit_list *paint_down_to_common(struct repository *r,
parents = parents->next;
if ((p->object.flags & flags) == flags)
continue;
- if (repo_parse_commit(r, p))
+ if (repo_parse_commit(r, p)) {
+ free_commit_list(result);
return NULL;
+ }
p->object.flags |= flags;
prio_queue_put(&queue, p);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 00/12] The merge-base logic vs missing commit objects
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
This patch series is in the same spirit as what I proposed in
https://lore.kernel.org/git/pull.1651.git.1707212981.gitgitgadget@gmail.com/,
where I tackled missing tree objects. This here patch series tackles missing
commit objects instead: Git's merge-base logic handles these missing commit
objects as if there had not been any commit object at all, i.e. if two
commits' merge base commit is missing, they will be treated as if they had
no common commit history at all, which is a bug. Those commit objects should
not be missing, of course, i.e. this is only a problem in corrupt
repositories.
This patch series is a bit more tricky than the "missing tree objects" one,
though: The function signature of quite a few functions need to be changed
to allow callers to discern between "missing commit object" vs "no
merge-base found".
And of course it gets even more tricky because in shallow and partial clones
we expect commit objects to be missing, and that must not be treated like an
error but the existing logic is actually desirable in those scenarios.
I am deeply sorry both about the length of this patch series as well as
having to send it in the -rc phase.
Johannes Schindelin (12):
paint_down_to_common: plug a memory leak
Let `repo_in_merge_bases()` report missing commits
Prepare `repo_in_merge_bases_many()` to optionally expect missing
commits
Prepare `paint_down_to_common()` for handling shallow commits
commit-reach: start reporting errors in `paint_down_to_common()`
merge_bases_many(): pass on errors from `paint_down_to_common()`
get_merge_bases_many_0(): pass on errors from `merge_bases_many()`
repo_get_merge_bases(): pass on errors from `merge_bases_many()`
get_octopus_merge_bases(): pass on errors from `merge_bases_many()`
repo_get_merge_bases_many(): pass on errors from `merge_bases_many()`
repo_get_merge_bases_many_dirty(): pass on errors from
`merge_bases_many()`
paint_down_to_common(): special-case shallow/partial clones
bisect.c | 7 +-
builtin/branch.c | 12 +-
builtin/fast-import.c | 6 +-
builtin/fetch.c | 2 +
builtin/log.c | 30 +++--
builtin/merge-base.c | 18 ++-
builtin/merge-tree.c | 5 +-
builtin/merge.c | 26 ++--
builtin/pull.c | 9 +-
builtin/rebase.c | 8 +-
builtin/receive-pack.c | 6 +-
builtin/rev-parse.c | 5 +-
commit-reach.c | 202 +++++++++++++++++++------------
commit-reach.h | 26 ++--
commit.c | 7 +-
diff-lib.c | 5 +-
http-push.c | 5 +-
log-tree.c | 5 +-
merge-ort.c | 81 +++++++++++--
merge-recursive.c | 52 ++++++--
notes-merge.c | 3 +-
object-name.c | 5 +-
remote.c | 2 +-
revision.c | 10 +-
sequencer.c | 8 +-
shallow.c | 21 ++--
submodule.c | 7 +-
t/helper/test-reach.c | 11 +-
t/t4301-merge-tree-write-tree.sh | 12 ++
29 files changed, 413 insertions(+), 183 deletions(-)
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1657%2Fdscho%2Fmerge-base-and-missing-objects-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1657/dscho/merge-base-and-missing-objects-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1657
--
gitgitgadget
^ permalink raw reply
* [PATCH 02/12] Let `repo_in_merge_bases()` report missing commits
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Some functions in Git's source code follow the convention that returning
a negative value indicates a fatal error, e.g. repository corruption.
Let's use this convention in `repo_in_merge_bases()` to report when one
of the specified commits is missing (i.e. when `repo_parse_commit()`
reports an error).
Also adjust the callers of `repo_in_merge_bases()` to handle such
negative return values.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/branch.c | 12 +++++--
builtin/fast-import.c | 6 +++-
builtin/fetch.c | 2 ++
builtin/log.c | 7 ++--
builtin/merge-base.c | 6 +++-
builtin/pull.c | 4 +++
builtin/receive-pack.c | 6 +++-
commit-reach.c | 12 ++++---
http-push.c | 5 ++-
merge-ort.c | 75 ++++++++++++++++++++++++++++++++++++------
merge-recursive.c | 48 ++++++++++++++++++++++-----
shallow.c | 18 ++++++----
12 files changed, 163 insertions(+), 38 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index e7ee9bd0f15..7f9e79237f3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -161,6 +161,8 @@ static int branch_merged(int kind, const char *name,
merged = reference_rev ? repo_in_merge_bases(the_repository, rev,
reference_rev) : 0;
+ if (merged < 0)
+ exit(128);
/*
* After the safety valve is fully redefined to "check with
@@ -169,9 +171,13 @@ static int branch_merged(int kind, const char *name,
* any of the following code, but during the transition period,
* a gentle reminder is in order.
*/
- if ((head_rev != reference_rev) &&
- (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
- if (merged)
+ if (head_rev != reference_rev) {
+ int expect = head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0;
+ if (expect < 0)
+ exit(128);
+ if (expect == merged)
+ ; /* okay */
+ else if (merged)
warning(_("deleting branch '%s' that has been merged to\n"
" '%s', but not yet merged to HEAD"),
name, reference_name);
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 444f41cf8ca..14c2efa88fc 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1625,6 +1625,7 @@ static int update_branch(struct branch *b)
oidclr(&old_oid);
if (!force_update && !is_null_oid(&old_oid)) {
struct commit *old_cmit, *new_cmit;
+ int ret;
old_cmit = lookup_commit_reference_gently(the_repository,
&old_oid, 0);
@@ -1633,7 +1634,10 @@ static int update_branch(struct branch *b)
if (!old_cmit || !new_cmit)
return error("Branch %s is missing commits.", b->name);
- if (!repo_in_merge_bases(the_repository, old_cmit, new_cmit)) {
+ ret = repo_in_merge_bases(the_repository, old_cmit, new_cmit);
+ if (ret < 0)
+ exit(128);
+ if (!ret) {
warning("Not updating %s"
" (new tip %s does not contain %s)",
b->name, oid_to_hex(&b->oid),
diff --git a/builtin/fetch.c b/builtin/fetch.c
index fd134ba74d9..0584a1f8b64 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -978,6 +978,8 @@ static int update_local_ref(struct ref *ref,
uint64_t t_before = getnanotime();
fast_forward = repo_in_merge_bases(the_repository, current,
updated);
+ if (fast_forward < 0)
+ exit(128);
forced_updates_ms += (getnanotime() - t_before) / 1000000;
} else {
fast_forward = 1;
diff --git a/builtin/log.c b/builtin/log.c
index ba775d7b5cf..1705da71aca 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1623,7 +1623,7 @@ static struct commit *get_base_commit(const char *base_commit,
{
struct commit *base = NULL;
struct commit **rev;
- int i = 0, rev_nr = 0, auto_select, die_on_failure;
+ int i = 0, rev_nr = 0, auto_select, die_on_failure, ret;
switch (auto_base) {
case AUTO_BASE_NEVER:
@@ -1723,7 +1723,10 @@ static struct commit *get_base_commit(const char *base_commit,
rev_nr = DIV_ROUND_UP(rev_nr, 2);
}
- if (!repo_in_merge_bases(the_repository, base, rev[0])) {
+ ret = repo_in_merge_bases(the_repository, base, rev[0]);
+ if (ret < 0)
+ exit(128);
+ if (!ret) {
if (die_on_failure) {
die(_("base commit should be the ancestor of revision list"));
} else {
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index e68b7fe45d7..0308fd73289 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -103,12 +103,16 @@ static int handle_octopus(int count, const char **args, int show_all)
static int handle_is_ancestor(int argc, const char **argv)
{
struct commit *one, *two;
+ int ret;
if (argc != 2)
die("--is-ancestor takes exactly two commits");
one = get_commit_reference(argv[0]);
two = get_commit_reference(argv[1]);
- if (repo_in_merge_bases(the_repository, one, two))
+ ret = repo_in_merge_bases(the_repository, one, two);
+ if (ret < 0)
+ exit(128);
+ if (ret)
return 0;
else
return 1;
diff --git a/builtin/pull.c b/builtin/pull.c
index be2b2c9ebc9..e6f2942c0c5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -931,6 +931,8 @@ static int get_can_ff(struct object_id *orig_head,
merge_head = lookup_commit_reference(the_repository, orig_merge_head);
ret = repo_is_descendant_of(the_repository, merge_head, list);
free_commit_list(list);
+ if (ret < 0)
+ exit(128);
return ret;
}
@@ -955,6 +957,8 @@ static int already_up_to_date(struct object_id *orig_head,
commit_list_insert(theirs, &list);
ok = repo_is_descendant_of(the_repository, ours, list);
free_commit_list(list);
+ if (ok < 0)
+ exit(128);
if (!ok)
return 0;
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8c4f0cb90a9..956fea6293e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1546,6 +1546,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
starts_with(name, "refs/heads/")) {
struct object *old_object, *new_object;
struct commit *old_commit, *new_commit;
+ int ret2;
old_object = parse_object(the_repository, old_oid);
new_object = parse_object(the_repository, new_oid);
@@ -1559,7 +1560,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}
old_commit = (struct commit *)old_object;
new_commit = (struct commit *)new_object;
- if (!repo_in_merge_bases(the_repository, old_commit, new_commit)) {
+ ret2 = repo_in_merge_bases(the_repository, old_commit, new_commit);
+ if (ret2 < 0)
+ exit(128);
+ if (!ret2) {
rp_error("denying non-fast-forward %s"
" (you should pull first)", name);
ret = "non-fast-forward";
diff --git a/commit-reach.c b/commit-reach.c
index b2b102926c9..dab32eb470d 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -463,11 +463,13 @@ int repo_is_descendant_of(struct repository *r,
} else {
while (with_commit) {
struct commit *other;
+ int ret;
other = with_commit->item;
with_commit = with_commit->next;
- if (repo_in_merge_bases_many(r, other, 1, &commit))
- return 1;
+ ret = repo_in_merge_bases_many(r, other, 1, &commit);
+ if (ret)
+ return ret;
}
return 0;
}
@@ -484,10 +486,10 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
timestamp_t generation, max_generation = GENERATION_NUMBER_ZERO;
if (repo_parse_commit(r, commit))
- return ret;
+ return -1;
for (i = 0; i < nr_reference; i++) {
if (repo_parse_commit(r, reference[i]))
- return ret;
+ return -1;
generation = commit_graph_generation(reference[i]);
if (generation > max_generation)
@@ -596,6 +598,8 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)
commit_list_insert(old_commit, &old_commit_list);
ret = repo_is_descendant_of(the_repository,
new_commit, old_commit_list);
+ if (ret < 0)
+ exit(128);
free_commit_list(old_commit_list);
return ret;
}
diff --git a/http-push.c b/http-push.c
index a704f490fdb..85fa2f457d4 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1576,8 +1576,11 @@ static int verify_merge_base(struct object_id *head_oid, struct ref *remote)
struct commit *head = lookup_commit_or_die(head_oid, "HEAD");
struct commit *branch = lookup_commit_or_die(&remote->old_oid,
remote->name);
+ int i = repo_in_merge_bases(the_repository, branch, head);
- return repo_in_merge_bases(the_repository, branch, head);
+ if (i < 0)
+ exit(128);
+ return i;
}
static int delete_remote_branch(const char *pattern, int force)
diff --git a/merge-ort.c b/merge-ort.c
index 6491070d965..64e76afe89f 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -544,6 +544,7 @@ enum conflict_and_info_types {
CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,
CONFLICT_SUBMODULE_MAY_HAVE_REWINDS,
CONFLICT_SUBMODULE_NULL_MERGE_BASE,
+ CONFLICT_SUBMODULE_CORRUPT,
/* Keep this entry _last_ in the list */
NB_CONFLICT_TYPES,
@@ -596,7 +597,9 @@ static const char *type_short_descriptions[] = {
[CONFLICT_SUBMODULE_MAY_HAVE_REWINDS] =
"CONFLICT (submodule may have rewinds)",
[CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
- "CONFLICT (submodule lacks merge base)"
+ "CONFLICT (submodule lacks merge base)",
+ [CONFLICT_SUBMODULE_CORRUPT] =
+ "CONFLICT (submodule corrupt)"
};
struct logical_conflict_info {
@@ -1710,7 +1713,11 @@ static int find_first_merges(struct repository *repo,
die("revision walk setup failed");
while ((commit = get_revision(&revs)) != NULL) {
struct object *o = &(commit->object);
- if (repo_in_merge_bases(repo, b, commit))
+ int ret = repo_in_merge_bases(repo, b, commit);
+
+ if (ret < 0)
+ return ret;
+ if (ret > 0)
add_object_array(o, NULL, &merges);
}
reset_revision_walk();
@@ -1725,9 +1732,14 @@ static int find_first_merges(struct repository *repo,
contains_another = 0;
for (j = 0; j < merges.nr; j++) {
struct commit *m2 = (struct commit *) merges.objects[j].item;
- if (i != j && repo_in_merge_bases(repo, m2, m1)) {
- contains_another = 1;
- break;
+ if (i != j) {
+ int ret = repo_in_merge_bases(repo, m2, m1);
+ if (ret < 0)
+ return ret;
+ if (ret > 0) {
+ contains_another = 1;
+ break;
+ }
}
}
@@ -1749,7 +1761,7 @@ static int merge_submodule(struct merge_options *opt,
{
struct repository subrepo;
struct strbuf sb = STRBUF_INIT;
- int ret = 0;
+ int ret = 0, ret2;
struct commit *commit_o, *commit_a, *commit_b;
int parent_count;
struct object_array merges;
@@ -1796,8 +1808,26 @@ static int merge_submodule(struct merge_options *opt,
}
/* check whether both changes are forward */
- if (!repo_in_merge_bases(&subrepo, commit_o, commit_a) ||
- !repo_in_merge_bases(&subrepo, commit_o, commit_b)) {
+ ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a);
+ if (ret2 < 1) {
+ path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+ path, NULL, NULL, NULL,
+ _("Failed to merge submodule %s "
+ "(repository corrupt)"),
+ path);
+ goto cleanup;
+ }
+ if (!ret2)
+ ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b);
+ if (ret2 < 1) {
+ path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+ path, NULL, NULL, NULL,
+ _("Failed to merge submodule %s "
+ "(repository corrupt)"),
+ path);
+ goto cleanup;
+ }
+ if (!ret2) {
path_msg(opt, CONFLICT_SUBMODULE_MAY_HAVE_REWINDS, 0,
path, NULL, NULL, NULL,
_("Failed to merge submodule %s "
@@ -1807,7 +1837,16 @@ static int merge_submodule(struct merge_options *opt,
}
/* Case #1: a is contained in b or vice versa */
- if (repo_in_merge_bases(&subrepo, commit_a, commit_b)) {
+ ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
+ if (ret2 < 0) {
+ path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+ path, NULL, NULL, NULL,
+ _("Failed to merge submodule %s "
+ "(repository corrupt)"),
+ path);
+ goto cleanup;
+ }
+ if (ret2 > 0) {
oidcpy(result, b);
path_msg(opt, INFO_SUBMODULE_FAST_FORWARDING, 1,
path, NULL, NULL, NULL,
@@ -1816,7 +1855,16 @@ static int merge_submodule(struct merge_options *opt,
ret = 1;
goto cleanup;
}
- if (repo_in_merge_bases(&subrepo, commit_b, commit_a)) {
+ ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
+ if (ret2 < 0) {
+ path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+ path, NULL, NULL, NULL,
+ _("Failed to merge submodule %s "
+ "(repository corrupt)"),
+ path);
+ goto cleanup;
+ }
+ if (ret2 > 0) {
oidcpy(result, a);
path_msg(opt, INFO_SUBMODULE_FAST_FORWARDING, 1,
path, NULL, NULL, NULL,
@@ -1841,6 +1889,13 @@ static int merge_submodule(struct merge_options *opt,
parent_count = find_first_merges(&subrepo, path, commit_a, commit_b,
&merges);
switch (parent_count) {
+ case -1:
+ path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+ path, NULL, NULL, NULL,
+ _("Failed to merge submodule %s "
+ "(repository corrupt)"),
+ path);
+ break;
case 0:
path_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0,
path, NULL, NULL, NULL,
diff --git a/merge-recursive.c b/merge-recursive.c
index e3beb0801b1..e3fe7803cbe 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1144,7 +1144,10 @@ static int find_first_merges(struct repository *repo,
die("revision walk setup failed");
while ((commit = get_revision(&revs)) != NULL) {
struct object *o = &(commit->object);
- if (repo_in_merge_bases(repo, b, commit))
+ int ret = repo_in_merge_bases(repo, b, commit);
+ if (ret < 0)
+ return ret;
+ if (ret)
add_object_array(o, NULL, &merges);
}
reset_revision_walk();
@@ -1159,9 +1162,14 @@ static int find_first_merges(struct repository *repo,
contains_another = 0;
for (j = 0; j < merges.nr; j++) {
struct commit *m2 = (struct commit *) merges.objects[j].item;
- if (i != j && repo_in_merge_bases(repo, m2, m1)) {
- contains_another = 1;
- break;
+ if (i != j) {
+ int ret = repo_in_merge_bases(repo, m2, m1);
+ if (ret < 0)
+ return ret;
+ if (ret > 0) {
+ contains_another = 1;
+ break;
+ }
}
}
@@ -1197,7 +1205,7 @@ static int merge_submodule(struct merge_options *opt,
const struct object_id *b)
{
struct repository subrepo;
- int ret = 0;
+ int ret = 0, ret2;
struct commit *commit_base, *commit_a, *commit_b;
int parent_count;
struct object_array merges;
@@ -1234,14 +1242,29 @@ static int merge_submodule(struct merge_options *opt,
}
/* check whether both changes are forward */
- if (!repo_in_merge_bases(&subrepo, commit_base, commit_a) ||
- !repo_in_merge_bases(&subrepo, commit_base, commit_b)) {
+ ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a);
+ if (ret2 < 0) {
+ output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ goto cleanup;
+ }
+ if (ret2)
+ ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b);
+ if (ret2 < 0) {
+ output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ goto cleanup;
+ }
+ if (!ret2) {
output(opt, 1, _("Failed to merge submodule %s (commits don't follow merge-base)"), path);
goto cleanup;
}
/* Case #1: a is contained in b or vice versa */
- if (repo_in_merge_bases(&subrepo, commit_a, commit_b)) {
+ ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
+ if (ret2 < 0) {
+ output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ goto cleanup;
+ }
+ if (ret2) {
oidcpy(result, b);
if (show(opt, 3)) {
output(opt, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
@@ -1254,7 +1277,12 @@ static int merge_submodule(struct merge_options *opt,
ret = 1;
goto cleanup;
}
- if (repo_in_merge_bases(&subrepo, commit_b, commit_a)) {
+ ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
+ if (ret2 < 0) {
+ output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ goto cleanup;
+ }
+ if (ret2) {
oidcpy(result, a);
if (show(opt, 3)) {
output(opt, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
@@ -1402,6 +1430,8 @@ static int merge_mode_and_contents(struct merge_options *opt,
&o->oid,
&a->oid,
&b->oid);
+ if (result->clean < 0)
+ return -1;
} else if (S_ISLNK(a->mode)) {
switch (opt->recursive_variant) {
case MERGE_VARIANT_NORMAL:
diff --git a/shallow.c b/shallow.c
index ac728cdd778..cf4b95114b7 100644
--- a/shallow.c
+++ b/shallow.c
@@ -795,12 +795,16 @@ static void post_assign_shallow(struct shallow_info *info,
if (!*bitmap)
continue;
for (j = 0; j < bitmap_nr; j++)
- if (bitmap[0][j] &&
- /* Step 7, reachability test at commit level */
- !repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits)) {
- update_refstatus(ref_status, info->ref->nr, *bitmap);
- dst++;
- break;
+ if (bitmap[0][j]) {
+ /* Step 7, reachability test at commit level */
+ int ret = repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits);
+ if (ret < 0)
+ exit(128);
+ if (!ret) {
+ update_refstatus(ref_status, info->ref->nr, *bitmap);
+ dst++;
+ break;
+ }
}
}
info->nr_ours = dst;
@@ -829,6 +833,8 @@ int delayed_reachability_test(struct shallow_info *si, int c)
commit,
si->nr_commits,
si->commits);
+ if (si->reachable[c] < 0)
+ exit(128);
si->need_reachability_test[c] = 0;
}
return si->reachable[c];
--
gitgitgadget
^ permalink raw reply related
* [PATCH 03/12] Prepare `repo_in_merge_bases_many()` to optionally expect missing commits
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Currently this function treats unrelated commit histories the same way
as commit histories with missing commit objects.
Typically, missing commit objects constitute a corrupt repository,
though, and should be reported as such. The next commits will make it
so, but there is one exception: In `git fetch --update-shallow` we
_expect_ commit objects to be missing, and we do want to treat the
now-incomplete commit histories as unrelated.
To allow for that, let's introduce an additional parameter that is
passed to `repo_in_merge_bases_many()` to trigger this behavior, and use
it in the two callers in `shallow.c`.
This does not change behavior in this commit, but prepares in an
easy-to-review way for the upcoming changes that will make the merge
base logic more stringent with regards to missing commit objects.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 9 +++++----
commit-reach.h | 3 ++-
remote.c | 2 +-
shallow.c | 5 +++--
t/helper/test-reach.c | 2 +-
5 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index dab32eb470d..248a0f2b39d 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -467,7 +467,7 @@ int repo_is_descendant_of(struct repository *r,
other = with_commit->item;
with_commit = with_commit->next;
- ret = repo_in_merge_bases_many(r, other, 1, &commit);
+ ret = repo_in_merge_bases_many(r, other, 1, &commit, 0);
if (ret)
return ret;
}
@@ -479,17 +479,18 @@ int repo_is_descendant_of(struct repository *r,
* Is "commit" an ancestor of one of the "references"?
*/
int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
- int nr_reference, struct commit **reference)
+ int nr_reference, struct commit **reference,
+ int ignore_missing_commits)
{
struct commit_list *bases;
int ret = 0, i;
timestamp_t generation, max_generation = GENERATION_NUMBER_ZERO;
if (repo_parse_commit(r, commit))
- return -1;
+ return ignore_missing_commits ? 0 : -1;
for (i = 0; i < nr_reference; i++) {
if (repo_parse_commit(r, reference[i]))
- return -1;
+ return ignore_missing_commits ? 0 : -1;
generation = commit_graph_generation(reference[i]);
if (generation > max_generation)
diff --git a/commit-reach.h b/commit-reach.h
index 35c4da49481..68f81549a44 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -30,7 +30,8 @@ int repo_in_merge_bases(struct repository *r,
struct commit *reference);
int repo_in_merge_bases_many(struct repository *r,
struct commit *commit,
- int nr_reference, struct commit **reference);
+ int nr_reference, struct commit **reference,
+ int ignore_missing_commits);
/*
* Takes a list of commits and returns a new list where those
diff --git a/remote.c b/remote.c
index abb24822beb..763c80f4a7d 100644
--- a/remote.c
+++ b/remote.c
@@ -2675,7 +2675,7 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)
if (MERGE_BASES_BATCH_SIZE < size)
size = MERGE_BASES_BATCH_SIZE;
- if ((ret = repo_in_merge_bases_many(the_repository, commit, size, chunk)))
+ if ((ret = repo_in_merge_bases_many(the_repository, commit, size, chunk, 0)))
break;
}
diff --git a/shallow.c b/shallow.c
index cf4b95114b7..f71496f35c3 100644
--- a/shallow.c
+++ b/shallow.c
@@ -797,7 +797,7 @@ static void post_assign_shallow(struct shallow_info *info,
for (j = 0; j < bitmap_nr; j++)
if (bitmap[0][j]) {
/* Step 7, reachability test at commit level */
- int ret = repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits);
+ int ret = repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits, 1);
if (ret < 0)
exit(128);
if (!ret) {
@@ -832,7 +832,8 @@ int delayed_reachability_test(struct shallow_info *si, int c)
si->reachable[c] = repo_in_merge_bases_many(the_repository,
commit,
si->nr_commits,
- si->commits);
+ si->commits,
+ 1);
if (si->reachable[c] < 0)
exit(128);
si->need_reachability_test[c] = 0;
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 3e173399a00..aa816e168ea 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -113,7 +113,7 @@ int cmd__reach(int ac, const char **av)
repo_in_merge_bases(the_repository, A, B));
else if (!strcmp(av[1], "in_merge_bases_many"))
printf("%s(A,X):%d\n", av[1],
- repo_in_merge_bases_many(the_repository, A, X_nr, X_array));
+ repo_in_merge_bases_many(the_repository, A, X_nr, X_array, 0));
else if (!strcmp(av[1], "is_descendant_of"))
printf("%s(A,X):%d\n", av[1], repo_is_descendant_of(r, A, X));
else if (!strcmp(av[1], "get_merge_bases_many")) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH 04/12] Prepare `paint_down_to_common()` for handling shallow commits
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `git fetch --update-shallow` needs to test for commit ancestry, it
can naturally run into a missing object (e.g. if it is a parent of a
shallow commit). To accommodate, the merge base logic will need to be
able to handle this situation gracefully.
Currently, that logic pretends that a missing parent commit is
equivalent to a a missing parent commit, and for the purpose of
`--update-shallow` that is exactly what we need it to do.
Therefore, let's introduce a flag to indicate when that is precisely the
logic we want.
We need a flag, and cannot rely on `is_repository_shallow()` to indicate
that situation, because that function would return 0: There may not
actually be a `shallow` file, as demonstrated e.g. by t5537.10 ("add new
shallow root with receive.updateshallow on") and t5538.4 ("add new
shallow root with receive.updateshallow on").
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 248a0f2b39d..1d1d8c989de 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -53,7 +53,8 @@ static int queue_has_nonstale(struct prio_queue *queue)
static struct commit_list *paint_down_to_common(struct repository *r,
struct commit *one, int n,
struct commit **twos,
- timestamp_t min_generation)
+ timestamp_t min_generation,
+ int ignore_missing_commits)
{
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
struct commit_list *result = NULL;
@@ -106,6 +107,13 @@ static struct commit_list *paint_down_to_common(struct repository *r,
if ((p->object.flags & flags) == flags)
continue;
if (repo_parse_commit(r, p)) {
+ /*
+ * At this stage, we know that the commit is
+ * missing: `repo_parse_commit()` uses
+ * `OBJECT_INFO_DIE_IF_CORRUPT` and therefore
+ * corrupt commits would already have been
+ * dispatched with a `die()`.
+ */
free_commit_list(result);
return NULL;
}
@@ -142,7 +150,7 @@ static struct commit_list *merge_bases_many(struct repository *r,
return NULL;
}
- list = paint_down_to_common(r, one, n, twos, 0);
+ list = paint_down_to_common(r, one, n, twos, 0, 0);
while (list) {
struct commit *commit = pop_commit(&list);
@@ -213,7 +221,7 @@ static int remove_redundant_no_gen(struct repository *r,
min_generation = curr_generation;
}
common = paint_down_to_common(r, array[i], filled,
- work, min_generation);
+ work, min_generation, 0);
if (array[i]->object.flags & PARENT2)
redundant[i] = 1;
for (j = 0; j < filled; j++)
@@ -503,7 +511,7 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
bases = paint_down_to_common(r, commit,
nr_reference, reference,
- generation);
+ generation, ignore_missing_commits);
if (commit->object.flags & PARENT2)
ret = 1;
clear_commit_marks(commit, all_flags);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 05/12] commit-reach: start reporting errors in `paint_down_to_common()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
If a commit cannot be parsed, it is currently ignored when looking for
merge bases. That's undesirable as the operation can pretend success in
a corrupt repository, even though the command should fail with an error
message.
Let's start at the bottom of the stack by teaching the
`paint_down_to_common()` function to return an `int`: if negative, it
indicates fatal error, if 0 success.
This requires a couple of callers to be adjusted accordingly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 56 +++++++++++++++++++++++++++++++-------------------
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 1d1d8c989de..dafe117036b 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -50,14 +50,14 @@ static int queue_has_nonstale(struct prio_queue *queue)
}
/* all input commits in one and twos[] must have been parsed! */
-static struct commit_list *paint_down_to_common(struct repository *r,
- struct commit *one, int n,
- struct commit **twos,
- timestamp_t min_generation,
- int ignore_missing_commits)
+static int paint_down_to_common(struct repository *r,
+ struct commit *one, int n,
+ struct commit **twos,
+ timestamp_t min_generation,
+ int ignore_missing_commits,
+ struct commit_list **result)
{
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
- struct commit_list *result = NULL;
int i;
timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
@@ -66,8 +66,8 @@ static struct commit_list *paint_down_to_common(struct repository *r,
one->object.flags |= PARENT1;
if (!n) {
- commit_list_append(one, &result);
- return result;
+ commit_list_append(one, result);
+ return 0;
}
prio_queue_put(&queue, one);
@@ -95,7 +95,7 @@ static struct commit_list *paint_down_to_common(struct repository *r,
if (flags == (PARENT1 | PARENT2)) {
if (!(commit->object.flags & RESULT)) {
commit->object.flags |= RESULT;
- commit_list_insert_by_date(commit, &result);
+ commit_list_insert_by_date(commit, result);
}
/* Mark parents of a found merge stale */
flags |= STALE;
@@ -114,8 +114,11 @@ static struct commit_list *paint_down_to_common(struct repository *r,
* corrupt commits would already have been
* dispatched with a `die()`.
*/
- free_commit_list(result);
- return NULL;
+ free_commit_list(*result);
+ if (ignore_missing_commits)
+ return 0;
+ return error(_("could not parse commit %s"),
+ oid_to_hex(&p->object.oid));
}
p->object.flags |= flags;
prio_queue_put(&queue, p);
@@ -123,7 +126,7 @@ static struct commit_list *paint_down_to_common(struct repository *r,
}
clear_prio_queue(&queue);
- return result;
+ return 0;
}
static struct commit_list *merge_bases_many(struct repository *r,
@@ -150,7 +153,8 @@ static struct commit_list *merge_bases_many(struct repository *r,
return NULL;
}
- list = paint_down_to_common(r, one, n, twos, 0, 0);
+ if (paint_down_to_common(r, one, n, twos, 0, 0, &list) < 0)
+ return NULL;
while (list) {
struct commit *commit = pop_commit(&list);
@@ -204,7 +208,7 @@ static int remove_redundant_no_gen(struct repository *r,
for (i = 0; i < cnt; i++)
repo_parse_commit(r, array[i]);
for (i = 0; i < cnt; i++) {
- struct commit_list *common;
+ struct commit_list *common = NULL;
timestamp_t min_generation = commit_graph_generation(array[i]);
if (redundant[i])
@@ -220,8 +224,9 @@ static int remove_redundant_no_gen(struct repository *r,
if (curr_generation < min_generation)
min_generation = curr_generation;
}
- common = paint_down_to_common(r, array[i], filled,
- work, min_generation, 0);
+ if (paint_down_to_common(r, array[i], filled,
+ work, min_generation, 0, &common))
+ return -1;
if (array[i]->object.flags & PARENT2)
redundant[i] = 1;
for (j = 0; j < filled; j++)
@@ -421,6 +426,10 @@ static struct commit_list *get_merge_bases_many_0(struct repository *r,
clear_commit_marks_many(n, twos, all_flags);
cnt = remove_redundant(r, rslt, cnt);
+ if (cnt < 0) {
+ free(rslt);
+ return NULL;
+ }
result = NULL;
for (i = 0; i < cnt; i++)
commit_list_insert_by_date(rslt[i], &result);
@@ -490,7 +499,7 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
int nr_reference, struct commit **reference,
int ignore_missing_commits)
{
- struct commit_list *bases;
+ struct commit_list *bases = NULL;
int ret = 0, i;
timestamp_t generation, max_generation = GENERATION_NUMBER_ZERO;
@@ -509,10 +518,11 @@ int repo_in_merge_bases_many(struct repository *r, struct commit *commit,
if (generation > max_generation)
return ret;
- bases = paint_down_to_common(r, commit,
- nr_reference, reference,
- generation, ignore_missing_commits);
- if (commit->object.flags & PARENT2)
+ if (paint_down_to_common(r, commit,
+ nr_reference, reference,
+ generation, ignore_missing_commits, &bases))
+ ret = -1;
+ else if (commit->object.flags & PARENT2)
ret = 1;
clear_commit_marks(commit, all_flags);
clear_commit_marks_many(nr_reference, reference, all_flags);
@@ -565,6 +575,10 @@ struct commit_list *reduce_heads(struct commit_list *heads)
}
}
num_head = remove_redundant(the_repository, array, num_head);
+ if (num_head < 0) {
+ free(array);
+ return NULL;
+ }
for (i = 0; i < num_head; i++)
tail = &commit_list_insert(array[i], tail)->next;
free(array);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 06/12] merge_bases_many(): pass on errors from `paint_down_to_common()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `paint_down_to_common()` function was just taught to indicate
parsing errors, and now the `merge_bases_many()` function is aware of
that, too.
One tricky aspect is that `merge_bases_many()` parses commits of its
own, but wants to gracefully handle the scenario where NULL is passed as
a merge head, returning the empty list of merge bases. The way this was
handled involved calling `repo_parse_commit(NULL)` and relying on it to
return an error. This has to be done differently now so that we can
handle missing commits correctly by producing a fatal error.
Next step: adjust the caller of `merge_bases_many()`:
`get_merge_bases_many_0()`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index dafe117036b..c9969da8c6c 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -129,39 +129,47 @@ static int paint_down_to_common(struct repository *r,
return 0;
}
-static struct commit_list *merge_bases_many(struct repository *r,
- struct commit *one, int n,
- struct commit **twos)
+static int merge_bases_many(struct repository *r,
+ struct commit *one, int n,
+ struct commit **twos,
+ struct commit_list **result)
{
struct commit_list *list = NULL;
- struct commit_list *result = NULL;
int i;
for (i = 0; i < n; i++) {
- if (one == twos[i])
+ if (one == twos[i]) {
/*
* We do not mark this even with RESULT so we do not
* have to clean it up.
*/
- return commit_list_insert(one, &result);
+ *result = commit_list_insert(one, result);
+ return 0;
+ }
}
+ if (!one)
+ return 0;
if (repo_parse_commit(r, one))
- return NULL;
+ return error(_("could not parse commit %s"),
+ oid_to_hex(&one->object.oid));
for (i = 0; i < n; i++) {
+ if (!twos[i])
+ return 0;
if (repo_parse_commit(r, twos[i]))
- return NULL;
+ return error(_("could not parse commit %s"),
+ oid_to_hex(&twos[i]->object.oid));
}
if (paint_down_to_common(r, one, n, twos, 0, 0, &list) < 0)
- return NULL;
+ return -1;
while (list) {
struct commit *commit = pop_commit(&list);
if (!(commit->object.flags & STALE))
- commit_list_insert_by_date(commit, &result);
+ commit_list_insert_by_date(commit, result);
}
- return result;
+ return 0;
}
struct commit_list *get_octopus_merge_bases(struct commit_list *in)
@@ -399,10 +407,11 @@ static struct commit_list *get_merge_bases_many_0(struct repository *r,
{
struct commit_list *list;
struct commit **rslt;
- struct commit_list *result;
+ struct commit_list *result = NULL;
int cnt, i;
- result = merge_bases_many(r, one, n, twos);
+ if (merge_bases_many(r, one, n, twos, &result) < 0)
+ return NULL;
for (i = 0; i < n; i++) {
if (one == twos[i])
return result;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 07/12] get_merge_bases_many_0(): pass on errors from `merge_bases_many()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `merge_bases_many()` function was just taught to indicate
parsing errors, and now the `get_merge_bases_many_0()` function is aware
of that, too.
Next step: adjust the callers of `get_merge_bases_many_0()`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 57 +++++++++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 21 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index c9969da8c6c..359853275a9 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -399,37 +399,38 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt
return remove_redundant_no_gen(r, array, cnt);
}
-static struct commit_list *get_merge_bases_many_0(struct repository *r,
- struct commit *one,
- int n,
- struct commit **twos,
- int cleanup)
+static int get_merge_bases_many_0(struct repository *r,
+ struct commit *one,
+ int n,
+ struct commit **twos,
+ int cleanup,
+ struct commit_list **result)
{
struct commit_list *list;
struct commit **rslt;
- struct commit_list *result = NULL;
int cnt, i;
- if (merge_bases_many(r, one, n, twos, &result) < 0)
- return NULL;
+ if (merge_bases_many(r, one, n, twos, result) < 0)
+ return -1;
for (i = 0; i < n; i++) {
if (one == twos[i])
- return result;
+ return 0;
}
- if (!result || !result->next) {
+ if (!*result || !(*result)->next) {
if (cleanup) {
clear_commit_marks(one, all_flags);
clear_commit_marks_many(n, twos, all_flags);
}
- return result;
+ return 0;
}
/* There are more than one */
- cnt = commit_list_count(result);
+ cnt = commit_list_count(*result);
CALLOC_ARRAY(rslt, cnt);
- for (list = result, i = 0; list; list = list->next)
+ for (list = *result, i = 0; list; list = list->next)
rslt[i++] = list->item;
- free_commit_list(result);
+ free_commit_list(*result);
+ *result = NULL;
clear_commit_marks(one, all_flags);
clear_commit_marks_many(n, twos, all_flags);
@@ -437,13 +438,12 @@ static struct commit_list *get_merge_bases_many_0(struct repository *r,
cnt = remove_redundant(r, rslt, cnt);
if (cnt < 0) {
free(rslt);
- return NULL;
+ return -1;
}
- result = NULL;
for (i = 0; i < cnt; i++)
- commit_list_insert_by_date(rslt[i], &result);
+ commit_list_insert_by_date(rslt[i], result);
free(rslt);
- return result;
+ return 0;
}
struct commit_list *repo_get_merge_bases_many(struct repository *r,
@@ -451,7 +451,12 @@ struct commit_list *repo_get_merge_bases_many(struct repository *r,
int n,
struct commit **twos)
{
- return get_merge_bases_many_0(r, one, n, twos, 1);
+ struct commit_list *result = NULL;
+ if (get_merge_bases_many_0(r, one, n, twos, 1, &result) < 0) {
+ free_commit_list(result);
+ return NULL;
+ }
+ return result;
}
struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
@@ -459,14 +464,24 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
int n,
struct commit **twos)
{
- return get_merge_bases_many_0(r, one, n, twos, 0);
+ struct commit_list *result = NULL;
+ if (get_merge_bases_many_0(r, one, n, twos, 0, &result) < 0) {
+ free_commit_list(result);
+ return NULL;
+ }
+ return result;
}
struct commit_list *repo_get_merge_bases(struct repository *r,
struct commit *one,
struct commit *two)
{
- return get_merge_bases_many_0(r, one, 1, &two, 1);
+ struct commit_list *result = NULL;
+ if (get_merge_bases_many_0(r, one, 1, &two, 1, &result) < 0) {
+ free_commit_list(result);
+ return NULL;
+ }
+ return result;
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH 08/12] repo_get_merge_bases(): pass on errors from `merge_bases_many()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `merge_bases_many()` function was just taught to indicate parsing
errors, and now the `repo_get_merge_bases()` function (which is also
surfaced via the `repo_get_merge_bases()` macro) is aware of that, too.
Naturally, there are a lot of callers that need to be adjusted now, too.
Next step: adjust the callers of `get_octopus_merge_bases()`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 10 +++++-----
builtin/merge-tree.c | 5 +++--
builtin/merge.c | 20 ++++++++++++--------
builtin/rebase.c | 8 +++++---
builtin/rev-parse.c | 5 +++--
commit-reach.c | 23 +++++++++++------------
commit-reach.h | 7 ++++---
diff-lib.c | 5 +++--
log-tree.c | 5 +++--
merge-ort.c | 6 +++++-
merge-recursive.c | 4 +++-
notes-merge.c | 3 ++-
object-name.c | 5 +++--
revision.c | 10 ++++++----
sequencer.c | 8 ++++++--
submodule.c | 7 ++++++-
t/t4301-merge-tree-write-tree.sh | 12 ++++++++++++
17 files changed, 92 insertions(+), 51 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 1705da71aca..befafd6ae04 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1702,11 +1702,11 @@ static struct commit *get_base_commit(const char *base_commit,
*/
while (rev_nr > 1) {
for (i = 0; i < rev_nr / 2; i++) {
- struct commit_list *merge_base;
- merge_base = repo_get_merge_bases(the_repository,
- rev[2 * i],
- rev[2 * i + 1]);
- if (!merge_base || merge_base->next) {
+ struct commit_list *merge_base = NULL;
+ if (repo_get_merge_bases(the_repository,
+ rev[2 * i],
+ rev[2 * i + 1], &merge_base) < 0 ||
+ !merge_base || merge_base->next) {
if (die_on_failure) {
die(_("failed to find exact merge base"));
} else {
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index a35e0452d66..76200250629 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -463,8 +463,9 @@ static int real_merge(struct merge_tree_options *o,
* Get the merge bases, in reverse order; see comment above
* merge_incore_recursive in merge-ort.h
*/
- merge_bases = repo_get_merge_bases(the_repository, parent1,
- parent2);
+ if (repo_get_merge_bases(the_repository, parent1,
+ parent2, &merge_bases) < 0)
+ exit(128);
if (!merge_bases && !o->allow_unrelated_histories)
die(_("refusing to merge unrelated histories"));
merge_bases = reverse_commit_list(merge_bases);
diff --git a/builtin/merge.c b/builtin/merge.c
index d748d46e135..ac9d58adc29 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1517,10 +1517,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (!remoteheads)
; /* already up-to-date */
- else if (!remoteheads->next)
- common = repo_get_merge_bases(the_repository, head_commit,
- remoteheads->item);
- else {
+ else if (!remoteheads->next) {
+ if (repo_get_merge_bases(the_repository, head_commit,
+ remoteheads->item, &common) < 0) {
+ ret = 2;
+ goto done;
+ }
+ } else {
struct commit_list *list = remoteheads;
commit_list_insert(head_commit, &list);
common = get_octopus_merge_bases(list);
@@ -1631,7 +1634,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
struct commit_list *j;
for (j = remoteheads; j; j = j->next) {
- struct commit_list *common_one;
+ struct commit_list *common_one = NULL;
struct commit *common_item;
/*
@@ -1639,9 +1642,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* merge_bases again, otherwise "git merge HEAD^
* HEAD^^" would be missed.
*/
- common_one = repo_get_merge_bases(the_repository,
- head_commit,
- j->item);
+ if (repo_get_merge_bases(the_repository, head_commit,
+ j->item, &common_one) < 0)
+ exit(128);
+
common_item = common_one->item;
free_commit_list(common_one);
if (!oideq(&common_item->object.oid, &j->item->object.oid)) {
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 043c65dccd9..06a55fc7325 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -879,7 +879,8 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
if (!upstream)
goto done;
- merge_bases = repo_get_merge_bases(the_repository, upstream, head);
+ if (repo_get_merge_bases(the_repository, upstream, head, &merge_bases) < 0)
+ exit(128);
if (!merge_bases || merge_bases->next)
goto done;
@@ -898,8 +899,9 @@ static void fill_branch_base(struct rebase_options *options,
{
struct commit_list *merge_bases = NULL;
- merge_bases = repo_get_merge_bases(the_repository, options->onto,
- options->orig_head);
+ if (repo_get_merge_bases(the_repository, options->onto,
+ options->orig_head, &merge_bases) < 0)
+ exit(128);
if (!merge_bases || merge_bases->next)
oidcpy(branch_base, null_oid());
else
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index fde8861ca4e..c97d0f6144c 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -297,7 +297,7 @@ static int try_difference(const char *arg)
show_rev(NORMAL, &end_oid, end);
show_rev(symmetric ? NORMAL : REVERSED, &start_oid, start);
if (symmetric) {
- struct commit_list *exclude;
+ struct commit_list *exclude = NULL;
struct commit *a, *b;
a = lookup_commit_reference(the_repository, &start_oid);
b = lookup_commit_reference(the_repository, &end_oid);
@@ -305,7 +305,8 @@ static int try_difference(const char *arg)
*dotdot = '.';
return 0;
}
- exclude = repo_get_merge_bases(the_repository, a, b);
+ if (repo_get_merge_bases(the_repository, a, b, &exclude) < 0)
+ exit(128);
while (exclude) {
struct commit *commit = pop_commit(&exclude);
show_rev(REVERSED, &commit->object.oid, NULL);
diff --git a/commit-reach.c b/commit-reach.c
index 359853275a9..3471c933f6c 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -185,9 +185,12 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)
struct commit_list *new_commits = NULL, *end = NULL;
for (j = ret; j; j = j->next) {
- struct commit_list *bases;
- bases = repo_get_merge_bases(the_repository, i->item,
- j->item);
+ struct commit_list *bases = NULL;
+ if (repo_get_merge_bases(the_repository, i->item,
+ j->item, &bases) < 0) {
+ free_commit_list(bases);
+ return NULL;
+ }
if (!new_commits)
new_commits = bases;
else
@@ -472,16 +475,12 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
return result;
}
-struct commit_list *repo_get_merge_bases(struct repository *r,
- struct commit *one,
- struct commit *two)
+int repo_get_merge_bases(struct repository *r,
+ struct commit *one,
+ struct commit *two,
+ struct commit_list **result)
{
- struct commit_list *result = NULL;
- if (get_merge_bases_many_0(r, one, 1, &two, 1, &result) < 0) {
- free_commit_list(result);
- return NULL;
- }
- return result;
+ return get_merge_bases_many_0(r, one, 1, &two, 1, result);
}
/*
diff --git a/commit-reach.h b/commit-reach.h
index 68f81549a44..2c6fcdd34f6 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -9,9 +9,10 @@ struct ref_filter;
struct object_id;
struct object_array;
-struct commit_list *repo_get_merge_bases(struct repository *r,
- struct commit *rev1,
- struct commit *rev2);
+int repo_get_merge_bases(struct repository *r,
+ struct commit *rev1,
+ struct commit *rev2,
+ struct commit_list **result);
struct commit_list *repo_get_merge_bases_many(struct repository *r,
struct commit *one, int n,
struct commit **twos);
diff --git a/diff-lib.c b/diff-lib.c
index 0e9ec4f68af..498224ccce2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -565,7 +565,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
{
int i;
struct commit *mb_child[2] = {0};
- struct commit_list *merge_bases;
+ struct commit_list *merge_bases = NULL;
for (i = 0; i < revs->pending.nr; i++) {
struct object *obj = revs->pending.objects[i].item;
@@ -592,7 +592,8 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
mb_child[1] = lookup_commit_reference(the_repository, &oid);
}
- merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
+ if (repo_get_merge_bases(the_repository, mb_child[0], mb_child[1], &merge_bases) < 0)
+ exit(128);
if (!merge_bases)
die(_("no merge base found"));
if (merge_bases->next)
diff --git a/log-tree.c b/log-tree.c
index 504da6b519e..4f337766a39 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -1010,7 +1010,7 @@ static int do_remerge_diff(struct rev_info *opt,
struct object_id *oid)
{
struct merge_options o;
- struct commit_list *bases;
+ struct commit_list *bases = NULL;
struct merge_result res = {0};
struct pretty_print_context ctx = {0};
struct commit *parent1 = parents->item;
@@ -1035,7 +1035,8 @@ static int do_remerge_diff(struct rev_info *opt,
/* Parse the relevant commits and get the merge bases */
parse_commit_or_die(parent1);
parse_commit_or_die(parent2);
- bases = repo_get_merge_bases(the_repository, parent1, parent2);
+ if (repo_get_merge_bases(the_repository, parent1, parent2, &bases) < 0)
+ exit(128);
/* Re-merge the parents */
merge_incore_recursive(&o, bases, parent1, parent2, &res);
diff --git a/merge-ort.c b/merge-ort.c
index 64e76afe89f..88f23a37f5a 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -5060,7 +5060,11 @@ static void merge_ort_internal(struct merge_options *opt,
struct strbuf merge_base_abbrev = STRBUF_INIT;
if (!merge_bases) {
- merge_bases = repo_get_merge_bases(the_repository, h1, h2);
+ if (repo_get_merge_bases(the_repository, h1, h2,
+ &merge_bases) < 0) {
+ result->clean = -1;
+ return;
+ }
/* See merge-ort.h:merge_incore_recursive() declaration NOTE */
merge_bases = reverse_commit_list(merge_bases);
}
diff --git a/merge-recursive.c b/merge-recursive.c
index e3fe7803cbe..0bed0b97424 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3632,7 +3632,9 @@ static int merge_recursive_internal(struct merge_options *opt,
}
if (!merge_bases) {
- merge_bases = repo_get_merge_bases(the_repository, h1, h2);
+ if (repo_get_merge_bases(the_repository, h1, h2,
+ &merge_bases) < 0)
+ return -1;
merge_bases = reverse_commit_list(merge_bases);
}
diff --git a/notes-merge.c b/notes-merge.c
index 8799b522a55..51282934ae6 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -607,7 +607,8 @@ int notes_merge(struct notes_merge_options *o,
assert(local && remote);
/* Find merge bases */
- bases = repo_get_merge_bases(the_repository, local, remote);
+ if (repo_get_merge_bases(the_repository, local, remote, &bases) < 0)
+ exit(128);
if (!bases) {
base_oid = null_oid();
base_tree_oid = the_hash_algo->empty_tree;
diff --git a/object-name.c b/object-name.c
index 0bfa29dbbfe..89c34f9b49e 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1481,7 +1481,7 @@ int repo_get_oid_mb(struct repository *r,
struct object_id *oid)
{
struct commit *one, *two;
- struct commit_list *mbs;
+ struct commit_list *mbs = NULL;
struct object_id oid_tmp;
const char *dots;
int st;
@@ -1509,7 +1509,8 @@ int repo_get_oid_mb(struct repository *r,
two = lookup_commit_reference_gently(r, &oid_tmp, 0);
if (!two)
return -1;
- mbs = repo_get_merge_bases(r, one, two);
+ if (repo_get_merge_bases(r, one, two, &mbs) < 0)
+ return -1;
if (!mbs || mbs->next)
st = -1;
else {
diff --git a/revision.c b/revision.c
index 00d5c29bfce..3492cc95159 100644
--- a/revision.c
+++ b/revision.c
@@ -1965,7 +1965,7 @@ static void add_pending_commit_list(struct rev_info *revs,
static void prepare_show_merge(struct rev_info *revs)
{
- struct commit_list *bases;
+ struct commit_list *bases = NULL;
struct commit *head, *other;
struct object_id oid;
const char **prune = NULL;
@@ -1980,7 +1980,8 @@ static void prepare_show_merge(struct rev_info *revs)
other = lookup_commit_or_die(&oid, "MERGE_HEAD");
add_pending_object(revs, &head->object, "HEAD");
add_pending_object(revs, &other->object, "MERGE_HEAD");
- bases = repo_get_merge_bases(the_repository, head, other);
+ if (repo_get_merge_bases(the_repository, head, other, &bases) < 0)
+ exit(128);
add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
free_commit_list(bases);
@@ -2068,14 +2069,15 @@ static int handle_dotdot_1(const char *arg, char *dotdot,
} else {
/* A...B -- find merge bases between the two */
struct commit *a, *b;
- struct commit_list *exclude;
+ struct commit_list *exclude = NULL;
a = lookup_commit_reference(revs->repo, &a_obj->oid);
b = lookup_commit_reference(revs->repo, &b_obj->oid);
if (!a || !b)
return dotdot_missing(arg, dotdot, revs, symmetric);
- exclude = repo_get_merge_bases(the_repository, a, b);
+ if (repo_get_merge_bases(the_repository, a, b, &exclude) < 0)
+ return -1;
add_rev_cmdline_list(revs, exclude, REV_CMD_MERGE_BASE,
flags_exclude);
add_pending_commit_list(revs, exclude, flags_exclude);
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed9..4417f2f1956 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3913,7 +3913,7 @@ static int do_merge(struct repository *r,
int run_commit_flags = 0;
struct strbuf ref_name = STRBUF_INIT;
struct commit *head_commit, *merge_commit, *i;
- struct commit_list *bases, *j;
+ struct commit_list *bases = NULL, *j;
struct commit_list *to_merge = NULL, **tail = &to_merge;
const char *strategy = !opts->xopts.nr &&
(!opts->strategy ||
@@ -4139,7 +4139,11 @@ static int do_merge(struct repository *r,
}
merge_commit = to_merge->item;
- bases = repo_get_merge_bases(r, head_commit, merge_commit);
+ if (repo_get_merge_bases(r, head_commit, merge_commit, &bases) < 0) {
+ ret = -1;
+ goto leave_merge;
+ }
+
if (bases && oideq(&merge_commit->object.oid,
&bases->item->object.oid)) {
ret = 0;
diff --git a/submodule.c b/submodule.c
index e603a19a876..04931a5474b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -595,7 +595,12 @@ static void show_submodule_header(struct diff_options *o,
(!is_null_oid(two) && !*right))
message = "(commits not present)";
- *merge_bases = repo_get_merge_bases(sub, *left, *right);
+ *merge_bases = NULL;
+ if (repo_get_merge_bases(sub, *left, *right, merge_bases) < 0) {
+ message = "(corrupt repository)";
+ goto output_header;
+ }
+
if (*merge_bases) {
if ((*merge_bases)->item == *left)
fast_forward = 1;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index b2c8a43fce3..5d1e7aca4c8 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -945,4 +945,16 @@ test_expect_success 'check the input format when --stdin is passed' '
test_cmp expect actual
'
+test_expect_success 'error out on missing commits as well' '
+ git init --bare missing-commit.git &&
+ git rev-list --objects side1 side3 >list-including-initial &&
+ grep -v ^$(git rev-parse side1^) <list-including-initial >list &&
+ git pack-objects missing-commit.git/objects/pack/missing-initial <list &&
+ side1=$(git rev-parse side1) &&
+ side3=$(git rev-parse side3) &&
+ test_must_fail git --git-dir=missing-commit.git \
+ merge-tree --allow-unrelated-histories $side1 $side3 >actual &&
+ test_must_be_empty actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 09/12] get_octopus_merge_bases(): pass on errors from `merge_bases_many()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `merge_bases_many()` function was just taught to indicate parsing
errors, and now the `repo_get_merge_bases()` function (which is also
surfaced via the `get_merge_bases()` macro) is aware of that, too.
Naturally, the callers need to be adjusted now, too.
Next step: adjust `repo_get_merge_bases_many()`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/merge-base.c | 5 +++--
builtin/merge.c | 6 +++++-
builtin/pull.c | 5 +++--
commit-reach.c | 20 +++++++++++---------
commit-reach.h | 2 +-
5 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 0308fd73289..6faabfb6698 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -77,13 +77,14 @@ static int handle_independent(int count, const char **args)
static int handle_octopus(int count, const char **args, int show_all)
{
struct commit_list *revs = NULL;
- struct commit_list *result, *rev;
+ struct commit_list *result = NULL, *rev;
int i;
for (i = count - 1; i >= 0; i--)
commit_list_insert(get_commit_reference(args[i]), &revs);
- result = get_octopus_merge_bases(revs);
+ if (get_octopus_merge_bases(revs, &result) < 0)
+ return 128;
free_commit_list(revs);
reduce_heads_replace(&result);
diff --git a/builtin/merge.c b/builtin/merge.c
index ac9d58adc29..94c5b693972 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1526,7 +1526,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
} else {
struct commit_list *list = remoteheads;
commit_list_insert(head_commit, &list);
- common = get_octopus_merge_bases(list);
+ if (get_octopus_merge_bases(list, &common) < 0) {
+ free(list);
+ ret = 2;
+ goto done;
+ }
free(list);
}
diff --git a/builtin/pull.c b/builtin/pull.c
index e6f2942c0c5..0c5a55f2f4d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -820,7 +820,7 @@ static int get_octopus_merge_base(struct object_id *merge_base,
const struct object_id *merge_head,
const struct object_id *fork_point)
{
- struct commit_list *revs = NULL, *result;
+ struct commit_list *revs = NULL, *result = NULL;
commit_list_insert(lookup_commit_reference(the_repository, curr_head),
&revs);
@@ -830,7 +830,8 @@ static int get_octopus_merge_base(struct object_id *merge_base,
commit_list_insert(lookup_commit_reference(the_repository, fork_point),
&revs);
- result = get_octopus_merge_bases(revs);
+ if (get_octopus_merge_bases(revs, &result) < 0)
+ exit(128);
free_commit_list(revs);
reduce_heads_replace(&result);
diff --git a/commit-reach.c b/commit-reach.c
index 3471c933f6c..1b618eb9cd1 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -172,24 +172,26 @@ static int merge_bases_many(struct repository *r,
return 0;
}
-struct commit_list *get_octopus_merge_bases(struct commit_list *in)
+int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)
{
- struct commit_list *i, *j, *k, *ret = NULL;
+ struct commit_list *i, *j, *k;
if (!in)
- return ret;
+ return 0;
- commit_list_insert(in->item, &ret);
+ commit_list_insert(in->item, result);
for (i = in->next; i; i = i->next) {
struct commit_list *new_commits = NULL, *end = NULL;
- for (j = ret; j; j = j->next) {
+ for (j = *result; j; j = j->next) {
struct commit_list *bases = NULL;
if (repo_get_merge_bases(the_repository, i->item,
j->item, &bases) < 0) {
free_commit_list(bases);
- return NULL;
+ free_commit_list(*result);
+ *result = NULL;
+ return -1;
}
if (!new_commits)
new_commits = bases;
@@ -198,10 +200,10 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)
for (k = bases; k; k = k->next)
end = k;
}
- free_commit_list(ret);
- ret = new_commits;
+ free_commit_list(*result);
+ *result = new_commits;
}
- return ret;
+ return 0;
}
static int remove_redundant_no_gen(struct repository *r,
diff --git a/commit-reach.h b/commit-reach.h
index 2c6fcdd34f6..4690b6ecd0c 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -21,7 +21,7 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
struct commit *one, int n,
struct commit **twos);
-struct commit_list *get_octopus_merge_bases(struct commit_list *in);
+int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result);
int repo_is_descendant_of(struct repository *r,
struct commit *commit,
--
gitgitgadget
^ permalink raw reply related
* [PATCH 10/12] repo_get_merge_bases_many(): pass on errors from `merge_bases_many()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `merge_bases_many()` function was just taught to indicate parsing
errors, and now the `repo_get_merge_bases_many()` function is aware of
that, too.
Naturally, there are a lot of callers that need to be adjusted now, too.
Next stop: `repo_get_merge_bases_dirty()`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
bisect.c | 7 ++++---
builtin/log.c | 13 +++++++------
commit-reach.c | 16 ++++++----------
commit-reach.h | 7 ++++---
commit.c | 7 ++++---
t/helper/test-reach.c | 9 ++++++---
6 files changed, 31 insertions(+), 28 deletions(-)
diff --git a/bisect.c b/bisect.c
index 1be8e0a2711..2018466d69f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -851,10 +851,11 @@ static void handle_skipped_merge_base(const struct object_id *mb)
static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
{
enum bisect_error res = BISECT_OK;
- struct commit_list *result;
+ struct commit_list *result = NULL;
- result = repo_get_merge_bases_many(the_repository, rev[0], rev_nr - 1,
- rev + 1);
+ if (repo_get_merge_bases_many(the_repository, rev[0], rev_nr - 1,
+ rev + 1, &result) < 0)
+ exit(128);
for (; result; result = result->next) {
const struct object_id *mb = &result->item->object.oid;
diff --git a/builtin/log.c b/builtin/log.c
index befafd6ae04..c75790a7cec 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1656,7 +1656,7 @@ static struct commit *get_base_commit(const char *base_commit,
struct branch *curr_branch = branch_get(NULL);
const char *upstream = branch_get_upstream(curr_branch, NULL);
if (upstream) {
- struct commit_list *base_list;
+ struct commit_list *base_list = NULL;
struct commit *commit;
struct object_id oid;
@@ -1667,11 +1667,12 @@ static struct commit *get_base_commit(const char *base_commit,
return NULL;
}
commit = lookup_commit_or_die(&oid, "upstream base");
- base_list = repo_get_merge_bases_many(the_repository,
- commit, total,
- list);
- /* There should be one and only one merge base. */
- if (!base_list || base_list->next) {
+ if (repo_get_merge_bases_many(the_repository,
+ commit, total,
+ list,
+ &base_list) < 0 ||
+ /* There should be one and only one merge base. */
+ !base_list || base_list->next) {
if (die_on_failure) {
die(_("could not find exact merge base"));
} else {
diff --git a/commit-reach.c b/commit-reach.c
index 1b618eb9cd1..f0006ab6422 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -451,17 +451,13 @@ static int get_merge_bases_many_0(struct repository *r,
return 0;
}
-struct commit_list *repo_get_merge_bases_many(struct repository *r,
- struct commit *one,
- int n,
- struct commit **twos)
+int repo_get_merge_bases_many(struct repository *r,
+ struct commit *one,
+ int n,
+ struct commit **twos,
+ struct commit_list **result)
{
- struct commit_list *result = NULL;
- if (get_merge_bases_many_0(r, one, n, twos, 1, &result) < 0) {
- free_commit_list(result);
- return NULL;
- }
- return result;
+ return get_merge_bases_many_0(r, one, n, twos, 1, result);
}
struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
diff --git a/commit-reach.h b/commit-reach.h
index 4690b6ecd0c..458043f4d58 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -13,9 +13,10 @@ int repo_get_merge_bases(struct repository *r,
struct commit *rev1,
struct commit *rev2,
struct commit_list **result);
-struct commit_list *repo_get_merge_bases_many(struct repository *r,
- struct commit *one, int n,
- struct commit **twos);
+int repo_get_merge_bases_many(struct repository *r,
+ struct commit *one, int n,
+ struct commit **twos,
+ struct commit_list **result);
/* To be used only when object flags after this call no longer matter */
struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
struct commit *one, int n,
diff --git a/commit.c b/commit.c
index 8405d7c3fce..00add5d81c6 100644
--- a/commit.c
+++ b/commit.c
@@ -1054,7 +1054,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)
{
struct object_id oid;
struct rev_collect revs;
- struct commit_list *bases;
+ struct commit_list *bases = NULL;
int i;
struct commit *ret = NULL;
char *full_refname;
@@ -1079,8 +1079,9 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)
for (i = 0; i < revs.nr; i++)
revs.commit[i]->object.flags &= ~TMP_MARK;
- bases = repo_get_merge_bases_many(the_repository, commit, revs.nr,
- revs.commit);
+ if (repo_get_merge_bases_many(the_repository, commit, revs.nr,
+ revs.commit, &bases) < 0)
+ exit(128);
/*
* There should be one and only one merge base, when we found
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index aa816e168ea..84ee9da8681 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -117,9 +117,12 @@ int cmd__reach(int ac, const char **av)
else if (!strcmp(av[1], "is_descendant_of"))
printf("%s(A,X):%d\n", av[1], repo_is_descendant_of(r, A, X));
else if (!strcmp(av[1], "get_merge_bases_many")) {
- struct commit_list *list = repo_get_merge_bases_many(the_repository,
- A, X_nr,
- X_array);
+ struct commit_list *list = NULL;
+ if (repo_get_merge_bases_many(the_repository,
+ A, X_nr,
+ X_array,
+ &list) < 0)
+ exit(128);
printf("%s(A,X):\n", av[1]);
print_sorted_commit_ids(list);
} else if (!strcmp(av[1], "reduce_heads")) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH 11/12] repo_get_merge_bases_many_dirty(): pass on errors from `merge_bases_many()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `merge_bases_many()` function was just taught to indicate parsing
errors, and now the `repo_get_merge_bases_many_dirty()` function is
aware of that, too.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/merge-base.c | 7 ++++---
commit-reach.c | 16 ++++++----------
commit-reach.h | 7 ++++---
3 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 6faabfb6698..2b6af1bc35b 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -13,10 +13,11 @@
static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
{
- struct commit_list *result, *r;
+ struct commit_list *result = NULL, *r;
- result = repo_get_merge_bases_many_dirty(the_repository, rev[0],
- rev_nr - 1, rev + 1);
+ if (repo_get_merge_bases_many_dirty(the_repository, rev[0],
+ rev_nr - 1, rev + 1, &result) < 0)
+ return -1;
if (!result)
return 1;
diff --git a/commit-reach.c b/commit-reach.c
index f0006ab6422..25b39c302a8 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -460,17 +460,13 @@ int repo_get_merge_bases_many(struct repository *r,
return get_merge_bases_many_0(r, one, n, twos, 1, result);
}
-struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
- struct commit *one,
- int n,
- struct commit **twos)
+int repo_get_merge_bases_many_dirty(struct repository *r,
+ struct commit *one,
+ int n,
+ struct commit **twos,
+ struct commit_list **result)
{
- struct commit_list *result = NULL;
- if (get_merge_bases_many_0(r, one, n, twos, 0, &result) < 0) {
- free_commit_list(result);
- return NULL;
- }
- return result;
+ return get_merge_bases_many_0(r, one, n, twos, 0, result);
}
int repo_get_merge_bases(struct repository *r,
diff --git a/commit-reach.h b/commit-reach.h
index 458043f4d58..bf63cc468fd 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -18,9 +18,10 @@ int repo_get_merge_bases_many(struct repository *r,
struct commit **twos,
struct commit_list **result);
/* To be used only when object flags after this call no longer matter */
-struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
- struct commit *one, int n,
- struct commit **twos);
+int repo_get_merge_bases_many_dirty(struct repository *r,
+ struct commit *one, int n,
+ struct commit **twos,
+ struct commit_list **result);
int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 12/12] paint_down_to_common(): special-case shallow/partial clones
From: Johannes Schindelin via GitGitGadget @ 2024-02-13 8:41 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.git.1707813709.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In shallow/partial clones, we _expect_ commits to be missing. Let's
teach the merge-base logic to ignore those and simply go ahead and
treat the involved commit histories as cut off at that point.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/commit-reach.c b/commit-reach.c
index 25b39c302a8..4af60c2501d 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -10,6 +10,8 @@
#include "tag.h"
#include "commit-reach.h"
#include "ewah/ewok.h"
+#include "shallow.h"
+#include "promisor-remote.h"
/* Remember to update object flag allocation in object.h */
#define PARENT1 (1u<<16)
@@ -115,7 +117,9 @@ static int paint_down_to_common(struct repository *r,
* dispatched with a `die()`.
*/
free_commit_list(*result);
- if (ignore_missing_commits)
+ if (ignore_missing_commits ||
+ is_repository_shallow(r) ||
+ repo_has_promisor_remote(r))
return 0;
return error(_("could not parse commit %s"),
oid_to_hex(&p->object.oid));
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 03/30] object-names: support input of oids in any supported hash
From: Linus Arver @ 2024-02-13 9:33 UTC (permalink / raw)
To: Eric W. Biederman, Junio C Hamano
Cc: git, brian m. carlson, Eric Sunshine, Eric W. Biederman
In-Reply-To: <20231002024034.2611-3-ebiederm@gmail.com>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Support short oids encoded in any algorithm, while ensuring enough of
> the oid is specified to disambiguate between all of the oids in the
> repository encoded in any algorithm.
>
> By default have the code continue to only accept oids specified in the
> storage hash algorithm of the repository, but when something is
> ambiguous display all of the possible oids from any accepted oid
> encoding.
>
> A new flag is added GET_OID_HASH_ANY that when supplied causes the
> code to accept oids specified in any hash algorithm, and to return the
> oids that were resolved.
>
> This implements the functionality that allows both SHA-1 and SHA-256
> object names, from the "Object names on the command line" section of
> the hash function transition document.
>
> Care is taken in get_short_oid so that when the result is ambiguous
> the output remains the same if GIT_OID_HASH_ANY was not supplied. If
> GET_OID_HASH_ANY was supplied objects of any hash algorithm that match
> the prefix are displayed.
>
> This required updating repo_for_each_abbrev to give it a parameter so
> that it knows to look at all hash algorithms.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> builtin/rev-parse.c | 2 +-
> hash-ll.h | 1 +
> object-name.c | 46 ++++++++++++++++++++++++++++++++++-----------
> object-name.h | 3 ++-
> 4 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index fde8861ca4e0..43e96765400c 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -882,7 +882,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> continue;
> }
> if (skip_prefix(arg, "--disambiguate=", &arg)) {
> - repo_for_each_abbrev(the_repository, arg,
> + repo_for_each_abbrev(the_repository, arg, the_hash_algo,
> show_abbrev, NULL);
> continue;
> }
> diff --git a/hash-ll.h b/hash-ll.h
> index 10d84cc20888..2cfde63ae1cf 100644
> --- a/hash-ll.h
> +++ b/hash-ll.h
> @@ -145,6 +145,7 @@ struct object_id {
> #define GET_OID_RECORD_PATH 0200
> #define GET_OID_ONLY_TO_DIE 04000
> #define GET_OID_REQUIRE_PATH 010000
> +#define GET_OID_HASH_ANY 020000
So far the "GET_OID_*" flags sound like the "GET_OID" is the prefix and
the remaining text describes the behavior. So in this sense, the
behavior here would be "HASH_ANY" and I think "ANY_HASH" is better. But
also, going by the description of this flag in the commit message, I
think "ACCEPT_ANY_HASH" is still better.
>
> #define GET_OID_DISAMBIGUATORS \
> (GET_OID_COMMIT | GET_OID_COMMITTISH | \
> diff --git a/object-name.c b/object-name.c
> index 0bfa29dbbfe9..7dd6e5e47566 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -25,6 +25,7 @@
> #include "midx.h"
> #include "commit-reach.h"
> #include "date.h"
> +#include "object-file-convert.h"
>
> static int get_oid_oneline(struct repository *r, const char *, struct object_id *, struct commit_list *);
>
> @@ -49,6 +50,7 @@ struct disambiguate_state {
>
> static void update_candidates(struct disambiguate_state *ds, const struct object_id *current)
> {
> + /* The hash algorithm of current has already been filtered */
Is this new comment describing existing behavior before this patch or
after?
> if (ds->always_call_fn) {
> ds->ambiguous = ds->fn(ds->repo, current, ds->cb_data) ? 1 : 0;
> return;
> @@ -134,6 +136,8 @@ static void unique_in_midx(struct multi_pack_index *m,
> {
> uint32_t num, i, first = 0;
> const struct object_id *current = NULL;
> + int len = ds->len > ds->repo->hash_algo->hexsz ?
> + ds->repo->hash_algo->hexsz : ds->len;
Nit: So this is just trying to use the shorter length between ds->len
and hexsz. Would it be good to encode this information into the variable
name, such as "len_short" or similar? And if so, flipping the ">" to
"<" would be more natural, like
int shorter_len = ds->len < ds->repo->hash_algo->hexsz ?
ds->len : ds->repo->hash_algo->hexsz;
because it reads "if ds->len is shorter, use it; otherwise use hexsz".
> num = m->num_objects;
>
> if (!num)
> @@ -149,7 +153,7 @@ static void unique_in_midx(struct multi_pack_index *m,
> for (i = first; i < num && !ds->ambiguous; i++) {
> struct object_id oid;
> current = nth_midxed_object_oid(&oid, m, i);
> - if (!match_hash(ds->len, ds->bin_pfx.hash, current->hash))
> + if (!match_hash(len, ds->bin_pfx.hash, current->hash))
> break;
> update_candidates(ds, current);
> }
> @@ -159,6 +163,8 @@ static void unique_in_pack(struct packed_git *p,
> struct disambiguate_state *ds)
> {
> uint32_t num, i, first = 0;
> + int len = ds->len > ds->repo->hash_algo->hexsz ?
> + ds->repo->hash_algo->hexsz : ds->len;
Ditto.
>
> if (p->multi_pack_index)
> return;
> @@ -177,7 +183,7 @@ static void unique_in_pack(struct packed_git *p,
> for (i = first; i < num && !ds->ambiguous; i++) {
> struct object_id oid;
> nth_packed_object_id(&oid, p, i);
> - if (!match_hash(ds->len, ds->bin_pfx.hash, oid.hash))
> + if (!match_hash(len, ds->bin_pfx.hash, oid.hash))
> break;
> update_candidates(ds, &oid);
> }
> @@ -188,6 +194,10 @@ static void find_short_packed_object(struct disambiguate_state *ds)
> struct multi_pack_index *m;
> struct packed_git *p;
>
> + /* Skip, unless oids from the storage hash algorithm are wanted */
Perhaps a simpler phrasing would be
/* Only accept oids specified in the storage hash algorithm of the repository. */
which is closer to the wording of the commit message?
> + if (ds->bin_pfx.algo && (&hash_algos[ds->bin_pfx.algo] != ds->repo->hash_algo))
> + return;
> +
> for (m = get_multi_pack_index(ds->repo); m && !ds->ambiguous;
> m = m->next)
> unique_in_midx(m, ds);
> @@ -326,11 +336,12 @@ int set_disambiguate_hint_config(const char *var, const char *value)
>
> static int init_object_disambiguation(struct repository *r,
> const char *name, int len,
> + const struct git_hash_algo *algo,
> struct disambiguate_state *ds)
> {
> int i;
>
> - if (len < MINIMUM_ABBREV || len > the_hash_algo->hexsz)
> + if (len < MINIMUM_ABBREV || len > GIT_MAX_HEXSZ)
> return -1;
>
> memset(ds, 0, sizeof(*ds));
> @@ -357,6 +368,7 @@ static int init_object_disambiguation(struct repository *r,
> ds->len = len;
> ds->hex_pfx[len] = '\0';
> ds->repo = r;
> + ds->bin_pfx.algo = algo ? hash_algo_by_ptr(algo) : GIT_HASH_UNKNOWN;
> prepare_alt_odb(r);
> return 0;
> }
> @@ -491,9 +503,10 @@ static int repo_collect_ambiguous(struct repository *r UNUSED,
> return collect_ambiguous(oid, data);
> }
>
> -static int sort_ambiguous(const void *a, const void *b, void *ctx)
> +static int sort_ambiguous(const void *va, const void *vb, void *ctx)
> {
> struct repository *sort_ambiguous_repo = ctx;
> + const struct object_id *a = va, *b = vb;
> int a_type = oid_object_info(sort_ambiguous_repo, a, NULL);
> int b_type = oid_object_info(sort_ambiguous_repo, b, NULL);
> int a_type_sort;
> @@ -503,8 +516,12 @@ static int sort_ambiguous(const void *a, const void *b, void *ctx)
> * Sorts by hash within the same object type, just as
> * oid_array_for_each_unique() would do.
> */
> - if (a_type == b_type)
> - return oidcmp(a, b);
> + if (a_type == b_type) {
> + if (a->algo == b->algo)
> + return oidcmp(a, b);
> + else
> + return a->algo > b->algo ? 1 : -1;
> + }
This is duplicated from the previous patch (void_hashcmp). Do we want to
avoid a performance penalty from calling out to a common function?
> /*
> * Between object types show tags, then commits, and finally
> @@ -533,8 +550,12 @@ static enum get_oid_result get_short_oid(struct repository *r,
> int status;
> struct disambiguate_state ds;
> int quietly = !!(flags & GET_OID_QUIETLY);
> + const struct git_hash_algo *algo = r->hash_algo;
I see some existing uses of the name "algop" (presumably to mean
pointer-to-algorithm) and wonder if we should do the same here, for
consistency.
> +
> + if (flags & GET_OID_HASH_ANY)
> + algo = NULL;
If we look at the change to init_object_disambiguation() above, it looks
like the "algo" here is only used to find a GIT_HASH_* constant. So it
seems a bit roundabout to use a NULL here, then inside
init_object_disambiguation() do
ds->bin_pfx.algo = algo ? hash_algo_by_ptr(algo) : GIT_HASH_UNKNOWN;
to convert the NULL to GIT_HASH_UNKNOWN when we could probably just pass
in the GIT_HASH_* constant directly from the caller.
> - if (init_object_disambiguation(r, name, len, &ds) < 0)
> + if (init_object_disambiguation(r, name, len, algo, &ds) < 0)
> return -1;
>
> if (HAS_MULTI_BITS(flags & GET_OID_DISAMBIGUATORS))
> @@ -588,7 +609,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
> if (!ds.ambiguous)
> ds.fn = NULL;
>
> - repo_for_each_abbrev(r, ds.hex_pfx, collect_ambiguous, &collect);
> + repo_for_each_abbrev(r, ds.hex_pfx, algo, collect_ambiguous, &collect);
> sort_ambiguous_oid_array(r, &collect);
>
> if (oid_array_for_each(&collect, show_ambiguous_object, &out))
> @@ -610,13 +631,14 @@ static enum get_oid_result get_short_oid(struct repository *r,
> }
>
> int repo_for_each_abbrev(struct repository *r, const char *prefix,
> + const struct git_hash_algo *algo,
> each_abbrev_fn fn, void *cb_data)
> {
> struct oid_array collect = OID_ARRAY_INIT;
> struct disambiguate_state ds;
> int ret;
>
> - if (init_object_disambiguation(r, prefix, strlen(prefix), &ds) < 0)
> + if (init_object_disambiguation(r, prefix, strlen(prefix), algo, &ds) < 0)
> return -1;
>
> ds.always_call_fn = 1;
> @@ -787,10 +809,12 @@ void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid,
> int repo_find_unique_abbrev_r(struct repository *r, char *hex,
> const struct object_id *oid, int len)
> {
> + const struct git_hash_algo *algo =
> + oid->algo ? &hash_algos[oid->algo] : r->hash_algo;
It would be nice to have symmetry with the style in get_short_oid() and
instead do
const struct git_hash_algo *algo = r->hash_algo;
if (oid->algo)
algo = &hash_algos[oid->algo];
> struct disambiguate_state ds;
> struct min_abbrev_data mad;
> struct object_id oid_ret;
> - const unsigned hexsz = r->hash_algo->hexsz;
> + const unsigned hexsz = algo->hexsz;
>
> if (len < 0) {
> unsigned long count = repo_approximate_object_count(r);
> @@ -826,7 +850,7 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex,
>
> find_abbrev_len_packed(&mad);
>
> - if (init_object_disambiguation(r, hex, mad.cur_len, &ds) < 0)
> + if (init_object_disambiguation(r, hex, mad.cur_len, algo, &ds) < 0)
> return -1;
>
> ds.fn = repo_extend_abbrev_len;
> diff --git a/object-name.h b/object-name.h
> index 9ae522307148..064ddc97d1fe 100644
> --- a/object-name.h
> +++ b/object-name.h
> @@ -67,7 +67,8 @@ enum get_oid_result get_oid_with_context(struct repository *repo, const char *st
>
>
> typedef int each_abbrev_fn(const struct object_id *oid, void *);
> -int repo_for_each_abbrev(struct repository *r, const char *prefix, each_abbrev_fn, void *);
> +int repo_for_each_abbrev(struct repository *r, const char *prefix,
> + const struct git_hash_algo *algo, each_abbrev_fn, void *);
>
> int set_disambiguate_hint_config(const char *var, const char *value);
>
> --
> 2.41.0
^ permalink raw reply
* Re: [PATCH v3 0/4] completion: remove hardcoded config variable names
From: Patrick Steinhardt @ 2024-02-13 9:35 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <pull.1660.v3.git.git.1707589943.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]
On Sat, Feb 10, 2024 at 06:32:19PM +0000, Philippe Blain via GitGitGadget wrote:
> Changes since v2:
>
> * Moved the addition of the tests to 2/4, and tweaked 3/4 and 4/4 so they
> simply adjust the test names
> * Added a test for user-defined submodule names, as suggested by Patrick
> * Added more details in the commit message of 3/4 around the use of global
> variables as caches
> * Slightly improved commit message wording and fixed typos
> * Added 'local' where suggested
> * Dropped 4/5 which modified 'git help', since it's not needed (thanks
> Patrick!)
>
> Changes since v1:
>
> * Corrected my email in PATCH 2/5 (sorry for the noise)
>
> v1: This series removes hardcoded config variable names in the
> __git_complete_config_variable_name function, partly by adding a new mode to
> 'git help'. It also adds completion for 'submodule.*' config variables,
> which were previously missing.
>
> I think it makes sense to do that in the same series since it's closely
> related, and splitting it would result in textual conflicts between both
> series if one does not build on top of the other, but I'm open to other
> suggestions.
>
> Thanks,
>
> Philippe.
I ain't got anything else to add to this patch series. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 04/30] repository: add a compatibility hash algorithm
From: Linus Arver @ 2024-02-13 10:02 UTC (permalink / raw)
To: Eric W. Biederman, Junio C Hamano
Cc: git, brian m. carlson, Eric Sunshine, Eric W. Biederman
In-Reply-To: <20231002024034.2611-4-ebiederm@gmail.com>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
>
> We currently have support for using a full stage 4 SHA-256
> implementation. However, we'd like to support interoperability with
> SHA-1 repositories as well. The transition plan anticipates a
> compatibility hash algorithm configuration option that we can use to
> implement support for this.
Perhaps add
See section "Object names on the command line" in
git/Documentation/technical/hash-function-transition.txt .
? That section does not use the language "compatibility hash algorithm"
though, and I think "hash compatibility option" is easier to say.
Hmm, or are you talking about "compatObjectFormat" discussed in that doc?
> Let's add an element to the repository
> structure that indicates the compatibility hash algorithm so we can use
> it when we need to consider interoperability between algorithms.
How about just
Add a hash compatibility option to the repository structure to
consider interoperability between hash algorithms.
?
Aside: already we are seeing multiple keywords "compatibility",
"transition", "interoperability" to all mean roughly similar things. I
hope we can settle on just one (ideally) in the codebase by the end of
this series.
> Add a helper function repo_set_compat_hash_algo that takes a
> compatibility hash algorithm and sets "repo->compat_hash_algo". If
> GIT_HASH_UNKNOWN is passed as the compatibility hash algorithm
> "repo->compat_hash_algo" is set to NULL.
>
> For now, the code results in "repo->compat_hash_algo" always being set
> to NULL, but that will change once a configuration option is added.
It's not clear to me whether you are talking about a config option to
describe the different stages of transition around algorithms, or a hash
algorithm itself (SHA1, SHA256, UNKNOWN).
> Inspired-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
> repository.c | 8 ++++++++
> repository.h | 4 ++++
> setup.c | 3 +++
> 3 files changed, 15 insertions(+)
>
> diff --git a/repository.c b/repository.c
> index a7679ceeaa45..80252b79e93e 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -104,6 +104,13 @@ void repo_set_hash_algo(struct repository *repo, int hash_algo)
> repo->hash_algo = &hash_algos[hash_algo];
> }
>
> +void repo_set_compat_hash_algo(struct repository *repo, int algo)
> +{
> + if (hash_algo_by_ptr(repo->hash_algo) == algo)
> + BUG("hash_algo and compat_hash_algo match");
> + repo->compat_hash_algo = algo ? &hash_algos[algo] : NULL;
> +}
Ah, OK. So we are talking about an algorithm itself. Looking at this
code it seems like a compat_hash_algo is something like "the hash
algorithm I want my repository to start using but which has not
already". Such a description would have been useful in the commit
message.
Nit: I think
BUG("compat_hash_algo may not be the same as hash_algo");
is more natural because the error message should explain the badness of
the behavior rather than merely reflect the triggering condition. And
the "star of the show" here is the new compat_hash_algo member, so it
makes sense to emphasize that more as the only subject of the sentence
instead of grouping it together with hash_algo (given them equal
importance).
> +
> /*
> * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.
> * Return 0 upon success and a non-zero value upon failure.
> @@ -184,6 +191,7 @@ int repo_init(struct repository *repo,
> goto error;
>
> repo_set_hash_algo(repo, format.hash_algo);
> + repo_set_compat_hash_algo(repo, GIT_HASH_UNKNOWN);
> repo->repository_format_worktree_config = format.worktree_config;
>
> /* take ownership of format.partial_clone */
> diff --git a/repository.h b/repository.h
> index 5f18486f6465..bf3fc601cc53 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -160,6 +160,9 @@ struct repository {
> /* Repository's current hash algorithm, as serialized on disk. */
> const struct git_hash_algo *hash_algo;
>
> + /* Repository's compatibility hash algorithm. */
Perhaps add "May not be the same as hash_algo." ?
> + const struct git_hash_algo *compat_hash_algo;
> +
> /* A unique-id for tracing purposes. */
> int trace2_repo_id;
>
> @@ -199,6 +202,7 @@ void repo_set_gitdir(struct repository *repo, const char *root,
> const struct set_gitdir_args *extra_args);
> void repo_set_worktree(struct repository *repo, const char *path);
> void repo_set_hash_algo(struct repository *repo, int algo);
> +void repo_set_compat_hash_algo(struct repository *repo, int compat_algo);
> void initialize_the_repository(void);
> RESULT_MUST_BE_USED
> int repo_init(struct repository *r, const char *gitdir, const char *worktree);
> diff --git a/setup.c b/setup.c
> index 18927a847b86..aa8bf5da5226 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1564,6 +1564,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
> }
> if (startup_info->have_repository) {
> repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
> + repo_set_compat_hash_algo(the_repository,
> + GIT_HASH_UNKNOWN);
> the_repository->repository_format_worktree_config =
> repo_fmt.worktree_config;
> /* take ownership of repo_fmt.partial_clone */
> @@ -1657,6 +1659,7 @@ void check_repository_format(struct repository_format *fmt)
> check_repository_format_gently(get_git_dir(), fmt, NULL);
> startup_info->have_repository = 1;
> repo_set_hash_algo(the_repository, fmt->hash_algo);
> + repo_set_compat_hash_algo(the_repository, GIT_HASH_UNKNOWN);
> the_repository->repository_format_worktree_config =
> fmt->worktree_config;
> the_repository->repository_format_partial_clone =
> --
> 2.41.0
^ permalink raw reply
* Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too
From: Phillip Wood @ 2024-02-13 10:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Randall S. Becker
In-Reply-To: <xmqqsf1x486b.fsf@gitster.g>
On 12/02/2024 22:41, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>>> There is a larger clean-up opportunity to drop the need for making a
>>> copy, which probably is worth doing, so I folded the above into this
>>> version.
>>
>> Ooh, that's nice. This version looks good, I found the code comments
>> very helpful
>
> Thanks.
>
> Judging from https://github.com/git/git/actions/runs/7878254534/job/21496314393#step:5:142
> I do not seen to have broken Windows with this change, so let's
> fast-track and merge it down to 'master' before -rc1.
I think it was only the MSVC that needed the paths munging which we
don't test by default. I have tweaked our github actions to run those
tests and they pass
https://github.com/phillipwood/git/actions/runs/7885144920/job/21515922057#step:5:146
Best Wishes
Phillip
>> Best Wishes
>>
>> Phillip
>>
>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>> There are compilers other than Visual C that want to show absolute
>>> paths. Generalize the helper introduced by a2c5e294 (unit-tests: do
>>> show relative file paths, 2023-09-25) so that it can also work with
>>> a path that uses slash as the directory separator, and becomes
>>> almost no-op once one-time preparation finds out that we are using a
>>> compiler that already gives relative paths. Incidentally, this also
>>> should do the right thing on Windows with a compiler that shows
>>> relative paths but with backslash as the directory separator (if
>>> such a thing exists and is used to build git).
>>> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
>>> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>> * I found that the diff relative to the result of applying v1 was
>>> easier to follow than the range-diff, so here it is.
>>> diff --git c/t/unit-tests/test-lib.c w/t/unit-tests/test-lib.c
>>> index 83c9eb8c59..66d6980ffb 100644
>>> --- c/t/unit-tests/test-lib.c
>>> +++ w/t/unit-tests/test-lib.c
>>> @@ -64,34 +64,33 @@ static const char *make_relative(const char *location)
>>> * prefix_len == 0 if the compiler gives paths relative
>>> * to the root of the working tree. Otherwise, we want
>>> * to see that we did find the needle[] at a directory
>>> - * boundary.
>>> + * boundary. Again we rely on that needle[] begins with
>>> + * "t" followed by the directory separator.
>>> */
>>> if (fspathcmp(needle, prefix + prefix_len) ||
>>> - (prefix_len &&
>>> - prefix[prefix_len - 1] != '/' &&
>>> - prefix[prefix_len - 1] != '\\'))
>>> + (prefix_len && prefix[prefix_len - 1] != needle[1]))
>>> die("unexpected suffix of '%s'", prefix);
>>> -
>>> }
>>> /*
>>> - * If our compiler gives relative paths and we do not need
>>> - * to munge directory separator, we can return location as-is.
>>> + * Does it not start with the expected prefix?
>>> + * Return it as-is without making it worse.
>>> */
>>> - if (!prefix_len && !need_bs_to_fs)
>>> + if (prefix_len && fspathncmp(location, prefix, prefix_len))
>>> return location;
>>> - /* Does it not start with the expected prefix? */
>>> - if (fspathncmp(location, prefix, prefix_len))
>>> - return location;
>>> + /*
>>> + * If we do not need to munge directory separator, we can return
>>> + * the substring at the tail of the location.
>>> + */
>>> + if (!need_bs_to_fs)
>>> + return location + prefix_len;
>>> - strlcpy(buf, location + prefix_len, sizeof(buf));
>>> /* convert backslashes to forward slashes */
>>> - if (need_bs_to_fs) {
>>> - for (p = buf; *p; p++)
>>> - if (*p == '\\')
>>> - *p = '/';
>>> - }
>>> + strlcpy(buf, location + prefix_len, sizeof(buf));
>>> + for (p = buf; *p; p++)
>>> + if (*p == '\\')
>>> + *p = '/';
>>> return buf;
>>> }
>>> t/unit-tests/test-lib.c | 61
>>> +++++++++++++++++++++++++++++++----------
>>> 1 file changed, 47 insertions(+), 14 deletions(-)
>>> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
>>> index 7bf9dfdb95..66d6980ffb 100644
>>> --- a/t/unit-tests/test-lib.c
>>> +++ b/t/unit-tests/test-lib.c
>>> @@ -21,12 +21,11 @@ static struct {
>>> .result = RESULT_NONE,
>>> };
>>> -#ifndef _MSC_VER
>>> -#define make_relative(location) location
>>> -#else
>>> /*
>>> * Visual C interpolates the absolute Windows path for `__FILE__`,
>>> * but we want to see relative paths, as verified by t0080.
>>> + * There are other compilers that do the same, and are not for
>>> + * Windows.
>>> */
>>> #include "dir.h"
>>> @@ -34,32 +33,66 @@ static const char *make_relative(const char
>>> *location)
>>> {
>>> static char prefix[] = __FILE__, buf[PATH_MAX], *p;
>>> static size_t prefix_len;
>>> + static int need_bs_to_fs = -1;
>>> - if (!prefix_len) {
>>> + /* one-time preparation */
>>> + if (need_bs_to_fs < 0) {
>>> size_t len = strlen(prefix);
>>> - const char *needle = "\\t\\unit-tests\\test-lib.c";
>>> + char needle[] = "t\\unit-tests\\test-lib.c";
>>> size_t needle_len = strlen(needle);
>>> - if (len < needle_len || strcmp(needle, prefix + len -
>>> needle_len))
>>> - die("unexpected suffix of '%s'", prefix);
>>> + if (len < needle_len)
>>> + die("unexpected prefix '%s'", prefix);
>>> +
>>> + /*
>>> + * The path could be relative (t/unit-tests/test-lib.c)
>>> + * or full (/home/user/git/t/unit-tests/test-lib.c).
>>> + * Check the slash between "t" and "unit-tests".
>>> + */
>>> + prefix_len = len - needle_len;
>>> + if (prefix[prefix_len + 1] == '/') {
>>> + /* Oh, we're not Windows */
>>> + for (size_t i = 0; i < needle_len; i++)
>>> + if (needle[i] == '\\')
>>> + needle[i] = '/';
>>> + need_bs_to_fs = 0;
>>> + } else {
>>> + need_bs_to_fs = 1;
>>> + }
>>> - /* let it end in a directory separator */
>>> - prefix_len = len - needle_len + 1;
>>> + /*
>>> + * prefix_len == 0 if the compiler gives paths relative
>>> + * to the root of the working tree. Otherwise, we want
>>> + * to see that we did find the needle[] at a directory
>>> + * boundary. Again we rely on that needle[] begins with
>>> + * "t" followed by the directory separator.
>>> + */
>>> + if (fspathcmp(needle, prefix + prefix_len) ||
>>> + (prefix_len && prefix[prefix_len - 1] != needle[1]))
>>> + die("unexpected suffix of '%s'", prefix);
>>> }
>>> - /* Does it not start with the expected prefix? */
>>> - if (fspathncmp(location, prefix, prefix_len))
>>> + /*
>>> + * Does it not start with the expected prefix?
>>> + * Return it as-is without making it worse.
>>> + */
>>> + if (prefix_len && fspathncmp(location, prefix, prefix_len))
>>> return location;
>>> - strlcpy(buf, location + prefix_len, sizeof(buf));
>>> + /*
>>> + * If we do not need to munge directory separator, we can return
>>> + * the substring at the tail of the location.
>>> + */
>>> + if (!need_bs_to_fs)
>>> + return location + prefix_len;
>>> +
>>> /* convert backslashes to forward slashes */
>>> + strlcpy(buf, location + prefix_len, sizeof(buf));
>>> for (p = buf; *p; p++)
>>> if (*p == '\\')
>>> *p = '/';
>>> -
>>> return buf;
>>> }
>>> -#endif
>>> static void msg_with_prefix(const char *prefix, const char
>>> *format, va_list ap)
>>> {
^ permalink raw reply
* Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)
From: Phillip Wood @ 2024-02-13 11:07 UTC (permalink / raw)
To: Junio C Hamano, Xiaoguang WANG, Taylor Blau,
Torsten Bögershausen, Chandra Pratap
Cc: git
In-Reply-To: <xmqq8r3p7glr.fsf@gitster.g>
On 12/02/2024 17:11, Junio C Hamano wrote:
> Xiaoguang WANG <wxiaoguang@gmail.com> writes:
>
>> If GIT_FLUSH=true, it should mean to "do the flush". But that commit
>> made skip_stdout_flush=true when GIT_FLUSH=true.
>
> Thanks for reporting. I am surprised that this flipping of polarity
> slipped through.
>
>> And by the way, only accepting GIT_FLUSH=true is quite breaking, it
>> drops the compatibility of GIT_FLUSH=1
>
> I do not think so. If the polarity is corrected, git_env_bool()
> would say "that's affirmative" when any one of the "1", "true",
> "yes", "on", etc. is given. If you have been passing "1", you
> should get the "always flush" behaviour.
>
>
> Perhaps something like this would fix it?
>
>
> write-or-die.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git c/write-or-die.c w/write-or-die.c
> index 3942152865..3ecb9e2af5 100644
> --- c/write-or-die.c
> +++ w/write-or-die.c
> @@ -22,8 +22,11 @@ void maybe_flush_or_die(FILE *f, const char *desc)
>
> if (f == stdout) {
> if (skip_stdout_flush < 0) {
> - skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
> - if (skip_stdout_flush < 0) {
> + int flush_setting = git_env_bool("GIT_FLUSH", -1);
> +
> + if (0 <= flush_setting)
> + skip_stdout_flush = !flush_setting;
> + else {
> struct stat st;
> if (fstat(fileno(stdout), &st))
> skip_stdout_flush = 0;
Given we're in a rc-period a minimal fix like this looks appropriate
(though it is missing some braces according to our coding
guidelines). The interaction of "skip_stdout_flush" and git_env_bool()
is unfortunate, It might be clearer if we changed to having
"force_stdout_flush" instead but that would be a more invasive change.
Best Wishes
Phillip
diff --git a/write-or-die.c b/write-or-die.c
index 39421528653..e68265a94a6 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -18,20 +18,20 @@
*/
void maybe_flush_or_die(FILE *f, const char *desc)
{
- static int skip_stdout_flush = -1;
+ static int force_stdout_flush = -1;
if (f == stdout) {
- if (skip_stdout_flush < 0) {
- skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
- if (skip_stdout_flush < 0) {
+ if (force_stdout_flush < 0) {
+ force_stdout_flush = git_env_bool("GIT_FLUSH", -1);
+ if (force_stdout_flush < 0) {
struct stat st;
if (fstat(fileno(stdout), &st))
- skip_stdout_flush = 0;
+ force_stdout_flush = 1;
else
- skip_stdout_flush = S_ISREG(st.st_mode);
+ force_stdout_flush = !S_ISREG(st.st_mode);
}
}
- if (skip_stdout_flush && !ferror(f))
+ if (!force_stdout_flush && !ferror(f))
return;
}
if (fflush(f)) {
^ permalink raw reply related
* Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-13 13:14 UTC (permalink / raw)
To: Jean-Noël Avila, git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Michael Lohmann, Junio C Hamano
In-Reply-To: <7a2a0ed5-f9dc-42dd-886b-457641b9bc79@gmail.com>
Hi Jean-Nöel,
Le 2024-02-13 à 03:33, Jean-Noël Avila a écrit :
> Le 11/02/2024 à 00:35, Philippe Blain a écrit :
>> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>>
>>
>> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
>> index c2213bb77b..80ff4e149a 100644
>> --- a/Documentation/gitk.txt
>> +++ b/Documentation/gitk.txt
>> @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list.
>> --merge::
>> - After an attempt to merge stops with conflicts, show the commits on
>> - the history between two branches (i.e. the HEAD and the MERGE_HEAD)
>> - that modify the conflicted files and do not exist on all the heads
>> - being merged.
>> + Show commits touching conflicted paths in the range `HEAD...$OTHER`,
>
> if $OTHER is a placeholder, why not use the placeholder notation <other>
> instead of a notation that could deceive the reader into thinking that
> this is an actual environment variable?
Good point, I'll make that change.
Thanks!
Philippe.
^ permalink raw reply
* Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-13 13:27 UTC (permalink / raw)
To: phillip.wood, git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Patrick Steinhardt,
Michael Lohmann, Junio C Hamano
In-Reply-To: <c5d60b5b-3181-4bb7-a7f8-eb97474526d7@gmail.com>
Hi Phillip,
Le 2024-02-12 à 06:02, Phillip Wood a écrit :
> Hi Philippe
>
> On 10/02/2024 23:35, Philippe Blain wrote:
>> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>>
>> 'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...],
>> 2006-07-03) to show commits touching conflicted files in the range
>> HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document
>> rev-list's option --merge, 2006-08-04).
>>
>> It can be useful to look at the commit history to understand what lead
>> to merge conflicts also for other mergy operations besides merges, like
>> cherry-pick, revert and rebase.
>>
>> For rebases, an interesting range to look at is HEAD...REBASE_HEAD,
>> since the conflicts are usually caused by how the code changed
>> differently on HEAD since REBASE_HEAD forked from it.
>>
>> For cherry-picks and revert, it is less clear that
>> HEAD...CHERRY_PICK_HEAD and HEAD...REVERT_HEAD are indeed interesting
>> ranges, since these commands are about applying or unapplying a single
>> (or a few, for cherry-pick) commit(s) on top of HEAD. However, conflicts
>> encountered during these operations can indeed be caused by changes
>> introduced in preceding commits on both sides of the history.
>
> I tend to think that there isn't much difference between rebase and cherry-pick here - they are both cherry-picking commits and it is perfectly possible to rebase a branch onto an unrelated upstream. The important part for me is that we're showing these commits because even though they aren't part of the 3-way merge they are relevant for investigating where any merge conflicts come from.
>
> For revert I'd argue that the only sane use is reverting an ancestor of HEAD but maybe I'm missing something. In that case REVERT_HEAD...HEAD is the same as REVERT_HEAD..HEAD so it shows the changes since the commit that is being reverted which will be the ones causing the conflict.
Thanks, I can rework the wording from that angle.
>> Adjust the code in prepare_show_merge so it constructs the range
>> HEAD...$OTHER for each of OTHER={MERGE_HEAD, CHERRY_PICK_HEAD,
>> REVERT_HEAD or REBASE_HEAD}. Note that we try these pseudorefs in order,
>> so keep REBASE_HEAD last since the three other operations can be
>> performed during a rebase. Note also that in the uncommon case where
>> $OTHER and HEAD do not share a common ancestor, this will show the
>> complete histories of both sides since their root commits, which is the
>> same behaviour as currently happens in that case for HEAD and
>> MERGE_HEAD.
>>
>> Adjust the documentation of this option accordingly.
>
> Thanks for the comprehensive commit message.
>
>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
>> index 2bf239ff03..5b4672c346 100644
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -341,8 +341,10 @@ See also linkgit:git-reflog[1].
>> Under `--pretty=reference`, this information will not be shown at all.
>> --merge::
>> - After a failed merge, show refs that touch files having a
>> - conflict and don't exist on all heads to merge.
>> + Show commits touching conflicted paths in the range `HEAD...$OTHER`,
>> + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`,
>> + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
>> + when the index has unmerged entries.
>
> Do you know what "and don't exist on all heads to merge" in the original is referring to? The new text doesn't mention anything that sounds like that but I don't understand what the original was trying to say.
Yes, it took me a while to understand what that meant. I think it is simply
describing the range of commits shown. If we substitute "refs" for "commits"
and switch the order of the sentence, it reads:
After a failed merge, show commits that don't exist on all heads to merge
and that touch files having a conflict.
So it's just describing (a bit awkwardly) the HEAD...MERGE_HEAD range.
> It might be worth adding a sentence explaining when this option is useful.
>
> This option can be used to show the commits that are relevant
> when resolving conflicts from a 3-way merge
>
> or something like that.
Nice idea, I'll add that.
>
>> --boundary::
>> Output excluded boundary commits. Boundary commits are
>> diff --git a/revision.c b/revision.c
>> index aa4c4dc778..36dc2f94f7 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,
>> }
>> }
>> +static const char *lookup_other_head(struct object_id *oid)
>> +{
>> + int i;
>> + static const char *const other_head[] = {
>> + "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
>> + };
>> +
>> + for (i = 0; i < ARRAY_SIZE(other_head); i++)
>> + if (!read_ref_full(other_head[i],
>> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>> + oid, NULL)) {
>> + if (is_null_oid(oid))
>> + die("%s is a symbolic ref???", other_head[i]);
>
> This would benefit from being translated and I think one '?' would suffice (I'm not sure we even need that - are there other possible causes of a null oid here?)
This bit was suggested by Junio upthread in <xmqqzfxa9usx.fsf@gitster.g>.
I'm not sure if the are other causes of null oid, as I don't know well this
part of the code.
I agree that a single '?' would be enough, but I'm not sure about marking
this for translation, I think maybe this situation would be best handled with
BUG() ?
>> + return other_head[i];
>> + }
>> +
>> + die("--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?");
>
> This is not a question and would also benefit from translation. It might be more helpful to say that "--merge" requires one of those pseudorefs.
Yes, I agree. I'll tweak that.
> Thanks for pick this series up and polishing it
>
> Phillip
>
Thanks,
Philippe.
^ permalink raw reply
* RE: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: rsbecker @ 2024-02-13 14:36 UTC (permalink / raw)
To: 'Jeff King', 'Junio C Hamano'
Cc: 'Josh Steadmon', git, johannes.schindelin, phillip.wood
In-Reply-To: <20240213074118.GA2225494@coredump.intra.peff.net>
On Tuesday, February 13, 2024 2:41 AM, Peff wrote:
>On Mon, Feb 12, 2024 at 01:27:11PM -0800, Junio C Hamano wrote:
>
>> Josh Steadmon <steadmon@google.com> writes:
>>
>> > I see this line in the docs [1]: "As with wildcard expansion in
>> > rules, the results of the wildcard function are sorted". GNU Make
>> > has restored the sorted behavior of $(wildcard) since 2018 [2]. I'll
>> > leave the sort off for now, but if folks feel like we need to
>> > support older versions of `make`, I'll add it back.
>> >
>> > [1]
>> > https://www.gnu.org/software/make/manual/html_node/Wildcard-Function
>> > .html [2] https://savannah.gnu.org/bugs/index.php?52076
>>
>> Thanks for digging. I thought I was certain that woldcard is sorted
>> and stable and was quite perplexed when I could not find the mention
>> in a version of doc I had handy ("""This is Edition 0.75, last updated
>> 19 January 2020, of 'The GNU Make Manual', for GNU 'make'
>> version 4.3.""").
>
>Likewise (mine is the latest version in Debian unstable). The change to sort comes
>from their[1] eedea52a, which was in GNU make 4.2.90. But the matching
>documentation change didn't happen until 5b993ae, which was
>4.3.90 in late 2021. So that explains the mystery.
>
>Those dates imply to me that we should keep the $(sort), though. Six years is not so
>long in distro timescales, especially given that Debian unstable is on a 4-year-old
>version. (And if we did want to get rid of it, certainly we should do so consistently
>across the Makefile in a separate patch).
I am stuck on 4.2.1 and cannot get to 4.3.90 any time soon. Can you want on this? It will take us out unless we can suppress the $(sort)
Sincerely,
Randall
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox