* [PATCH v3 03/11] Start reporting missing commits in `repo_in_merge_bases_many()`
From: Johannes Schindelin via GitGitGadget @ 2024-02-27 13:28 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.v3.git.1709040497.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.
Note: As of this patch, errors are returned only if any of the specified
merge heads is missing. Over the course of the next patches, missing
commits will also be reported by the `paint_down_to_common()` function,
which is called by `repo_in_merge_bases_many()`, and those errors will
be properly propagated back to the caller at that stage.
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 | 8 +++--
http-push.c | 5 ++-
merge-ort.c | 81 ++++++++++++++++++++++++++++++++++++------
merge-recursive.c | 54 +++++++++++++++++++++++-----
shallow.c | 18 ++++++----
12 files changed, 173 insertions(+), 36 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 5c1b5256598..5ff71d72d51 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -464,11 +464,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, 0))
- return 1;
+ ret = repo_in_merge_bases_many(r, other, 1, &commit, 0);
+ if (ret)
+ return ret;
}
return 0;
}
@@ -598,6 +600,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..24c16a4f5ff 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 ret = repo_in_merge_bases(the_repository, branch, head);
- return repo_in_merge_bases(the_repository, branch, head);
+ if (ret < 0)
+ exit(128);
+ return ret;
}
static int delete_remote_branch(const char *pattern, int force)
diff --git a/merge-ort.c b/merge-ort.c
index 6491070d965..9f3af46333a 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,14 @@ 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) {
+ object_array_clear(&merges);
+ release_revisions(&revs);
+ return ret;
+ }
+ if (ret > 0)
add_object_array(o, NULL, &merges);
}
reset_revision_walk();
@@ -1725,9 +1735,17 @@ 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) {
+ object_array_clear(&merges);
+ release_revisions(&revs);
+ return ret;
+ }
+ if (ret > 0) {
+ contains_another = 1;
+ break;
+ }
}
}
@@ -1749,7 +1767,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 +1814,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 < 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)
+ ret2 = repo_in_merge_bases(&subrepo, commit_o, 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) {
path_msg(opt, CONFLICT_SUBMODULE_MAY_HAVE_REWINDS, 0,
path, NULL, NULL, NULL,
_("Failed to merge submodule %s "
@@ -1807,7 +1843,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 +1861,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 +1895,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..0d931cc14ad 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1144,7 +1144,13 @@ 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) {
+ object_array_clear(&merges);
+ release_revisions(&revs);
+ return ret;
+ }
+ if (ret)
add_object_array(o, NULL, &merges);
}
reset_revision_walk();
@@ -1159,9 +1165,17 @@ 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) {
+ object_array_clear(&merges);
+ release_revisions(&revs);
+ return ret;
+ }
+ if (ret > 0) {
+ contains_another = 1;
+ break;
+ }
}
}
@@ -1197,7 +1211,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 +1248,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 > 0)
+ 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 +1283,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 +1436,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 dfcc1f86a7f..f71496f35c3 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, 1)) {
- 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, 1);
+ if (ret < 0)
+ exit(128);
+ if (!ret) {
+ update_refstatus(ref_status, info->ref->nr, *bitmap);
+ dst++;
+ break;
+ }
}
}
info->nr_ours = dst;
@@ -830,6 +834,8 @@ int delayed_reachability_test(struct shallow_info *si, int c)
si->nr_commits,
si->commits,
1);
+ if (si->reachable[c] < 0)
+ exit(128);
si->need_reachability_test[c] = 0;
}
return si->reachable[c];
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 04/11] Prepare `paint_down_to_common()` for handling shallow commits
From: Johannes Schindelin via GitGitGadget @ 2024-02-27 13:28 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.v3.git.1709040497.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 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").
Note: shallow commits' parents are set to `NULL` internally already,
therefore there is no need to special-case shallow repositories here, as
the merge-base logic will not try to access parent commits of shallow
commits.
Likewise, partial clones aren't an issue either: If a commit is missing
during the revision walk in the merge-base logic, it is fetched via
`promisor_remote_get_direct()`. And not only the single missing commit
object: Due to the way the "promised" objects are fetched (in
`fetch_objects()` in `promisor-remote.c`, using `fetch
--filter=blob:none`), there is no actual way to fetch a single commit
object, as the remote side will pass that commit OID to `pack-objects
--revs [...]` which in turn passes it to `rev-list` which interprets
this as a commit _range_ instead of a single object. Therefore, in
partial clones (unless they are shallow in addition), all commits
reachable from a commit that is in the local object database are also
present in that local database.
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 5ff71d72d51..7112b10eeea 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;
@@ -108,6 +109,13 @@ static struct commit_list *paint_down_to_common(struct repository *r,
if (repo_parse_commit(r, p)) {
clear_prio_queue(&queue);
free_commit_list(result);
+ /*
+ * 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()`.
+ */
return NULL;
}
p->object.flags |= flags;
@@ -143,7 +151,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);
@@ -214,7 +222,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++)
@@ -504,7 +512,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 v3 02/11] Prepare `repo_in_merge_bases_many()` to optionally expect missing commits
From: Johannes Schindelin via GitGitGadget @ 2024-02-27 13:28 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.v3.git.1709040497.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 commit changes behavior slightly: unless called from the
`shallow.c` functions that set the `ignore_missing_commits` bit, any
non-existing tip commit that is passed to `repo_in_merge_bases_many()`
will now result in an error.
Note: When encountering missing commits while traversing the commit
history in search for merge bases, with this commit there won't be a
change in behavior just yet, their children will still be interpreted as
root commits. This bug will get fixed by follow-up commits.
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 7ea916f9ebd..5c1b5256598 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;
- if (repo_in_merge_bases_many(r, other, 1, &commit))
+ if (repo_in_merge_bases_many(r, other, 1, &commit, 0))
return 1;
}
return 0;
@@ -478,17 +478,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 ret;
+ return ignore_missing_commits ? 0 : -1;
for (i = 0; i < nr_reference; i++) {
if (repo_parse_commit(r, reference[i]))
- return ret;
+ 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 ac728cdd778..dfcc1f86a7f 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 */
- !repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits)) {
+ !repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits, 1)) {
update_refstatus(ref_status, info->ref->nr, *bitmap);
dst++;
break;
@@ -828,7 +828,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);
si->need_reachability_test[c] = 0;
}
return si->reachable[c];
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 v3 01/11] paint_down_to_common: plug two memory leaks
From: Johannes Schindelin via GitGitGadget @ 2024-02-27 13:28 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1657.v3.git.1709040497.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.
The priority queue has a similar issue: There might still be a commit in
that queue.
Let's release both, to address the potential memory leaks.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
commit-reach.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/commit-reach.c b/commit-reach.c
index a868a575ea1..7ea916f9ebd 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -105,8 +105,11 @@ 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)) {
+ clear_prio_queue(&queue);
+ free_commit_list(result);
return NULL;
+ }
p->object.flags |= flags;
prio_queue_put(&queue, p);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 00/11] The merge-base logic vs missing commit objects
From: Johannes Schindelin via GitGitGadget @ 2024-02-27 13:28 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <pull.1657.v2.git.1708608110.gitgitgadget@gmail.com>
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 lean so heavily on reviews on the Git mailing list.
Changes since v2:
* Moved a hunk from 3/11 to 2/11 that lets the repo_in_merge_bases_many()
function return an error if non-existing commits have been passed to it,
unless the ignore_missing_commits parameter is non-zero.
* The end result is tree-same.
Changes since v1:
* Addressed a lot of memory leaks.
* Reordered patch 2 and 3 to render the commit message's comment about
unchanged behavior true.
* Fixed the incorrectly converted condition in the merge_submodule()
function.
* The last patch ("paint_down_to_common(): special-case shallow/partial
clones") was dropped because it is not actually necessary, and the
explanation for that was added to the commit message of "Prepare
paint_down_to_common() for handling shallow commits".
* An inconsistently-named variable i was renamed to be consistent with the
other variables called ret.
Johannes Schindelin (11):
paint_down_to_common: plug two memory leaks
Prepare `repo_in_merge_bases_many()` to optionally expect missing
commits
Start reporting missing commits in `repo_in_merge_bases_many()`
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()`
bisect.c | 7 +-
builtin/branch.c | 12 +-
builtin/fast-import.c | 6 +-
builtin/fetch.c | 2 +
builtin/log.c | 30 +++--
builtin/merge-base.c | 23 +++-
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 | 209 ++++++++++++++++++++-----------
commit-reach.h | 26 ++--
commit.c | 7 +-
diff-lib.c | 5 +-
http-push.c | 5 +-
log-tree.c | 5 +-
merge-ort.c | 87 +++++++++++--
merge-recursive.c | 58 +++++++--
notes-merge.c | 3 +-
object-name.c | 7 +-
remote.c | 2 +-
revision.c | 12 +-
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, 441 insertions(+), 183 deletions(-)
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1657%2Fdscho%2Fmerge-base-and-missing-objects-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1657/dscho/merge-base-and-missing-objects-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1657
Range-diff vs v2:
1: 6e4e409cd43 = 1: 6e4e409cd43 paint_down_to_common: plug two memory leaks
2: 5c16becfb9b ! 2: f68d77c6123 Prepare `repo_in_merge_bases_many()` to optionally expect missing commits
@@ Commit message
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.
+ This commit changes behavior slightly: unless called from the
+ `shallow.c` functions that set the `ignore_missing_commits` bit, any
+ non-existing tip commit that is passed to `repo_in_merge_bases_many()`
+ will now result in an error.
+
+ Note: When encountering missing commits while traversing the commit
+ history in search for merge bases, with this commit there won't be a
+ change in behavior just yet, their children will still be interpreted as
+ root commits. This bug will get fixed by follow-up commits.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@@ commit-reach.c: int repo_is_descendant_of(struct repository *r,
{
struct commit_list *bases;
int ret = 0, i;
+ timestamp_t generation, max_generation = GENERATION_NUMBER_ZERO;
+
+ if (repo_parse_commit(r, commit))
+- return ret;
++ return ignore_missing_commits ? 0 : -1;
+ for (i = 0; i < nr_reference; i++) {
+ if (repo_parse_commit(r, reference[i]))
+- return ret;
++ return ignore_missing_commits ? 0 : -1;
+
+ generation = commit_graph_generation(reference[i]);
+ if (generation > max_generation)
## commit-reach.h ##
@@ commit-reach.h: int repo_in_merge_bases(struct repository *r,
3: 4dd214f91d4 ! 3: 0aaa224b5db Start reporting missing commits in `repo_in_merge_bases_many()`
@@ commit-reach.c: int repo_is_descendant_of(struct repository *r,
}
return 0;
}
-@@ commit-reach.c: 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 ignore_missing_commits ? 0 : -1;
- for (i = 0; i < nr_reference; i++) {
- if (repo_parse_commit(r, reference[i]))
-- return ret;
-+ return ignore_missing_commits ? 0 : -1;
-
- generation = commit_graph_generation(reference[i]);
- if (generation > max_generation)
@@ commit-reach.c: 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,
4: 53bdeddb4cb = 4: 84e7fbc07e0 Prepare `paint_down_to_common()` for handling shallow commits
5: ec3ebf0ed17 = 5: 85332b58c37 commit-reach: start reporting errors in `paint_down_to_common()`
6: 05756fbf71a = 6: 2ae6a54dd59 merge_bases_many(): pass on errors from `paint_down_to_common()`
7: e3d37a326e5 = 7: 4321795102d get_merge_bases_many_0(): pass on errors from `merge_bases_many()`
8: 9ca504525b9 = 8: 35545c4b777 repo_get_merge_bases(): pass on errors from `merge_bases_many()`
9: b11879edb73 = 9: a963058d2ba get_octopus_merge_bases(): pass on errors from `merge_bases_many()`
10: 602a7383f72 = 10: c3bed9c8500 repo_get_merge_bases_many(): pass on errors from `merge_bases_many()`
11: 96850ed2d69 = 11: bdbf47ae505 repo_get_merge_bases_many_dirty(): pass on errors from `merge_bases_many()`
--
gitgitgadget
^ permalink raw reply
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: phillip.wood123 @ 2024-02-27 10:43 UTC (permalink / raw)
To: Patrick Steinhardt, phillip.wood
Cc: rsbecker, 'Torsten Bögershausen', git
In-Reply-To: <Zd2hMmIzHKQ7JE45@tanuki>
Hi Patrick
On 27/02/2024 08:45, Patrick Steinhardt wrote:
> On Mon, Feb 26, 2024 at 03:32:14PM +0000, Phillip Wood wrote:
>>>>> reftable/writer.c: int n = w->write(w->write_arg, zeroed,
>>>>> w->pending_padding);
>>>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>>
>> Neither of these appear to check for short writes and reftable_fd_write() is
>> a thin wrapper around write(). Maybe reftable_fd_write() should be using
>> write_in_full()?
>
> It already does starting with 85a8c899ce (reftable: handle interrupted
> writes, 2023-12-11):
>
> ```
> static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
> {
> int *fdp = (int *)arg;
> return write_in_full(*fdp, data, sz);
> }
> ```
Oh, the branch I had checkout out was older than I realized, sorry for
the confusion.
Best Wishes
Phillip
^ permalink raw reply
* Re: Interactive rebase: using "pick" for merge commits
From: phillip.wood123 @ 2024-02-27 10:41 UTC (permalink / raw)
To: Stefan Haller, phillip.wood, Patrick Steinhardt; +Cc: git
In-Reply-To: <6a557891-ffcd-4c42-9768-ec2da0fce92a@haller-berlin.de>
On 26/02/2024 19:07, Stefan Haller wrote:
> On 26.02.24 11:56, Phillip Wood wrote:
>>> It probably makes more sense to teach lazygit to visualize the
>>> .git/sequencer/todo file, and then use git cherry-pick.
>>
>> If lazygit is generating the todo list for the cherry-pick could it
>> check if the commit is a merge and insert "exec cherry-pick -m ..." for
>> those commits?
>
> That's a good idea, but it wouldn't buy us very much. We'd still have to
> add support for conflicts during a cherry-pick; when there's a conflict
> during a rebase, lazygit has this nice visualization of the conflicting
> commit (we talked about that in [1], and it turned out to be working
> extremely well), so it would have to learn to do the same thing for a
> conflicting cherry-pick (although this does seem to be a lot easier).
> And then it would have to learn to call "cherry-pick --continue" rather
> than "rebase --continue" after resolving. But if we do all these things,
> then we're not so far away from being able to just call git cherry-pick
> ourselves.
Oh I'd forgotten about handling conflicts - that does make my proposal
less attractive.
Best Wishes
Phillip
> -Stefan
>
> [1] <https://public-inbox.org/git/
> 961e68d7-5f43-c385-10fa-455b8e2f32d0@haller-berlin.de/>
^ permalink raw reply
* Re: [PATCH v2 8/8] cherry-pick: add `--empty` for more robust redundant commit handling
From: phillip.wood123 @ 2024-02-27 10:39 UTC (permalink / raw)
To: Brian Lyles, phillip.wood, gitster; +Cc: git, newren, me
In-Reply-To: <17b74c2ffa1884ed.70b1dd9aae081c6e.203dcd72f6563036@zivdesk>
Hi Brian
On 26/02/2024 03:32, Brian Lyles wrote:
> Hi Phillip and Junio
> On Fri, Feb 23, 2024 at 12:08 AM Brian Lyles <brianmlyles@gmail.com> wrote:
>>
>> On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>>> I agree that if we were starting from scratch there would be no reason
>>> to tie --apply-empty and --keep-redundant-commits together but I'm not
>>> sure it is worth the disruption of changing it now. We're about to add
>>> empty=keep which won't imply --allow-empty for anyone who wants that
>>> behavior and I still tend to think the practical effect of implying
>>> --allow-empty with --keep-redundant-commits is largely beneficial as I'm
>>> skeptical that users want to keep commits that become empty but not the
>>> ones that started empty.
>>
>> I think that's fair. I am okay dropping this potentially disruptive
>> change.
>>
>> It sounds like you are on board with `--empty=keep` not having the same
>> implication?
Yes indeed
>> That said...
>>
>> On Thu, Feb 22, 2024 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> I do not quite see a good reason to do the opposite, dropping
>>> commits that started out as empty but keeping the ones that have
>>> become empty. Such a behaviour has additional downside that after
>>> such a cherry-pick, when you cherry-pick the resulting history onto
>>> yet another base, your precious "were not empty but have become so
>>> during the initial cherry-pick" commits will appear as commits that
>>> were empty from the start. So I do not see much reason to allow the
>>> decoupling, even with the new "empty=keep" thing that does not imply
>>> "allow-empty".
>>
>> Junio -- can you clarify this part?
>>
>>> So I do not see much reason to allow the decoupling, even with the new
>>> "empty=keep" thing that does not imply "allow-empty"
>>
>> I'm not 100% sure if you are saying that you want `--empty=keep` to
>> *also* imply `--allow-empty`, or that you simply want
>> `--keep-redundant-commits` to continue implying `--allow-empty`
>> *despite* the new `--empty=keep` no implying the same.
FWIW I read it as the latter, but I can't claim to know what was in
Junio's mind when he wrote it.
>> On the one hand, I agree with Phillip's sentiment of "if we were
>> starting from scratch there would be no reason to tie --apply-empty and
>> --keep-redundant-commits together" (though your points perhaps provide
>> such a reason). On the other, if both `--keep-redundant-commits` and
>> `--empty=keep` behave the same way, it makes sense to soft-deprecate
>> `--keep-redundant-commits` as I have currently done later in this
>> series. If they do not behave the same way, that deprecation makes less
>> sense and we have two very similar (but not quite identical) options.
>>
>> Just to make sure we're on the same page, the options I see are:
>>
>> - (A): Neither `--keep-redundant-commits` nor `--empty=keep` imply
>> `--allow-empty`, `--keep-redundant-commits` is soft-deprecated
>> - (B): Both `--keep-redundant-commits` and `--empty=keep` imply
>> `--allow-empty`, `--keep-redundant-commits` is soft-deprecated
>> - (C): Both `--keep-redundant-commits` and `--empty=keep` imply
>> `--allow-empty`, `--keep-redundant-commits` is *not* soft-deprecated
>> as it is more descriptive as noted by Junio here[1]
>> - (D): `--keep-redundant-commits` continues to imply `--allow-empty` but
>> `--empty=keep` does not. `--keep-redundant-commits` is *not*
>> soft-deprecated as it behaves differently.
>>
>> (A) represents this v2 of the patch.
>>
>> I'm coming around to (B) based on Junio's workflow concerns, but to be
>> honest I am fine with any of these options. Junio, I *think* you're
>> saying you'd prefer (B) or (C)? Phillip, it sounds like you are okay
>> with (D) based on your response -- how do you feel about (B)?
Yes, I'd prefer (D) as I think it gets confusing if some values of
--empty imply --allow-empty and others don't. I could live with (B) though.
Best Wishes
Phillip
^ permalink raw reply
* Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string
From: Christian Couder @ 2024-02-27 10:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Achu Luma, git, Christian Couder
In-Reply-To: <xmqq7cirnlkg.fsf@gitster.g>
On Mon, Feb 26, 2024 at 7:10 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > Unfortunately strbuf_add() calls strbuf_grow() itself which is not
> > needed as we have already called strbuf_grow() to avoid repeated
> > reallocation.
>
> Why is it unfortunate? If the current allocation is sufficient, it
> is a cheap no-op, isn't it (if not, we should make it so)?
Yeah, I agree that we don't need to be extra efficient and calling
strbuf_grow() doesn't add much overhead. So let's just use
strbuf_add() repeatedly after calling it once.
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: Christian Couder @ 2024-02-27 10:04 UTC (permalink / raw)
To: Josh Steadmon, René Scharfe, Christian Couder, git,
Phillip Wood, Achu Luma
In-Reply-To: <ZdzfYPim2SP22eeS@google.com>
On Mon, Feb 26, 2024 at 7:58 PM Josh Steadmon <steadmon@google.com> wrote:
>
> On 2024.02.26 18:26, René Scharfe wrote:
> > The output is clean as well, but there's a lot of it. Perhaps too much.
> > The success messages are boring, though, and if all checks pass then the
> > only useful information is the status code. A TAP harness like prove
> > summarizes that nicely:
> >
> > $ prove t/unit-tests/bin/t-ctype
> > t/unit-tests/bin/t-ctype .. ok
> > All tests successful.
> > Files=1, Tests=3598, 0 wallclock secs ( 0.08 usr + 0.00 sys = 0.08 CPU)
> > Result: PASS
> >
> > Filtering out passing checks e.g. with "| grep -v ^ok" would help when
> > debugging a test failure. I vaguely miss the --immediate switch from the
> > regular test library, however.
>
> Yeah, I agree here. It's a lot of output but it's almost always going to
> be consumed by a test harness rather than a human, and it's easy to
> filter out the noise if someone does need to do some manual debugging.
Yeah, I know about TAP harnesses like prove, but the most
straightforward way to run the unit tests is still `make unit-tests`
in the t/ directory. Also when you add or change some tests, it's a
good idea to run `make unit-tests` to see what the output is, so you
still have to see that output quite often when you work on tests and
going through 3598 of mostly useless output instead of just 14 isn't
nice.
^ permalink raw reply
* Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests
From: Christian Couder @ 2024-02-27 9:59 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <CZF8YROS9RVC.9H2EKYCF08VK@gmail.com>
On Mon, Feb 26, 2024 at 8:11 PM Ghanshyam Thakkar
<shyamthakkar001@gmail.com> wrote:
>
> On Mon Feb 26, 2024 at 8:41 PM IST, Christian Couder wrote:
> > So I think it would be better to work on other things instead, like
> > perhaps reviewing other people's work or working on other bug fixes or
> > features. Anyway now that this is on the mailing list, I might as well
> > review it as it could help with your application. But please consider
> > working on other things.
>
> I understand and will work on other things.
Thanks!
> > > In unit testing however, we do not
> > > need to initialize the repo. We can set the length of the hexadecimal
> > > strbuf according to the algorithm used directly.
> >
> > So is your patch doing that or not? It might be better to be explicit.
> > Also if 'strbuf's are used, then is it really worth it to set their
> > length in advance, instead of just letting them grow to the right
> > length as we add hex to them?
>
> I thought of it like this: If we were to just let them grow, then we
> would need separate logic for reusing that strbuf or use a different
> one everytime since it always grows. By separating allocation
> (hex_strbuf_init) and manipulation (fill_hex_strbuf), that same strbuf
> can be reused for different hex values.
>
> But, none of the test currently need to reuse the same strbuf, so I
> suppose it is better to just let it grow and even if the need arises we
> can use strbuf_splice().
It's not a problem to use a new strbuf for each different hex value.
Tests don't need a lot of performance as they are used mostly by
developers, not by everyone using Git. Also if you want to reuse a
strbuf, you can just use strbuf_reset() on it and then reuse it.
^ permalink raw reply
* [PATCH v2 2/2] commit: Unify logic to avoid multiple scissors lines when merging
From: Josh Triplett @ 2024-02-27 9:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <4f97933f173220544a5be2bf05c2bee2b044d2b1.1709024540.git.josh@joshtriplett.org>
prepare_to_commit has some logic to figure out whether merge already
added a scissors line, and therefore it shouldn't add another. Now that
wt_status_add_cut_line has built-in state for whether it has
already added a previous line, just set that state instead, and then
remove that condition from subsequent calls to wt_status_add_cut_line.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v2: New patch.
builtin/commit.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index e0a6d43179..142f54ea7c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -737,7 +737,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
const char *hook_arg2 = NULL;
int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
int old_display_comment_prefix;
- int merge_contains_scissors = 0;
int invoked_hook;
/* This checks and barfs if author is badly specified */
@@ -841,7 +840,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
wt_status_locate_end(sb.buf + merge_msg_start,
sb.len - merge_msg_start) <
sb.len - merge_msg_start)
- merge_contains_scissors = 1;
+ s->added_cut_line = 1;
} else if (!stat(git_path_squash_msg(the_repository), &statbuf)) {
if (strbuf_read_file(&sb, git_path_squash_msg(the_repository), 0) < 0)
die_errno(_("could not read SQUASH_MSG"));
@@ -924,8 +923,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
" yourself if you want to.\n"
"An empty message aborts the commit.\n");
if (whence != FROM_COMMIT) {
- if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
- !merge_contains_scissors)
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
wt_status_add_cut_line(s);
status_printf_ln(
s, GIT_COLOR_NORMAL,
@@ -946,7 +944,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)
status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_all, comment_line_char);
else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
- if (whence == FROM_COMMIT && !merge_contains_scissors)
+ if (whence == FROM_COMMIT)
wt_status_add_cut_line(s);
} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_space, comment_line_char);
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/2] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Josh Triplett @ 2024-02-27 9:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <Zd2eLxPelxvP8FDk@localhost>
`git commit --cleanup=scissors -v` currently prints two scissors lines:
one at the start of the comment lines, and the other right before the
diff. This is redundant, and pushes the diff further down in the user's
editor than it needs to be.
Make wt_status_add_cut_line remember if it has added a cut line before,
and avoid adding a redundant one.
Add a test for this.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
v2: Make wt_status_add_cut_line remember if it has added a cut line,
rather than making later callers try to figure out if it has been called
before. Add a test.
Note that other parts of the code do already try to figure out if the
*merge* logic has added scissors already, which is where
merge_contains_scissors comes from. Patch 2/2 unifies that machinery.
builtin/commit.c | 4 ++--
t/t7502-commit-porcelain.sh | 5 +++++
wt-status.c | 12 ++++++++----
wt-status.h | 3 ++-
4 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 6d1fa71676..e0a6d43179 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -926,7 +926,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
if (whence != FROM_COMMIT) {
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
!merge_contains_scissors)
- wt_status_add_cut_line(s->fp);
+ wt_status_add_cut_line(s);
status_printf_ln(
s, GIT_COLOR_NORMAL,
whence == FROM_MERGE ?
@@ -947,7 +947,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_all, comment_line_char);
else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
if (whence == FROM_COMMIT && !merge_contains_scissors)
- wt_status_add_cut_line(s->fp);
+ wt_status_add_cut_line(s);
} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_space, comment_line_char);
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index a87c211d0b..b37e2018a7 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -736,6 +736,11 @@ test_expect_success 'message shows date when it is explicitly set' '
.git/COMMIT_EDITMSG
'
+test_expect_success 'message does not have multiple scissors lines' '
+ git commit --cleanup=scissors -v --allow-empty -e -m foo &&
+ test $(grep -c -e "--- >8 ---" .git/COMMIT_EDITMSG) -eq 1
+'
+
test_expect_success AUTOIDENT 'message shows committer when it is automatic' '
echo >>negative &&
diff --git a/wt-status.c b/wt-status.c
index ea13f5d8db..2d576f7a44 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1108,12 +1108,15 @@ void wt_status_append_cut_line(struct strbuf *buf)
strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
}
-void wt_status_add_cut_line(FILE *fp)
+void wt_status_add_cut_line(struct wt_status *s)
{
struct strbuf buf = STRBUF_INIT;
+ if (s->added_cut_line)
+ return;
+ s->added_cut_line = 1;
wt_status_append_cut_line(&buf);
- fputs(buf.buf, fp);
+ fputs(buf.buf, s->fp);
strbuf_release(&buf);
}
@@ -1144,11 +1147,12 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
* file (and even the "auto" setting won't work, since it
* will have checked isatty on stdout). But we then do want
* to insert the scissor line here to reliably remove the
- * diff before committing.
+ * diff before committing, if we didn't already include one
+ * before.
*/
if (s->fp != stdout) {
rev.diffopt.use_color = 0;
- wt_status_add_cut_line(s->fp);
+ wt_status_add_cut_line(s);
}
if (s->verbose > 1 && s->committable) {
/* print_updated() printed a header, so do we */
diff --git a/wt-status.h b/wt-status.h
index 819dcad723..5e99ba4707 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -130,6 +130,7 @@ struct wt_status {
int rename_score;
int rename_limit;
enum wt_status_format status_format;
+ unsigned char added_cut_line; /* boolean */
struct wt_status_state state;
struct object_id oid_commit; /* when not Initial */
@@ -147,7 +148,7 @@ struct wt_status {
size_t wt_status_locate_end(const char *s, size_t len);
void wt_status_append_cut_line(struct strbuf *buf);
-void wt_status_add_cut_line(FILE *fp);
+void wt_status_add_cut_line(struct wt_status *s);
void wt_status_prepare(struct repository *r, struct wt_status *s);
void wt_status_print(struct wt_status *s);
void wt_status_collect(struct wt_status *s);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Jeff King @ 2024-02-27 9:05 UTC (permalink / raw)
To: Kyle Lippincott
Cc: Sven Strickroth, Junio C Hamano, Calvin Wan, git, Jonathan Tan,
phillip.wood123
In-Reply-To: <20240227084529.GJ3263678@coredump.intra.peff.net>
On Tue, Feb 27, 2024 at 03:45:29AM -0500, Jeff King wrote:
> It is good to clean up old conditionals if they are no longer
> applicable, as they are a burden to reason about later (as this
> discussion shows). But I am not sure about your "just to demonstrate we
> can". It is good to try it out, but it looks like there is a non-zero
> chance that MSVC on Windows might break. It is probably better to try
> building there or looping in folks who can, rather than just making a
> change and seeing if anybody screams.
>
> I think the "win+VS" test in the GitHub Actions CI job might cover this
> case. It is not run by default (because it was considered be mostly
> redundant with the mingw build), but it shouldn't be too hard to enable
> it for a one-off test.
Here's a successful run with the NO_INTTYPES_H line removed:
https://github.com/peff/git/actions/runs/8062063219
Blaming the code around the inttypes.h reference in compat/mingw.h
turned up 0ef60afdd4 (MSVC: use shipped headers instead of fallback
definitions, 2016-03-30), which claims that inttypes.h was added to
VS2013.
That sounds old-ish in 2024, but I don't know how old is normal in the
Windows world.
All of which to me says that cleaning this up is something that should
involve Windows folks, who can make the judgement for their platform.
-Peff
^ permalink raw reply
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: Patrick Steinhardt @ 2024-02-27 8:45 UTC (permalink / raw)
To: phillip.wood; +Cc: rsbecker, 'Torsten Bögershausen', git
In-Reply-To: <5e807c1c-20a0-407b-9fc2-acd38521ba45@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 866 bytes --]
On Mon, Feb 26, 2024 at 03:32:14PM +0000, Phillip Wood wrote:
> Hi Randal
>
> [cc'ing Patrick for the reftable writer]
>
> > The question is which call is bad? The cruft stuff is
> > relatively new and I don't know the code.
> >
> > > > reftable/writer.c: int n = w->write(w->write_arg, zeroed,
> > > > w->pending_padding);
> > > > reftable/writer.c: n = w->write(w->write_arg, data, len);
>
> Neither of these appear to check for short writes and reftable_fd_write() is
> a thin wrapper around write(). Maybe reftable_fd_write() should be using
> write_in_full()?
It already does starting with 85a8c899ce (reftable: handle interrupted
writes, 2023-12-11):
```
static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
{
int *fdp = (int *)arg;
return write_in_full(*fdp, data, sz);
}
```
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Jeff King @ 2024-02-27 8:45 UTC (permalink / raw)
To: Kyle Lippincott
Cc: Junio C Hamano, Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <CAO_smVjeNYFTgh4MZjRM9U1coY=UJxo-E6bD9OfdS1A1Xf6vcQ@mail.gmail.com>
On Mon, Feb 26, 2024 at 04:56:28PM -0800, Kyle Lippincott wrote:
> > We use inttypes.h by default because the C standard already talks
> > about it, and fall back to stdint.h when the platform lacks it. But
> > what I suspect is that nobody compiles with NO_INTTYPES_H and we
> > would unknowingly (but not "unintentionally") start using the
> > extended types that are only available in <inttypes.h> but not in
> > <stdint.h> sometime in the future. It might already have happened,
>
> It has. We use PRIuMAX, which is from inttypes.h.
Is it always, though? That's what C99 says, but if you have a system
that does not have inttypes.h in the first place, but does have
stdint.h, it seems possible that it provides conversion macros elsewhere
(either via stdint.h, or possibly just as part of stdio.h).
So it might be that things have been horribly broken on NO_INTTYPES_H
systems for a while, and nobody is checking. But I don't think you can
really say so without looking at such a system.
And looking at config.mak.uname, it looks like Windows is such a system.
Does it really have inttypes.h and it is getting included from somewhere
else, making format conversion macros work? Or does it provide those
macros elsewhere, and really needs stdint? It does look like
compat/mingw.h includes it, but I think we wouldn't use that for msvc
builds.
> I think it's only
> "accidentally" working if anyone uses NO_INTTYPES_H. I changed my
> stance halfway through this investigation in my previous email, I
> apologize for not going back and editing it to make it clear at the
> beginning that I'd done so. My current stance is that
> <git-compat-util.h> should be either (a) including only inttypes.h
> (since it includes stdint.h), or (b) including both inttypes.h and
> stdint.h (in either order), just to demonstrate that we can.
It is good to clean up old conditionals if they are no longer
applicable, as they are a burden to reason about later (as this
discussion shows). But I am not sure about your "just to demonstrate we
can". It is good to try it out, but it looks like there is a non-zero
chance that MSVC on Windows might break. It is probably better to try
building there or looping in folks who can, rather than just making a
change and seeing if anybody screams.
I think the "win+VS" test in the GitHub Actions CI job might cover this
case. It is not run by default (because it was considered be mostly
redundant with the mingw build), but it shouldn't be too hard to enable
it for a one-off test.
-Peff
^ permalink raw reply
* Re: [PATCH] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Josh Triplett @ 2024-02-27 8:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbk83nlw5.fsf@gitster.g>
On Mon, Feb 26, 2024 at 10:03:22AM -0800, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
>
> > `git commit --cleanup=scissors -v` currently prints two scissors lines:
> > one at the start of the comment lines, and the other right before the
> > diff. This is redundant, and pushes the diff further down in the user's
> > editor than it needs to be.
>
> Interesting discovery.
>
> > diff --git a/wt-status.c b/wt-status.c
> > index b5a29083df..459d399baa 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1143,11 +1143,13 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
> > * file (and even the "auto" setting won't work, since it
> > * will have checked isatty on stdout). But we then do want
> > * to insert the scissor line here to reliably remove the
> > - * diff before committing.
> > + * diff before committing, if we didn't already include one
> > + * before.
> > */
> > if (s->fp != stdout) {
> > rev.diffopt.use_color = 0;
> > - wt_status_add_cut_line(s->fp);
> > + if (s->cleanup_mode != COMMIT_MSG_CLEANUP_SCISSORS)
> > + wt_status_add_cut_line(s->fp);
> > }
>
> The machinery to populate the log message buffer should ideally be
> taught to remember if it already has added a scissors-line and to
> refrain from adding redundant ones. That way, we do not have to
> rely on the order of places that make wt_status_add_cut_line() calls
> or what condition they use to decide to make these calls.
>
> This hunk for example knows not just this one produces cut-line
> after the other one potentially added one, but also the logic used
> by the other one to decide to add one, which is even worse. I find
> the solution presented here a bit unsatisfactory, for this reason,
> but for now it may be OK, as we probably are not adding any more
> places and conditions to emit a scissors line.
I could add statefulness to wt_status_add_cut_line instead, on the
assumption that it's the only thing that should be adding a cut line,
and having it not add the line if previously added. For instance, it
could accept a pointer to the full wt_status rather than just the fp,
and keep a boolean state there.
> > builtin/commit.c | 2 ++
> > sequencer.h | 7 -------
> > wt-status.c | 6 ++++--
> > wt-status.h | 8 ++++++++
> > 4 files changed, 14 insertions(+), 9 deletions(-)
>
> If this change did not break any existing tests that checked the
> combination of options and output when they are used together, it
> means we have a gap in the test coverage. We needs a test or two
> to protect this fix from future breakages.
I did run the testsuite, and it passed. I can add a simple test easily
enough.
^ permalink raw reply
* Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.
From: Jeff King @ 2024-02-27 8:22 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240227082027.GH3263678@coredump.intra.peff.net>
On Tue, Feb 27, 2024 at 03:20:27AM -0500, Jeff King wrote:
> OK, so we detect the error and return it to the caller. Who is the
> caller? The only use of this function is in repack_promisor_objects(),
> which calls:
>
> for_each_packed_object(write_oid, &cmd,
> FOR_EACH_OBJECT_PROMISOR_ONLY);
>
> So when we return the error, now for_each_packed_object() will stop
> traversing, and propagate that error up to the caller. But as we can see
> above, the caller ignores it!
Oh, one other thing I meant to mention: as the test failure you saw was
related to repacking, this seemed like a likely culprit. But the code is
only triggered when repacking promisor objects in a partial clone, and
it didn't look like the test you posted covered that (it was just about
cruft packs). So I would not expect this code to be run at all in the
failing test you saw.
-Peff
^ permalink raw reply
* Re: [PATCH v2] completion: fix __git_complete_worktree_paths
From: Patrick Steinhardt @ 2024-02-27 8:21 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <eaf3649e-30cf-4eba-befa-5be826c828a8@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3845 bytes --]
On Mon, Feb 26, 2024 at 12:53:31AM +0100, Rubén Justo wrote:
> Use __git to invoke "worktree list" in __git_complete_worktree_paths, to
> respect any "-C" and "--git-dir" options present on the command line.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>
> I stumbled upon this in a situation like:
>
> $ git init /tmp/foo && cd /tmp/foo
> $ git worktree add --orphan foo_wt
>
> $ git init /tmp/bar && cd /tmp/bar
> $ git worktree add --orphan bar_wt
>
> $ cd /tmp/foo
> $ git -C /tmp/bar worktree remove <TAB>
> ... expecting /tmp/bar/wt, but ...
> $ git -C /tmp/bar worktree remove /tmp/foo_wt
>
> In this iteration, v2, some tests are included.
>
> The function __git was introduced in 1cd23e9e05 (completion: don't use
> __gitdir() for git commands, 2017-02-03). It is a small function, so
> I'll include here to ease the review of this patch:
>
> # Runs git with all the options given as argument, respecting any
> # '--git-dir=<path>' and '-C <path>' options present on the command line
> __git ()
> {
> git ${__git_C_args:+"${__git_C_args[@]}"} \
> ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
> }
>
>
> contrib/completion/git-completion.bash | 2 +-
> t/t9902-completion.sh | 24 ++++++++++++++++++++++++
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 444b3efa63..86e55dc67f 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3571,7 +3571,7 @@ __git_complete_worktree_paths ()
> # Generate completion reply from worktree list skipping the first
> # entry: it's the path of the main worktree, which can't be moved,
> # removed, locked, etc.
> - __gitcomp_nl "$(git worktree list --porcelain |
> + __gitcomp_nl "$(__git worktree list --porcelain |
> sed -n -e '2,$ s/^worktree //p')"
> }
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index b16c284181..7c0f82f31a 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1263,6 +1263,30 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
> test_cmp expected out
> '
>
> +test_expect_success '__git_complete_worktree_paths' '
> + test_when_finished "git worktree remove other_wt" &&
> + git worktree add --orphan other_wt &&
> + run_completion "git worktree remove " &&
> + grep other_wt out
> +'
> +
> +test_expect_success '__git_complete_worktree_paths - not a git repository' '
> + (
> + cd non-repo &&
> + GIT_CEILING_DIRECTORIES="$ROOT" &&
> + export GIT_CEILING_DIRECTORIES &&
> + test_completion "git worktree remove " "" 2>err &&
> + test_must_be_empty err
> + )
> +'
If I understand correctly, we assume that the repo isn't detected here,
and thus we will fail to complete the command. We don't want an error
message though, which we assert. But do we also want to assert that
there is no output on stdout?
> +
> +test_expect_success '__git_complete_worktree_paths with -C' '
> + test_when_finished "rm -rf to_delete" &&
What does this delete? I don't see "to_delete" being created as part of
this test.
> + git -C otherrepo worktree add --orphan otherrepo_wt &&
> + run_completion "git -C otherrepo worktree remove " &&
> + grep otherrepo_wt out
And as far as I can see, we don't write to "out" in this test, either.
So I think we're accidentally relying on state by the first test here.
Patrick
> +'
> +
> test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '
> test_completion "git switch " <<-\EOF
> branch-in-other Z
> --
> 2.44.0.1.g0da3aa8f7f
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.
From: Jeff King @ 2024-02-27 8:20 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240226220539.3494-4-randall.becker@nexbridge.ca>
On Mon, Feb 26, 2024 at 05:05:37PM -0500, Randall S. Becker wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> This change is required because some platforms do not support file writes of
> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
> maximum single I/O size possible for the destination device. The result of
> write_in_full() is also passed to the caller, which was previously ignored.
These are going to be tiny compared to single-write() I/O limits, I'd
think, but in general we should be on guard for the OS returning short
reads (this is a pipe and so for most systems PIPE_BUF would guarantee
atomicity, I think, but IMHO it is simpler to just make things
obviously-correct by looping with write_in_full). So I'd be surprised if
this spot was the cause of a visible bug, but I think it's worth
changing regardless.
The error detection is a separate question, though. I think it is good
to check the result of the write here, as an error here means that the
child pack-objects misses some objects we wanted it to pack, which could
lead to a corrupt repository. But I don't think what you have here is
quite enough:
> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,
> die(_("could not start pack-objects to repack promisor objects"));
> }
>
> - xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> - xwrite(cmd->in, "\n", 1);
> + err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> + if (err <= 0)
> + return err;
> + err = write_in_full(cmd->in, "\n", 1);
> + if (err <= 0)
> + return err;
> return 0;
OK, so we detect the error and return it to the caller. Who is the
caller? The only use of this function is in repack_promisor_objects(),
which calls:
for_each_packed_object(write_oid, &cmd,
FOR_EACH_OBJECT_PROMISOR_ONLY);
So when we return the error, now for_each_packed_object() will stop
traversing, and propagate that error up to the caller. But as we can see
above, the caller ignores it!
So I think you'd either want to die directly (perhaps using
write_or_die). Or you'd need to additionally check the return from
for_each_packed_object(). That would also catch cases where that
function failed to open a pack (I'm not sure how important that is to
this code).
But as it is, your patch just causes a write error to truncate the list
of oids send to the child process (though that is probably not
materially different from the current behavior, as the subsequent calls
would presumably fail, too).
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog
From: Jeff King @ 2024-02-27 8:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <xmqqh6hvcf3n.fsf@gitster.g>
On Mon, Feb 26, 2024 at 09:25:32AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > That's one of the reasons I split this out from patch 2; we can see
> > exactly what must be done to make each case work. And in fact I had
> > originally started to write a patch that simply changed t1508 to expect
> > failure. I could still be persuaded to go that way if anybody feels
> > strongly.
>
> I do not feel strongly either way myself. It just is interesting
> that the older end of the history is with @{20.years.ago} special
> case that is only for time-based query, while the newer end of the
> history is with @{0} special case.
I laid out my thinking on the 20.years.ago special case in another
reply, but I wanted to say one more thing. The special case here in
patch 3 is making @{0} work for the empty reflog, but there is no
matching special case for time-based timestamps. If you have an empty
reflog and ask for @{20.years.ago}, you will get the usual "nope, the
reflog is empty" response (as opposed to having a non-empty reflog that
cuts off before 20 years ago).
Obviously we could make that work, but I think the point is that "@{0}"
is special magic for "the current value" in a way that a timestamp isn't
really.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog
From: Jeff King @ 2024-02-27 8:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <xmqqmsrncf3r.fsf@gitster.g>
On Mon, Feb 26, 2024 at 09:25:28AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > if (!cb.reccnt) {
> > + if (cnt == 0) {
>
> Style "if (!cnt)" ? In this particular case I do not think it
> actually is an improvement, though, simply because zero is really
> special in this logic.
Yeah, I didn't really choose "== 0" as a conscious decision. But I do
tend to write "!cnt" when available, so I think I sub-consciously chose
this as a better description of the intent.
>
> > + /*
> > + * The caller asked for ref@{0}, and we had no entries.
> > + * It's a bit subtle, but in practice all callers have
> > + * prepped the "oid" field with the current value of
> > + * the ref, which is the most reasonable fallback.
> > + *
> > + * We'll put dummy values into the out-parameters (so
> > + * they're not just uninitialized garbage), and the
> > + * caller can take our return value as a hint that
> > + * we did not find any such reflog.
> > + */
> > + set_read_ref_cutoffs(&cb, 0, 0, "empty reflog");
> > + return 1;
> > + }
>
> The dummy value I 100% agree with ;-).
>
> You mentioned the convenience special case for time-based reflog
> query for a time older than (e.g. @{20.years.ago}) the reflog
> itself, and perhaps this one should be treated as its counterpart,
> that is only useful for count-based access.
I think the true counterpart to that would be extending what's added by
patch 2 to get_oid_basic() to stop checking that we hit the count
exactly.
Let me try to lay out my thinking. If you _do_ have a reflog and the
request (whether time or count-based) goes too far back, read_ref_at()
will give you the oldest entry and return "1". And then in
get_oid_basic():
- if it was a time based cutoff (like @{20.years.ago}), we will issue
a warning ("log only goes back to 2024-01-01") but return the value
anyway.
- before this series, if it is a count based cutoff (like @{9999}), we
fail and say "there are only have N entries".
- after this series, we special case asking for @{9999} when there are
9998 entries by returning the "old" oid but not issuing any warning
(we do not need to, because we found the right answer for what it
would have been had that old entry not been pruned).
- what we _could_ do (but this series does not), and what would be the
true counterpart to the @{20.years.ago} case, is to allow @{9999}
for a reflog with only 20 entries, returning the old value from 20
(or the new value if it was a creation!?) and issuing a warning
saying "well, it only went back 20, but here you go".
I'm not so sure about that last one. It is pretty subjective, but
somehow asking for timestamps feels more "fuzzy" to me, and Git
returning a fuzzy answer is OK. Whereas asking for item 9999 in a list
with 20 items and getting back an answer feels more absolutely wrong. I
could be persuaded if there were a concrete use case, but I can't really
think of one. It seems more likely to confuse and hinder a user than to
help them.
-Peff
^ permalink raw reply
* Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()
From: Patrick Steinhardt @ 2024-02-27 7:58 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git List
In-Reply-To: <08db9346-d10d-4b32-ae55-3ee5a8adf89d@web.de>
[-- Attachment #1: Type: text/plain, Size: 4023 bytes --]
On Mon, Feb 26, 2024 at 09:04:27PM +0100, René Scharfe wrote:
> Am 26.02.24 um 18:11 schrieb Junio C Hamano:
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >>> - !strncmp(rs->items[i].src,
> >>> - ref_namespace[NAMESPACE_TAGS].ref,
> >>> - strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {
> >>> + starts_with(rs->items[i].src,
> >>> + ref_namespace[NAMESPACE_TAGS].ref))) {
> >>
> >> The original tries to check that "namespace" fully matches the
> >> initial part of .src string, which is exactly what starts_with()
> >> does. Makes sense.
> >
> > There are two more such instances in the codebase, easily found with
> >
> > $ cat >contrib/coccinelle/starts_with.cocci <<\-EOF
> > @@
> > expression string, prefix;
> > @@
> > - !strncmp(string, prefix, strlen(prefix))
> > + starts_with(string, prefix)
> >
> > @@
> > expression string, prefix;
> > @@
> > - strncmp(string, prefix, strlen(prefix))
> > + !starts_with(string, prefix)
> > EOF
> > $ make contrib/coccinelle/starts_with.cocci.patch
> >
> > which finds the one you just fixed (naturally, since the Cocci patch
> > was written by taking inspiration from your fix).
> >
> > Here is one in the reftable code. The other one is in xdiff/ I'd
> > rather not touch (as starts_with() is probably not available there).
>
> Indeed, starts_with() is not available in xdiff/xpatience.c. That whole
> file is a Git-specific extension to libxdiff, though. We could add an
> include or copy the function definition.
It's kind of the same for the reftable library code. It's in this weird
in-between state where it's supposed to be usable as a library, but it
already does use some of Git's infra. I would personally be happy to use
more bits of the Git library in the reftable library, but we do need to
acknowledge that this makes it harder for other projects to use the
code.
So we should think about how we want to proceed here:
- Is the reftable library now a part of the Git codebase that can use
all of the features that we already have?
- Is the reftable library an independent part that has the goal of
being reusable for other projects?
With the ongoing libification project I certainly think that the first
option becomes more viable, as we can essentially have the best of both
worlds. The reftable library can be reused by other projects, and at the
same time we can use our own internals. But it's still going to take
quite some time until that can be fully realized, I assume.
Patrick
> It might make sense for performance reasons alone if there are lots of
> anchors or they are very long, as with starts_with() we no longer would
> have to call strlen() on all of them for each line to diff. Never used
> the anchored diff algorithm, though, so I don't know whether that's a
> problem in practice.
>
> >
> > diff -u -p a/reftable/refname.c b/reftable/refname.c
> > --- a/reftable/refname.c
> > +++ b/reftable/refname.c
> > @@ -103,7 +103,7 @@ static int modification_has_ref_with_pre
> > }
> > }
> >
> > - if (strncmp(ref.refname, prefix, strlen(prefix))) {
> > + if (!starts_with(ref.refname, prefix)) {
> > err = 1;
> > goto done;
> > }
>
>
> That file has another one with reversed argument order:
>
> diff --git a/reftable/refname.c b/reftable/refname.c
> index 7570e4acf9..10fc8b872d 100644
> --- a/reftable/refname.c
> +++ b/reftable/refname.c
> @@ -78,8 +78,7 @@ static int modification_has_ref_with_prefix(struct modification *mod,
> .want = prefix,
> };
> int idx = binsearch(mod->add_len, find_name, &arg);
> - if (idx < mod->add_len &&
> - !strncmp(prefix, mod->add[idx], strlen(prefix)))
> + if (idx < mod->add_len && starts_with(mod->add[idx], prefix))
> goto done;
> }
> err = reftable_table_seek_ref(&mod->tab, &it, prefix);
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] mem-pool: add mem_pool_strfmt()
From: Jeff King @ 2024-02-27 7:53 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <1868c502-b915-42fd-8cb2-efe90429f9b7@web.de>
On Mon, Feb 26, 2024 at 07:17:00PM +0100, René Scharfe wrote:
> > This is pulling heavily from strbuf_vaddf(). This might be a dumb idea,
> > but... would it be reasonable to instead push a global flag that causes
> > xmalloc() to use a memory pool instead of the regular heap?
> >
> > Then you could do something like:
> >
> > push_mem_pool(pool);
> > str = xstrfmt("%.*s~%d^%d", ...etc...);
> > pop_mem_pool(pool);
> >
> > It's a little more involved at the caller, but it means that it now
> > works for all allocations, not just this one string helper.
>
> That would allow to keep track of allocations that would otherwise leak.
> We can achieve that more easily by pushing the pointer to a global array
> and never freeing it. Hmm.
I see two uses for memory pools:
1. You want to optimize allocations (either locality or per-allocation
overhead).
2. You want to make a bunch of allocations with the same lifetime
without worrying about their lifetime otherwise. And then you can
free them all in one swoop at the end.
And my impression is that you were interested in (2) here. If what
you're saying is that another way to do that is:
str = xstrfmt(...whatever...);
attach_to_pool(pool, str);
...much later...
free_pool(pool);
then yeah, I'd agree. And that is a lot less tricky / invasive than what
I suggested.
> It would not allow the shortcut of using the vast pool as a scratch
> space to format the string with a single vsnprintf call in most cases.
> Or am I missing something?
So here it sounds like you do care about some of the performance
aspects. So no, it would not allow that. You'd be using the vast pool of
heap memory provided by malloc(), and trusting that a call to malloc()
is not that expensive in practice. I don't know how true that is, or how
much it matters for this case.
-Peff
^ permalink raw reply
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Jeff King @ 2024-02-27 7:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqqfrxexx15.fsf@gitster.g>
On Mon, Feb 26, 2024 at 10:04:54PM -0800, Junio C Hamano wrote:
> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH v2 3/3] git: extend --no-lazy-fetch to work across subprocesses
> [...]
This looks pretty reasonable to me, and the lines you drew for
#leftoverbits all seemed liked good spots.
The only thing I noticed in the patch was this (which I think is not
even new in this round):
> diff --git a/environment.c b/environment.c
> index 9e37bf58c0..afad78a3f8 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -136,6 +136,7 @@ const char * const local_repo_env[] = {
> GRAFT_ENVIRONMENT,
> INDEX_ENVIRONMENT,
> NO_REPLACE_OBJECTS_ENVIRONMENT,
> + NO_LAZY_FETCH_ENVIRONMENT,
> GIT_REPLACE_REF_BASE_ENVIRONMENT,
> GIT_PREFIX_ENVIRONMENT,
> GIT_SHALLOW_FILE_ENVIRONMENT,
This will clear the environment variable when we move between
repositories. I can see an argument for it, and certainly that's how
GIT_NO_REPLACE_OBJECTS works.
But I can also see an argument that this is not a property of the
_repository_, but of the request. For example, if I run "git
--no-lazy-fetch log" and we cross into a submodule to do a diff, should
that submodule also avoid lazy-fetching? I'd think so, but I think your
patch would restore the defaults when we "enter" the submodule repo.
There's some prior art there, I think, in how GIT_CEILING_DIRECTORIES
works, or even something like "git --literal-pathspecs", neither of
which appear in local_repo_env.
-Peff
^ 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