* [PATCH 09/10] commit-graph: free write-context base_graph_name during cleanup
From: Jeff King @ 2023-10-03 20:31 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
Commit 6c622f9f0b (commit-graph: write commit-graph chains, 2019-06-18)
added a base_graph_name string to the write_commit_graph_context struct.
But the end-of-function cleanup forgot to free it, causing a leak.
This (presumably in combination with the preceding leak-fixes) lets us
mark t5328 as leak-free.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 1 +
t/t5328-commit-graph-64bit-time.sh | 2 ++
2 files changed, 3 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 744b7eb1a3..e4d09da090 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2518,6 +2518,7 @@ int write_commit_graph(struct object_directory *odb,
cleanup:
free(ctx->graph_name);
+ free(ctx->base_graph_name);
free(ctx->commits.list);
oid_array_clear(&ctx->oids);
clear_topo_level_slab(&topo_levels);
diff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh
index e9c521c061..ca476e80a0 100755
--- a/t/t5328-commit-graph-64bit-time.sh
+++ b/t/t5328-commit-graph-64bit-time.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='commit graph with 64-bit timestamps'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
if ! test_have_prereq TIME_IS_64BIT || ! test_have_prereq TIME_T_IS_64BIT
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 08/10] commit-graph: free write-context entries before overwriting
From: Jeff King @ 2023-10-03 20:30 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When writing a split graph file, we replace the final element of the
commit_graph_hash_after and commit_graph_filenames_after arrays. But
since these are allocated strings, we need to free them before
overwriting to avoid leaking the old string.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 4aa2f294f1..744b7eb1a3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
free(graph_name);
}
+ free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));
final_graph_name = get_split_graph_filename(ctx->odb,
ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
+ free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
result = rename(ctx->graph_name, final_graph_name);
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 07/10] commit-graph: free graph struct that was not added to chain
From: Jeff King @ 2023-10-03 20:30 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When reading the graph chain file, we open (and allocate) each
individual slice it mentions and then add them to a linked-list chain.
But if adding to the chain fails (e.g., because the base-graph chunk it
contains didn't match what we expected), we leave the function without
freeing the graph struct that caused the failure, leaking it.
We can fix it by calling free_graph_commit().
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 2c72a554c2..4aa2f294f1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -566,6 +566,8 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
if (add_graph_to_chain(g, graph_chain, oids, i)) {
graph_chain = g;
valid = 1;
+ } else {
+ free_commit_graph(g);
}
break;
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain()
From: Jeff King @ 2023-10-03 20:30 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When adding a graph to a chain, we do some consistency checks and then
if everything looks good, set g->base_graph to add a link to the chain.
But when we added a new consistency check in 209250ef38 (commit-graph.c:
prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_
we've already set g->base_graph. So we might return failure, even though
we actually added to the chain.
This hasn't caused a bug yet, because after failing to add to the chain,
we discard the failed graph struct completely, leaking it. But in order
to fix that, it's important that the struct be in a consistent and
predictable state after the failure.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 2f75ecd9ae..2c72a554c2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,
cur_g = cur_g->base_graph;
}
- g->base_graph = chain;
-
if (chain) {
if (unsigned_add_overflows(chain->num_commits,
chain->num_commits_in_base)) {
@@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,
g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;
}
+ g->base_graph = chain;
+
return 1;
}
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 05/10] commit-graph: free all elements of graph chain
From: Jeff King @ 2023-10-03 20:29 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When running "commit-graph verify", we call free_commit_graph(). That's
sufficient for the case of a single graph file, but if we loaded a chain
of split graph files, they form a linked list via the base_graph
pointers. We need to free all of them, or we leak all but the first
struct.
We can make this work by teaching free_commit_graph() to walk the
base_graph pointers and free each element. This in turn lets us simplify
close_commit_graph(), which does the same thing by recursion (we cannot
just use close_commit_graph() in "commit-graph verify", as the function
takes a pointer to an object store, and the verify command creates a
single one-off graph struct).
While indenting the code in free_commit_graph() for the loop, I noticed
that setting g->data to NULL is rather pointless, as we free the struct
a few lines later. So I cleaned that up while we're here.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index dc54ef4776..2f75ecd9ae 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -723,19 +723,10 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)
return NULL;
}
-static void close_commit_graph_one(struct commit_graph *g)
-{
- if (!g)
- return;
-
- close_commit_graph_one(g->base_graph);
- free_commit_graph(g);
-}
-
void close_commit_graph(struct raw_object_store *o)
{
clear_commit_graph_data_slab(&commit_graph_data_slab);
- close_commit_graph_one(o->commit_graph);
+ free_commit_graph(o->commit_graph);
o->commit_graph = NULL;
}
@@ -2753,15 +2744,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
void free_commit_graph(struct commit_graph *g)
{
- if (!g)
- return;
- if (g->data) {
- munmap((void *)g->data, g->data_len);
- g->data = NULL;
+ while (g) {
+ struct commit_graph *next = g->base_graph;
+
+ if (g->data)
+ munmap((void *)g->data, g->data_len);
+ free(g->filename);
+ free(g->bloom_filter_settings);
+ free(g);
+
+ g = next;
}
- free(g->filename);
- free(g->bloom_filter_settings);
- free(g);
}
void disable_commit_graph(struct repository *r)
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph()
From: Jeff King @ 2023-10-03 20:27 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When closing and freeing a commit-graph, the main entry point is
close_commit_graph(), which then uses close_commit_graph_one() to
recurse through the base_graph links and free each one.
Commit 957ba814bf (commit-graph: when closing the graph, also release
the slab, 2021-09-08) put the call to clear the slab into the recursive
function, but this is pointless: there's only a single global slab
variable. It works OK in practice because clearing the slab is
idempotent, but it makes the code harder to reason about and refactor.
Move it into the parent function so it's only called once (and there are
no other direct callers of the recursive close_commit_graph_one(), so we
are not hurting them).
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/commit-graph.c b/commit-graph.c
index 5e8a3a5085..dc54ef4776 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -728,13 +728,13 @@ static void close_commit_graph_one(struct commit_graph *g)
if (!g)
return;
- clear_commit_graph_data_slab(&commit_graph_data_slab);
close_commit_graph_one(g->base_graph);
free_commit_graph(g);
}
void close_commit_graph(struct raw_object_store *o)
{
+ clear_commit_graph_data_slab(&commit_graph_data_slab);
close_commit_graph_one(o->commit_graph);
o->commit_graph = NULL;
}
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 03/10] merge: free result of repo_get_merge_bases()
From: Jeff King @ 2023-10-03 20:27 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
We call repo_get_merge_bases(), which allocates a commit_list, but never
free the result, causing a leak.
The obvious solution is to free it, but we need to look at the contents
of the first item to decide whether to leave the loop. One option is to
free it in both code paths. But since the commit that the list points to
is longer-lived than the list itself, we can just dereference it
immediately, free the list, and then continue with the existing logic.
This is about the same amount of code, but keeps the list management all
in one place.
This lets us mark a number of merge-related test scripts as leak-free.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/merge.c | 5 ++++-
t/t4214-log-graph-octopus.sh | 1 +
t/t4215-log-skewed-merges.sh | 1 +
t/t6009-rev-list-parent.sh | 1 +
t/t6416-recursive-corner-cases.sh | 1 +
t/t6433-merge-toplevel.sh | 1 +
t/t6437-submodule-merge.sh | 1 +
t/t7602-merge-octopus-many.sh | 1 +
t/t7603-merge-reduce-heads.sh | 1 +
t/t7607-merge-state.sh | 1 +
t/t7608-merge-messages.sh | 1 +
11 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index fd21c0d4f4..ac4816c14e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1634,6 +1634,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
for (j = remoteheads; j; j = j->next) {
struct commit_list *common_one;
+ struct commit *common_item;
/*
* Here we *have* to calculate the individual
@@ -1643,7 +1644,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
common_one = repo_get_merge_bases(the_repository,
head_commit,
j->item);
- if (!oideq(&common_one->item->object.oid, &j->item->object.oid)) {
+ common_item = common_one->item;
+ free_commit_list(common_one);
+ if (!oideq(&common_item->object.oid, &j->item->object.oid)) {
up_to_date = 0;
break;
}
diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
index f70c46bbbf..7905597869 100755
--- a/t/t4214-log-graph-octopus.sh
+++ b/t/t4214-log-graph-octopus.sh
@@ -5,6 +5,7 @@ test_description='git log --graph of skewed left octopus merge.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-log-graph.sh
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index 28d0779a8c..b877ac7235 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -2,6 +2,7 @@
test_description='git log --graph of skewed merges'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-log-graph.sh
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 5a67bbc760..ced40157ed 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -5,6 +5,7 @@ test_description='ancestor culling and limiting by parent number'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
check_revlist () {
diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh
index 17b54d625d..5f414abc89 100755
--- a/t/t6416-recursive-corner-cases.sh
+++ b/t/t6416-recursive-corner-cases.sh
@@ -5,6 +5,7 @@ test_description='recursive merge corner cases involving criss-cross merges'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-merge.sh
diff --git a/t/t6433-merge-toplevel.sh b/t/t6433-merge-toplevel.sh
index b16031465f..2b42f095dc 100755
--- a/t/t6433-merge-toplevel.sh
+++ b/t/t6433-merge-toplevel.sh
@@ -5,6 +5,7 @@ test_description='"git merge" top-level frontend'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
t3033_reset () {
diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh
index c9a86f2e94..daa507862c 100755
--- a/t/t6437-submodule-merge.sh
+++ b/t/t6437-submodule-merge.sh
@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-merge.sh
diff --git a/t/t7602-merge-octopus-many.sh b/t/t7602-merge-octopus-many.sh
index ff085b086c..3669d33bd5 100755
--- a/t/t7602-merge-octopus-many.sh
+++ b/t/t7602-merge-octopus-many.sh
@@ -4,6 +4,7 @@ test_description='git merge
Testing octopus merge with more than 25 refs.'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 4887ca705b..0e85b21ec8 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -4,6 +4,7 @@ test_description='git merge
Testing octopus merge when reducing parents to independent branches.'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# 0 - 1
diff --git a/t/t7607-merge-state.sh b/t/t7607-merge-state.sh
index 89a62ac53b..9001674f2e 100755
--- a/t/t7607-merge-state.sh
+++ b/t/t7607-merge-state.sh
@@ -4,6 +4,7 @@ test_description="Test that merge state is as expected after failed merge"
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'Ensure we restore original state if no merge strategy handles it' '
diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh
index 0b908ab2e7..2179938c43 100755
--- a/t/t7608-merge-messages.sh
+++ b/t/t7608-merge-messages.sh
@@ -4,6 +4,7 @@ test_description='test auto-generated merge messages'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
check_oneline() {
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 02/10] commit-reach: free temporary list in get_octopus_merge_bases()
From: Jeff King @ 2023-10-03 20:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
We loop over the set of commits to merge, and for each one compute the
merge base against the existing set of merge base candidates we've
found. Then we replace the candidate set with a simple assignment of the
list head, leaking the old list. We should free it first before
assignment.
This makes t5521 leak-free, so mark it as such.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-reach.c | 1 +
t/t5521-pull-options.sh | 1 +
2 files changed, 2 insertions(+)
diff --git a/commit-reach.c b/commit-reach.c
index 4b7c233fd4..a868a575ea 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -173,6 +173,7 @@ 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;
}
return ret;
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 264de29c35..079b2f2536 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -5,6 +5,7 @@ test_description='pull options'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 01/10] t6700: mark test as leak-free
From: Jeff King @ 2023-10-03 20:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
This test has never leaked since it was added. Let's annotate it to make
sure it stays that way (and to reduce noise when looking for other
leak-free scripts after we fix some leaks).
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously not directly related to the rest; this could be spun off to
its own series, or put atop jk/tree-name-and-depth-limit and merged from
there.
t/t6700-tree-depth.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh
index e410c41234..9e70a7c763 100755
--- a/t/t6700-tree-depth.sh
+++ b/t/t6700-tree-depth.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='handling of deep trees in various commands'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# We'll test against two depths here: a small one that will let us check the
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* Re: [PATCH v2] git-status.txt: fix minor asciidoc format issue
From: Junio C Hamano @ 2023-10-03 20:25 UTC (permalink / raw)
To: cousteau via GitGitGadget; +Cc: git, Javier Mora
In-Reply-To: <pull.1591.v2.git.1696350802820.gitgitgadget@gmail.com>
"cousteau via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Javier Mora <cousteaulecommandant@gmail.com>
>
> The paragraph below the list of short option combinations
> isn't correctly formatted, making the result hard to read.
>
> Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
> ---
The above probably no longer describes the situation the patch
intends to correct, I suspect. It used to be near-impossible hard
to read, but at least with them indented they are legible.
The additional states for submodules are typeset differently
from how the states for paths for normal blobs are listed as
enumeration. Format them in the same way for consistency.
or something like that, perhaps.
> Documentation/git-status.txt | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index b27d127b5e2..48f46eb2047 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -246,10 +246,9 @@ U U unmerged, both modified
>
> Submodules have more state and instead report
>
> - M the submodule has a different HEAD than
> - recorded in the index
> - m the submodule has modified content
> - ? the submodule has untracked files
> +* 'M' = the submodule has a different HEAD than recorded in the index
> +* 'm' = the submodule has modified content
> +* '?' = the submodule has untracked files
>
> since modified content or untracked files in a submodule cannot be added
> via `git add` in the superproject to prepare a commit.
Thanks for making this part of the documentation better.
^ permalink raw reply
* [PATCH 0/10] some commit-graph leak fixes
From: Jeff King @ 2023-10-03 20:25 UTC (permalink / raw)
To: git
I noticed while working on the jk/commit-graph-verify-fix topic that
free_commit_graph() leaks any slices of a commit-graph-chain except for
the first. I naively hoped that fixing that would make t5324 leak-free,
but it turns out there were a number of other leaks, so I fixed those,
too. A couple of them were in the merge code, which in turn means a
bunch of new test scripts are now leak-free.
Even though I saw the problem on that other topic, there's no dependency
here; this series can be applied directly to master (or possibly even
maint, though I didn't try).
[01/10]: t6700: mark test as leak-free
[02/10]: commit-reach: free temporary list in get_octopus_merge_bases()
[03/10]: merge: free result of repo_get_merge_bases()
[04/10]: commit-graph: move slab-clearing to close_commit_graph()
[05/10]: commit-graph: free all elements of graph chain
[06/10]: commit-graph: delay base_graph assignment in add_graph_to_chain()
[07/10]: commit-graph: free graph struct that was not added to chain
[08/10]: commit-graph: free write-context entries before overwriting
[09/10]: commit-graph: free write-context base_graph_name during cleanup
[10/10]: commit-graph: clear oidset after finishing write
builtin/commit-graph.c | 1 +
builtin/merge.c | 5 +++-
commit-graph.c | 40 ++++++++++++++----------------
commit-reach.c | 1 +
t/t4214-log-graph-octopus.sh | 1 +
t/t4215-log-skewed-merges.sh | 1 +
t/t5324-split-commit-graph.sh | 2 ++
t/t5328-commit-graph-64bit-time.sh | 2 ++
t/t5521-pull-options.sh | 1 +
t/t6009-rev-list-parent.sh | 1 +
t/t6416-recursive-corner-cases.sh | 1 +
t/t6433-merge-toplevel.sh | 1 +
t/t6437-submodule-merge.sh | 1 +
t/t6700-tree-depth.sh | 2 ++
t/t7602-merge-octopus-many.sh | 1 +
t/t7603-merge-reduce-heads.sh | 1 +
t/t7607-merge-state.sh | 1 +
t/t7608-merge-messages.sh | 1 +
18 files changed, 42 insertions(+), 22 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Junio C Hamano @ 2023-10-03 20:20 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <ZRxBlrjyuBmJnx3p@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
>> I've had this sitting in my patch queue for a while now. It's a
>> non-critical performance fix that avoids the repack/MIDX machinery from
>> ever choosing a cruft pack as preferred when writing a MIDX bitmap
>> without a given --preferred-pack.
>>
>> There is no correctness issue here, but choosing a pack with few/no
>> reachable objects means that our pack reuse mechanism will rarely kick
>> in, resulting in performance degradation.
>>
>> builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
>> t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 85 insertions(+), 1 deletion(-)
>
> Oops, I should have mentioned that this is meant to be applied on top of
> 'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry
> about that.
Sorry, but I do not follow. tb/multi-cruft-pack was merged to
'master' at c0b5d46d (Documentation/gitformat-pack.txt: drop mixed
version section, 2023-08-28) but back then t7704 did not exist. Do
you mean the other topic in-flight from you about max-cruft-size?
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Jeff King @ 2023-10-03 20:16 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com>
On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
> Note that this behavior is usually just a performance regression. But
> it's possible it could be a correctness issue.
>
> Suppose an object was duplicated among the cruft and non-cruft pack. The
> MIDX will pick the one from the pack with the lowest mtime, which will
> always be the cruft one. But if the non-cruft pack happens to sort
> earlier in lexical order, we'll treat that one as preferred, but not all
> duplicates will be resolved in favor of that pack.
>
> So if we happened to have an object which appears in both packs
> (e.g., due to a cruft object being freshened, causing it to appear
> loose, and then repacking it via the `--geometric` repack) it's possible
> the duplicate would be picked from the non-preferred pack.
I'm not sure I understand how that is a correctness issue. The contents
of the object are the same in either pack. Or do you mean that the
pack-reuse code in pack-objects.c may get confused and try to use the
wrong pack/offset when sending the object out? I would think it would
always be coming from the preferred pack for that code (so the outcome
is just that we fail to do the pack-reuse optimization very well, but we
don't generate a wrong answer).
Other than that, the explanation and patch make perfect sense to me, and
the patch looks good.
-Peff
^ permalink raw reply
* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Štěpán Němec @ 2023-10-03 20:15 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cQHAK2_LEG0-6MTF52t2D8b_SRxzB51B_4N35ujtGeb6Q@mail.gmail.com>
On Tue, 3 Oct 2023 14:33:39 -0400
Eric Sunshine wrote:
> On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
>> There's even an example of such usage in the README.
>>
>> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
>> ---
>> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
>> @@ -9,7 +9,7 @@ The <mode> parameter is one of:
>>
>> diff: elements are diff hunks. Arguments are given to diff.
>>
>> -merge: elements are merge conflicts. Arguments are ignored.
>> +merge: elements are merge conflicts. Arguments are given to ls-files -u.
>
> Should "ls-files -u" be formatted with backticks?
>
> Arguments are passed to `git ls-files -u`.
My preference is against this (I don't like markup in plain-text files),
although, unlike the contrib README case in 1/5, here there _are_ some
pre-existing backquotes; but the usage is inconsistent. If we want to
unify one way or the other, I'd leave that for another patch, in any
case.
Thanks,
Štěpán
^ permalink raw reply
* Re: batch-command wishlist [was: [TOPIC 02/12] Libification Goals and Progress]
From: Jeff King @ 2023-10-03 20:10 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Junio C Hamano, Taylor Blau
In-Reply-To: <20231003005251.M353509@dcvr>
On Tue, Oct 03, 2023 at 12:52:51AM +0000, Eric Wong wrote:
> Not sure if cat-file is the place for it, but a persistent
> process to deal with:
>
> * `git config -f FILENAME ...' (especially --get-urlmatch --type=FOO)
> * approxidate parsing for other tools[2]
>
> [2] I did recently license the code of a standalone C++ executable
> as GPL-2+ so I can copy approxidate parsing from git and
> perhaps figure out enough C++ to use Xapian query parser
> bindings instead of the hacky `git rev-parse --since=' thing
> I do.
I know that you asked for a persistent process, but just for reference,
you can hackily access approxidate with:
git config --type=expiry-date --default='15 days ago' does.not.exist
-Peff
^ permalink raw reply
* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Štěpán Němec @ 2023-10-03 20:09 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cR2wgAsGCTphqHsf_pbM0q_xLcMx=acVD6MDKhpEt3HNA@mail.gmail.com>
On Tue, 3 Oct 2023 14:30:38 -0400
Eric Sunshine wrote:
> On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
>> Fix some typos, grammar or wording issues in the documentation
>
> SubmittingPatches suggests: s/Fix/fix
I think that only applies to subjects of the form "area: description".
Come to think of it, 'doc: fix some typos, grammar and wording issues'
might have made sense to begin with; I don't suppose C header comments
are off-limits to doc:.
>> diff --git a/Documentation/git.txt b/Documentation/git.txt
>> @@ -96,7 +96,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config
>> This is useful for cases where you want to pass transitory
>> -configuration options to git, but are doing so on OS's where
>> +configuration options to git, but are doing so on OSes where
>
> Since you're touching this anyhow, it could probably be made clearer
> by simply spelling it out: "operating systems"
Probably. Checking for other occurrences now I see I haven't done my
homework, anyway: there are "OS's" as plural in config/transfer.txt and
fsmonitor--daemon.h, too. I'll include those and use "operating
systems" in the next version, thanks.
>> other processes might be able to read your cmdline
>> (e.g. `/proc/self/cmdline`), but not your environ
>> (e.g. `/proc/self/environ`). That behavior is the default on
>
> I wonder if "cmdline" and "environ" would be better spelled out, as
> well, since the examples which immediately follow them give the
> necessary context.
Agreed.
> other processes might be able to read your command-line
> (e.g. `/proc/self/cmdline`), but not your environment
> (e.g. `/proc/self/environ`).
>
> But perhaps that's outside the scope of this patch?
I don't think so; I'll include those.
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> @@ -1151,7 +1151,7 @@ will be stored via placeholder `%P`.
>> This attribute controls the length of conflict markers left in
>> -the work tree file during a conflicted merge. Only setting to
>> +the work tree file during a conflicted merge. Only setting
>> the value to a positive integer has any meaningful effect.
>
> Or, more simply:
>
> Only a positive integer has a meaningful effect.
That's better, thanks.
>> diff --git a/contrib/README b/contrib/README
>> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
>> I expect that things that start their life in the contrib/ area
>> -to graduate out of contrib/ once they mature, either by becoming
>> +graduate out of contrib/ once they mature, either by becoming
>
> You probably want to add a comma after "area".
That would read awkward to me. How about going the other way,
I expect things that start their life in the contrib/ area
to graduate out of contrib/ once they mature
instead?
> Do we want to correct the formatting of this pathname:
>
> ...in the `contrib/` area...
> ...out of `contrib/` once...
>
> or is that outside the scope of this patch?
I'd prefer not to: this is a plain-text README, no other markup present,
and I don't think the backquotes would help here.
>> diff --git a/strbuf.h b/strbuf.h
>> @@ -12,9 +12,9 @@
>> - * strbuf's are meant to be used with all the usual C string and memory
>> + * strbufs are meant to be used with all the usual C string and memory
>
> Alternatively:
>
> strbuf is meant to be used...
>
>> * APIs. Given that the length of the buffer is known, it's often better to
>> - * use the mem* functions than a str* one (memchr vs. strchr e.g.).
>> + * use the mem* functions than a str* one (e.g., memchr vs. strchr).
>> * Though, one has to be careful about the fact that str* functions often
>> * stop on NULs and that strbufs may have embedded NULs.
>
> Similar:
>
> ... and that a strbuf may have...
Hm, not sure about this one. I think I'd prefer keeping the plurals,
but it's not a strong preference.
>> diff --git a/t/README b/t/README
>> @@ -262,8 +262,8 @@ The argument for --run, <test-selector>, is a list of description
>> suite to include (or exclude, if negated) in the run. A range is two
>> -numbers separated with a dash and matches a range of tests with both
>> -ends been included. You may omit the first or the second number to
>> +numbers separated with a dash and matches an inclusive range of tests
>> +to run. You may omit the first or the second number to
>
> Not the fault of this patch, but "matches" seems an odd choice.
> Perhaps "specifies" would feel more natural.
Indeed, will fix.
>> The argument to --run is split on commas into separate strings,
>> @@ -579,11 +579,10 @@ This test harness library does the following things:
>> -Here are some recommented styles when writing test case.
>
> Do you want to fix the spelling error while you're here or is that
> done in a later patch?
>
> s/recommented/recommended/
You really had me double-checking both the branch and the patch I sent
here. :-D Unless I'm very much missing something, that line is _removed_
by the patch (seemed redundant given the title immediately preceding
it).
>> - - Keep test title the same line with test helper function itself.
>> + - Keep test titles and helper function invocations on the same line.
>
> This would be clearer if it was switched around. Either:
>
> Keep the test_expect_* function call and test title on the same line.
>
> or, more verbosely:
>
> Place the test title on the same line as the test_expect_* call
> which precedes it.
I'll take your first suggestion.
Thank you for the review!
--
Štěpán
^ permalink raw reply
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Jeff King @ 2023-10-03 20:06 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git, avarab
In-Reply-To: <20231003082513.3003520-1-stepnem@smrk.net>
On Tue, Oct 03, 2023 at 10:25:13AM +0200, Štěpán Němec wrote:
> The DESCRIPTION's "first form" is actually the 1st, 2nd, 3rd and 5th
> form in SYNOPSIS, the "second form" is the 4th one.
>
> Interestingly, this state of affairs was introduced in
> 97fe7250753b (cat-file docs: fix SYNOPSIS and "-h" output, 2021-12-28)
> with the claim of "Now the two will match again." ("the two" being
> DESCRIPTION and SYNOPSIS)...
>
> Ordinals are hard, let's try talking about batch and non-batch mode
> instead.
Thanks, I think this is a good direction. Two things I noticed:
> DESCRIPTION
> -----------
> -In its first form, the command provides the content or the type of an object in
> +This command can operate in two modes, depending on whether an option from
> +the `--batch` family is specified.
> +
> +In non-batch mode, the command provides the content or the type of an object in
> the repository. The type is required unless `-t` or `-p` is used to find the
> object type, or `-s` is used to find the object size, or `--textconv` or
> `--filters` is used (which imply type "blob").
The existing text here is already a bit vague, considering the number of
operations it covers (like "-e", for example, which is not showing "the
content or the type" at all). But that is not new in your patch, and it
is maybe even OK to be a bit vague here, and let the OPTIONS section
cover the specifics.
> -In the second form, a list of objects (separated by linefeeds) is provided on
> +In batch mode, a list of objects (separated by linefeeds) is provided on
> stdin, and the SHA-1, type, and size of each object is printed on stdout. The
> output format can be overridden using the optional `<format>` argument. If
> either `--textconv` or `--filters` was specified, the input is expected to
I think this got a bit inaccurate with "--batch-command", which is a
whole different mode itself compared to --batch and --batch-check. I
don't think your patch is really making anything worse, but arguably
there are three "major modes" here.
-Peff
^ permalink raw reply
* Re: Possible to update-ref remote repository?
From: Jeff King @ 2023-10-03 20:00 UTC (permalink / raw)
To: Jesse Hopkins; +Cc: Junio C Hamano, git
In-Reply-To: <CAL3By-_jrYDenCc8HSrtnR3cZD19z7bvwVOOoO4XG+6aNFxyeQ@mail.gmail.com>
On Sun, Oct 01, 2023 at 04:03:29PM -0600, Jesse Hopkins wrote:
> Thanks for the reply. Would you be able to point me to some
> breadcrumbs for the "required protocol messages"? I might try to
> tinker in some spare time.
Try "git help protocol-pack" in a recent version of Git (this used to be
in Documentation/technical/ of the repository, but much of that content
was moved into manpages around the v2.38 timeframe).
For a local or ssh connection, I think it is as simple as:
# you somehow happen to know this commit exists on the server,
# and what the current value of the ref is. If you don't know the
# current value, you can pull it from receive-pack's ref
# advertisement (I'll leave that as an exercise for the reader).
old=1234abcd...
new=5678cdef...
ref=refs/heads/main
# we'll use a local repository here, but you can replace receive-pack
# invocation below with with "ssh $host git receive-pack $repo"
repo=/path/to/repo.git
{
# git's pkt-line format is a 4-byte header with the ascii hex size of
# the packet, followed by N-4 bytes of data. Each ref update is
# in its own pkt, but we have just one.
cmd="$old $new $ref"
printf "%04x%s" $((${#cmd} + 4)) "$cmd"
# An all-zero flush packet indicates the end of the list of updates.
printf "0000"
# the server insists that we send a valid packfile, even if it is
# empty. This is from "git help format-pack" (the section on .pack
# files), though you could also generate it with "git pack-objects
# --stdout </dev/null".
printf 'PACK' ;# packfile
printf '\0\0\0\2' ;# version 2
printf '\0\0\0\0' ;# zero objects
# checksum, which is the sha1 of the rest of the pack
printf "\2\235\10\202\73\330\250\352\265\20\255\152\307\134\202\74\375\76\323\36"
} |
git receive-pack "$repo"
You can get fancier by specifying capabilities (you might want
"report-status", for example).
That will work for local or ssh repos. For http, it gets a little more
complicated. See the section "smart service git-receive-pack" of "git
help protocol-http".
All that said, I do think it might be reasonable for git-push to support
this directly. It is basically:
1. Let the command run in a non-repo, skipping anything that requires
it. This _might_ be a maintenance headache, but as a fallback you
could always run from an empty local repository.
2. Tell it to always generate an empty pack (basically, a "trust me,
the other side will be OK with it" option).
The second part looks like something like this:
diff --git a/send-pack.c b/send-pack.c
index 89aca9d829..c54463c181 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -58,6 +58,9 @@ static void feed_object(const struct object_id *oid, FILE *fh, int negative)
putc('\n', fh);
}
+/* obviously this should be passed down somehow in a real patch */
+#define SPECIAL_EMPTY_PACK_OPTION 1
+
/*
* Make a pack stream and spit it out into file descriptor fd
*/
@@ -103,17 +106,19 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,
* parameters by writing to the pipe.
*/
po_in = xfdopen(po.in, "w");
- for (i = 0; i < advertised->nr; i++)
- feed_object(&advertised->oid[i], po_in, 1);
- for (i = 0; i < negotiated->nr; i++)
- feed_object(&negotiated->oid[i], po_in, 1);
-
- while (refs) {
- if (!is_null_oid(&refs->old_oid))
- feed_object(&refs->old_oid, po_in, 1);
- if (!is_null_oid(&refs->new_oid))
- feed_object(&refs->new_oid, po_in, 0);
- refs = refs->next;
+ if (!SPECIAL_EMPTY_PACK_OPTION) {
+ for (i = 0; i < advertised->nr; i++)
+ feed_object(&advertised->oid[i], po_in, 1);
+ for (i = 0; i < negotiated->nr; i++)
+ feed_object(&negotiated->oid[i], po_in, 1);
+
+ while (refs) {
+ if (!is_null_oid(&refs->old_oid))
+ feed_object(&refs->old_oid, po_in, 1);
+ if (!is_null_oid(&refs->new_oid))
+ feed_object(&refs->new_oid, po_in, 0);
+ refs = refs->next;
+ }
}
fflush(po_in);
Come to think of it, you could probably fake it by wrapping
git-pack-objects with a script that throws away its input. Maybe hard to
do because its a builtin (and we run it as "git pack-objects", which
executes it directly in-process).
-Peff
^ permalink raw reply related
* Re: [silly] loose, pack, and another thing?
From: Jeff King @ 2023-10-03 19:09 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Junio C Hamano, git
In-Reply-To: <20230928214010.3502838-1-jonathantanmy@google.com>
On Thu, Sep 28, 2023 at 02:40:10PM -0700, Jonathan Tan wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Just wondering if it would help to have the third kind of object
> > representation in the object database, sitting next to loose objects
> > and packed objects, say .git/objects/verbatim/<hex-object-name> for
> > the contents and .git/objects/verbatim/<hex-object-name>.type that
> > records "blob", "tree", "commit", or "tag" (in practice, I would
> > expect huge "blob" objects would be the only ones that use this
> > mechanism).
> >
> > The contents will be stored verbatim without compression and without
> > any object header (i.e., the usual "<type> <length>\0") and the file
> > could be "ln"ed (or "cow"ed if the underlying filesystem allows it)
> > to materialize it in the working tree if needed.
>
> This sounds like a useful feature. We probably would want to use the
> "ln" or "cow" every time we use streaming (stream_blob_to_fd() in
> streaming.h) currently, so hopefully we won't need to increase the
> number of ways in which we can write an object to the worktree (just
> change the streaming to write to a filename instead of an fd).
One thing that scares me about a regular "ln" between the worktree and
odb is that you are very susceptible to corrupting the repository by
modifying the worktree file with regular tools. If they do a complete
rewrite and atomic rename (or link) to put the new file in place, that
is OK. But opening the file for appending, or general writing, is bad.
You can get some safety with the immutable attribute (which applies to
the inode itself, and thus any path that hardlinks to it). But setting
that usually requires being root. And it creates other irritations for
normal use (you have to unset it before even removing the hardlink).
It would be nice if there was some portable copy-on-write abstraction we
could rely on, but I don't think there is one.
-Peff
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Junio C Hamano @ 2023-10-03 18:51 UTC (permalink / raw)
To: Luma; +Cc: git
In-Reply-To: <CAFR+8DynAJ7eieMYUrezoNii5tzARNbESFxRCcT4w6okS5FZDg@mail.gmail.com>
Luma <ach.lumap@gmail.com> writes:
> Hi;
> My name is Luma, and I wanted to take a moment to introduce myself
> and share some
> insights on an essential aspect of avoiding pipes in git related
> commands in test scripts.
>
> I am an outreachy applicant for the December 2023 cohort and look
> forward to learning from you.
I notice that the title of the message and the immediate topic you
discuss in the body of the message do not match. I presume that the
topic on the title is what you prefer to work on if the unit testing
framework is ready by the time Outreachy program starts, and the
mention about "do not clobber exit code of Git with pipes in the
tests" is your "dip the tip of a toe in water" microproject?
Welcome to the Git development community.
Do you have a single word name? If so please disregard the below,
but in case "Luma" is just a nickname (e.g. like I am introducing
myself to my Git friends "Hi, I am Gitster!") you use online, please
read on.
For signing off your patches, we'd prefer to see your real name
used, as Signed-off-by: is meant to have legal significance. And
because we also expect the authorship identity to match the
name/e-mail of the sign-off, it would mean your patch submissions
are expected to look like:
From: Luma <ach.lumap@gmail.com>
Subject: ... title of the patch goes here ...
... body of the proposed commit log message goes here...
Signed-off-by: Luma <ach.lumap@gmail.com>
but "Luma" replaced with your full real name.
Thanks.
^ permalink raw reply
* [PATCH 6/6] t7420: Test that we correctly handle renamed submodules
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
Create a second submodule with a name that differs from its path. Test
that calling set-url modifies the correct .gitmodules entries. Make sure
we don't create a section named after the path instead of the name.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7420-submodule-set-url.sh | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index aa63d806fe..bf7f15ee79 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -25,34 +25,56 @@ test_expect_success 'submodule config cache setup' '
git add file &&
git commit -ma
) &&
+ mkdir namedsubmodule &&
+ (
+ cd namedsubmodule &&
+ git init &&
+ echo 1 >file &&
+ git add file &&
+ git commit -m1
+ ) &&
mkdir super &&
(
cd super &&
git init &&
git submodule add ../submodule &&
- git commit -m "add submodule"
+ git submodule add --name thename ../namedsubmodule thepath &&
+ git commit -m "add submodules"
)
'
test_expect_success 'test submodule set-url' '
- # add a commit and move the submodule (change the url)
+ # add commits and move the submodules (change the urls)
(
cd submodule &&
echo b >>file &&
git add file &&
git commit -mb
) &&
mv submodule newsubmodule &&
+ (
+ cd namedsubmodule &&
+ echo 2 >>file &&
+ git add file &&
+ git commit -m2
+ ) &&
+ mv namedsubmodule newnamedsubmodule &&
+
git -C newsubmodule show >expect &&
+ git -C newnamedsubmodule show >>expect &&
(
cd super &&
test_must_fail git submodule update --remote &&
git submodule set-url submodule ../newsubmodule &&
test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
+ git submodule set-url thepath ../newnamedsubmodule &&
+ test_cmp_config ../newnamedsubmodule -f .gitmodules submodule.thename.url &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thepath.url &&
git submodule update --remote
) &&
git -C super/submodule show >actual &&
+ git -C super/thepath show >>actual &&
test_cmp expect actual
'
--
2.42.0
^ permalink raw reply related
* [PATCH 3/6] t7419: Actually test the branch switching
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
The submodule repo the test set up had the 'topic' branch checked out,
meaning the repo's default branch (HEAD) is the 'topic' branch.
The following tests then pretended to switch between the default branch
and the 'topic' branch. This was papered over by continually adding
commits to the 'topic' branch and checking if the submodule gets updated
to this new commit.
Return the submodule repo to the 'main' branch after setup so we can
actually test the switching behavior.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7419-submodule-set-branch.sh | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 232065504c..5ac16d0eb7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -11,23 +11,28 @@ as expected.
TEST_PASSES_SANITIZE_LEAK=true
TEST_NO_CREATE_REPO=1
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
. ./test-lib.sh
test_expect_success 'setup' '
git config --global protocol.file.allow always
'
test_expect_success 'submodule config cache setup' '
mkdir submodule &&
(cd submodule &&
git init &&
echo a >a &&
git add . &&
git commit -ma &&
git checkout -b topic &&
echo b >a &&
git add . &&
- git commit -mb
+ git commit -mb &&
+ git checkout main
) &&
mkdir super &&
(cd super &&
@@ -57,41 +62,38 @@ test_expect_success 'test submodule set-branch --branch' '
'
test_expect_success 'test submodule set-branch --default' '
- test_commit -C submodule c &&
(cd super &&
git submodule set-branch --default submodule &&
! grep branch .gitmodules &&
git submodule update --remote &&
cat <<-\EOF >expect &&
- c
+ a
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -b' '
- test_commit -C submodule b &&
(cd super &&
git submodule set-branch -b topic submodule &&
grep "branch = topic" .gitmodules &&
git submodule update --remote &&
cat <<-\EOF >expect &&
b
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -d' '
- test_commit -C submodule d &&
(cd super &&
git submodule set-branch -d submodule &&
! grep branch .gitmodules &&
git submodule update --remote &&
cat <<-\EOF >expect &&
- d
+ a
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
--
2.42.0
^ permalink raw reply related
* [PATCH 4/6] t7419, t7420: Use test_cmp_config instead of grepping .gitmodules
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
We have a test function to verify config files. Use it as it's more
precise.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7419-submodule-set-branch.sh | 10 +++++-----
t/t7420-submodule-set-url.sh | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 5ac16d0eb7..3cd30865a7 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -44,53 +44,53 @@ test_expect_success 'submodule config cache setup' '
test_expect_success 'ensure submodule branch is unset' '
(cd super &&
- ! grep branch .gitmodules
+ test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch
)
'
test_expect_success 'test submodule set-branch --branch' '
(cd super &&
git submodule set-branch --branch topic submodule &&
- grep "branch = topic" .gitmodules &&
+ test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
b
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch --default' '
(cd super &&
git submodule set-branch --default submodule &&
- ! grep branch .gitmodules &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
a
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -b' '
(cd super &&
git submodule set-branch -b topic submodule &&
- grep "branch = topic" .gitmodules &&
+ test_cmp_config topic -f .gitmodules submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
b
EOF
git -C submodule show -s --pretty=%s >actual &&
test_cmp expect actual
)
'
test_expect_success 'test submodule set-branch -d' '
(cd super &&
git submodule set-branch -d submodule &&
- ! grep branch .gitmodules &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.submodule.branch &&
git submodule update --remote &&
cat <<-\EOF >expect &&
a
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
index d6bf62b3ac..aa63d806fe 100755
--- a/t/t7420-submodule-set-url.sh
+++ b/t/t7420-submodule-set-url.sh
@@ -49,7 +49,7 @@ test_expect_success 'test submodule set-url' '
cd super &&
test_must_fail git submodule update --remote &&
git submodule set-url submodule ../newsubmodule &&
- grep -F "url = ../newsubmodule" .gitmodules &&
+ test_cmp_config ../newsubmodule -f .gitmodules submodule.submodule.url &&
git submodule update --remote
) &&
git -C super/submodule show >actual &&
--
2.42.0
^ permalink raw reply related
* [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <0a0a157f88321d25fdb0be771a454b3410a449f3.camel@archlinux.org>
The commands need a path to a submodule but treated it as the name when
modifying the .gitmodules file, leading to confusion when a submodule's
name does not match its path.
Because calling submodule_from_path initializes the submodule cache, we
need to manually trigger a reread before syncing, as the cache is
missing the config change we just made.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
builtin/submodule--helper.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6871efd95..f376466a5e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
N_("git submodule set-url [--quiet] <path> <newurl>"),
NULL
};
+ const struct submodule *sub;
argc = parse_options(argc, argv, prefix, options, usage, 0);
if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
usage_with_options(usage, options);
- config_name = xstrfmt("submodule.%s.url", path);
+ sub = submodule_from_path(the_repository, null_oid(), path);
+ if (!sub)
+ die(_("no submodule mapping found in .gitmodules for path '%s'"),
+ path);
+
+ config_name = xstrfmt("submodule.%s.url", sub->name);
config_set_in_gitmodules_file_gently(config_name, newurl);
- sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
+
+ repo_read_gitmodules (the_repository, 0);
+ sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
free(config_name);
-
return 0;
}
@@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
NULL
};
+ const struct submodule *sub;
argc = parse_options(argc, argv, prefix, options, usage, 0);
if (!opt_branch && !opt_default)
die(_("--branch or --default required"));
if (opt_branch && opt_default)
die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default");
if (argc != 1 || !(path = argv[0]))
usage_with_options(usage, options);
- config_name = xstrfmt("submodule.%s.branch", path);
+ sub = submodule_from_path(the_repository, null_oid(), path);
+
+ if (!sub)
+ die(_("no submodule mapping found in .gitmodules for path '%s'"),
+ path);
+
+ config_name = xstrfmt("submodule.%s.branch", sub->name);
ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
free(config_name);
--
2.42.0
^ permalink raw reply related
* [PATCH 5/6] t7419: Test that we correctly handle renamed submodules
From: Jan Alexander Steffens (heftig) @ 2023-10-03 18:50 UTC (permalink / raw)
To: git; +Cc: Jan Alexander Steffens (heftig)
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
Add the submodule again with an explicitly different name and path. Test
that calling set-branch modifies the correct .gitmodules entries. Make
sure we don't create a section named after the path instead of the name.
Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
t/t7419-submodule-set-branch.sh | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
index 3cd30865a7..a5d1bc5c54 100755
--- a/t/t7419-submodule-set-branch.sh
+++ b/t/t7419-submodule-set-branch.sh
@@ -38,7 +38,8 @@ test_expect_success 'submodule config cache setup' '
(cd super &&
git init &&
git submodule add ../submodule &&
- git commit -m "add submodule"
+ git submodule add --name thename ../submodule thepath &&
+ git commit -m "add submodules"
)
'
@@ -100,4 +101,31 @@ test_expect_success 'test submodule set-branch -d' '
)
'
+test_expect_success 'test submodule set-branch --branch with named submodule' '
+ (cd super &&
+ git submodule set-branch --branch topic thepath &&
+ test_cmp_config topic -f .gitmodules submodule.thename.branch &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thepath.branch &&
+ git submodule update --remote &&
+ cat <<-\EOF >expect &&
+ b
+ EOF
+ git -C thepath show -s --pretty=%s >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'test submodule set-branch --default with named submodule' '
+ (cd super &&
+ git submodule set-branch --default thepath &&
+ test_cmp_config "" -f .gitmodules --default "" submodule.thename.branch &&
+ git submodule update --remote &&
+ cat <<-\EOF >expect &&
+ a
+ EOF
+ git -C thepath show -s --pretty=%s >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.42.0
^ permalink raw reply related
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