* [PATCH v2 0/6] some "commit-graph verify" fixes for chains
From: Jeff King @ 2023-09-28 4:37 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@coredump.intra.peff.net>
On Tue, Sep 26, 2023 at 01:54:52AM -0400, Jeff King wrote:
> [1/6]: commit-graph: factor out chain opening function
> [2/6]: commit-graph: check mixed generation validation when loading chain file
> [3/6]: t5324: harmonize sha1/sha256 graph chain corruption
> [4/6]: commit-graph: detect read errors when verifying graph chain
> [5/6]: commit-graph: tighten chain size check
> [6/6]: commit-graph: report incomplete chains during verification
Here's a re-roll that fixes a missed case in patch 6 (I sent more
details elsewhere in the thread).
Range-diff is below, but the changes are:
- patch 3 extends its tests to cover corrupting both the first and
second lines of the file. The test and directory renaming spills
over into the range-diff context for the other patches (and I
renamed the directory used in the "too-short chain file" test in
patch 5 to align better with the others).
- patch 6 covers the case that get_oid_hex() fails (previously it only
detected that a parsed hex failed to result in us loading a valid
graph file). The range-diff is IMHO quite hard to read here, but the
patch itself is quite simple.
1: baa20e6391 = 1: a4893f325f commit-graph: factor out chain opening function
2: b77cc15584 = 2: 722181d8ed commit-graph: check mixed generation validation when loading chain file
3: af350d1d41 < -: ---------- t5324: harmonize sha1/sha256 graph chain corruption
-: ---------- > 3: 7de47054c7 t5324: harmonize sha1/sha256 graph chain corruption
4: 54ce3863f8 ! 4: 313c051a38 commit-graph: detect read errors when verifying graph chain
@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv, con
free_commit_graph(graph);
## t/t5324-split-commit-graph.sh ##
-@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption' '
+@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (base)' '
(
- cd verify-chain &&
+ cd verify-chain-base &&
corrupt_file "$graphdir/commit-graph-chain" 30 "G" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
5: 273f5e8b87 ! 5: 6d9efd761a commit-graph: tighten chain size check
@@ commit-graph.c: int open_commit_graph_chain(const char *chain_file,
return 1;
## t/t5324-split-commit-graph.sh ##
-@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption' '
+@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (tip)' '
)
'
+test_expect_success 'verify notices too-short chain file' '
-+ git clone --no-hardlinks . verify-chain-garbage &&
++ git clone --no-hardlinks . verify-chain-short &&
+ (
-+ cd verify-chain-garbage &&
++ cd verify-chain-short &&
+ git commit-graph verify &&
+ echo "garbage" >$graphdir/commit-graph-chain &&
+ test_must_fail git commit-graph verify 2>test_err &&
6: 2661efed03 ! 6: 8fc82e2254 commit-graph: report incomplete chains during verification
@@ Commit message
"tip" case is what is fixed by this patch (it complains to stderr but
still returns the base slice).
+ Likewise the existing tests for corruption of the commit-graph-chain
+ file itself need to be updated. We already exited non-zero correctly for
+ the "base" case, but the "tip" case can now do so, too.
+
Note that this also causes us to adjust a test later in the file that
similarly corrupts a tip (though confusingly the test script calls this
"base"). It checks stderr but erroneously expects the whole "verify"
@@ commit-graph.c: int open_commit_graph_chain(const char *chain_file,
{
struct commit_graph *graph_chain = NULL;
struct strbuf line = STRBUF_INIT;
- struct object_id *oids;
- int i = 0, valid = 1, count;
- FILE *fp = xfdopen(fd, "r");
-
-+ *incomplete_chain = 0;
-+
- count = st->st_size / (the_hash_algo->hexsz + 1);
- CALLOC_ARRAY(oids, count);
-
@@ commit-graph.c: struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
+ fclose(fp);
+ strbuf_release(&line);
+
++ *incomplete_chain = !valid;
+ return graph_chain;
+ }
- if (!valid) {
- warning(_("unable to find all commit-graph files"));
-+ *incomplete_chain = 1;
- break;
- }
- }
@@ commit-graph.c: static struct commit_graph *load_commit_graph_chain(struct repository *r,
struct commit_graph *g = NULL;
@@ t/t5324-split-commit-graph.sh: test_expect_success 'warn on base graph chunk inc
grep -v "^+" test_err >err &&
test_i18ngrep "commit-graph chain does not match" err
)
+@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (tip)' '
+ (
+ cd verify-chain-tip &&
+ corrupt_file "$graphdir/commit-graph-chain" 70 "G" &&
+- git commit-graph verify 2>test_err &&
++ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ test_i18ngrep "invalid commit-graph chain" err &&
+ corrupt_file "$graphdir/commit-graph-chain" 70 "A" &&
+- git commit-graph verify 2>test_err &&
++ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ test_i18ngrep "unable to find all commit-graph files" err
+ )
[1/6]: commit-graph: factor out chain opening function
[2/6]: commit-graph: check mixed generation validation when loading chain file
[3/6]: t5324: harmonize sha1/sha256 graph chain corruption
[4/6]: commit-graph: detect read errors when verifying graph chain
[5/6]: commit-graph: tighten chain size check
[6/6]: commit-graph: report incomplete chains during verification
builtin/commit-graph.c | 31 +++++++---
commit-graph.c | 109 +++++++++++++++++++++-------------
commit-graph.h | 4 ++
t/t5324-split-commit-graph.sh | 69 ++++++++++++++++++---
4 files changed, 158 insertions(+), 55 deletions(-)
-Peff
^ permalink raw reply
* [PATCH v2 1/6] commit-graph: factor out chain opening function
From: Jeff King @ 2023-09-28 4:38 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230928043746.GB57926@coredump.intra.peff.net>
The load_commit_graph_chain() function opens the chain file and all of
the slices of graph that it points to. If there is no chain file (which
is a totally normal condition), we return NULL. But if we run into
errors with the chain file or loading the actual graph data, we also
return NULL, and the caller cannot tell the difference.
The caller can check for ENOENT for the unremarkable "no such file"
case. But I'm hesitant to assume that the rest of the function would
never accidentally set errno to ENOENT itself, since it is opening the
slice files (and that would mean the caller fails to notice a real
error).
So let's break this into two functions: one to open the file, and one to
actually load it. This matches the interface we provide for the
non-chain graph file, which will also come in handy in a moment when we
fix some bugs in the "git commit-graph verify" code.
Some notes:
- I've kept the "1 is good, 0 is bad" return convention (and the weird
"fd" out-parameter) used by the matching open_commit_graph()
function and other parts of the commit-graph code. This is unlike
most of the rest of Git (which would just return the fd, with -1 for
error), but it makes sense to stay consistent with the adjacent bits
of the API here.
- The existing chain loading function will quietly return if the file
is too small to hold a single entry. I've retained that behavior
(and explicitly set ENOENT in the opener function) for now, under
the notion that it's probably valid (though I'd imagine unusual) to
have an empty chain file.
There are two small behavior changes here, but I think both are strictly
positive:
1. The original blindly did a stat() before checking if fopen()
succeeded, meaning we were making a pointless extra stat call.
2. We now use fstat() to check the file size. The previous code using
a regular stat() on the pathname meant we could technically race
with somebody updating the chain file, and end up with a size that
does not match what we just opened with fopen(). I doubt anybody
ever hit this in practice, but it may have caused an out-of-bounds
read.
We'll retain the load_commit_graph_chain() function which does both the
open and reading steps (most existing callers do not care about seeing
errors anyway, since loading commit-graphs is optimistic).
Signed-off-by: Jeff King <peff@peff.net>
---
Same as v1.
commit-graph.c | 58 +++++++++++++++++++++++++++++++++-----------------
commit-graph.h | 3 +++
2 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 5e8a3a5085..12cdd9af8e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -513,31 +513,34 @@ static int add_graph_to_chain(struct commit_graph *g,
return 1;
}
-static struct commit_graph *load_commit_graph_chain(struct repository *r,
- struct object_directory *odb)
+int open_commit_graph_chain(const char *chain_file,
+ int *fd, struct stat *st)
+{
+ *fd = git_open(chain_file);
+ if (*fd < 0)
+ return 0;
+ if (fstat(*fd, st)) {
+ close(*fd);
+ return 0;
+ }
+ if (st->st_size <= the_hash_algo->hexsz) {
+ close(*fd);
+ errno = ENOENT;
+ return 0;
+ }
+ return 1;
+}
+
+struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
+ int fd, struct stat *st)
{
struct commit_graph *graph_chain = NULL;
struct strbuf line = STRBUF_INIT;
- struct stat st;
struct object_id *oids;
int i = 0, valid = 1, count;
- char *chain_name = get_commit_graph_chain_filename(odb);
- FILE *fp;
- int stat_res;
+ FILE *fp = xfdopen(fd, "r");
- fp = fopen(chain_name, "r");
- stat_res = stat(chain_name, &st);
- free(chain_name);
-
- if (!fp)
- return NULL;
- if (stat_res ||
- st.st_size <= the_hash_algo->hexsz) {
- fclose(fp);
- return NULL;
- }
-
- count = st.st_size / (the_hash_algo->hexsz + 1);
+ count = st->st_size / (the_hash_algo->hexsz + 1);
CALLOC_ARRAY(oids, count);
prepare_alt_odb(r);
@@ -585,6 +588,23 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
return graph_chain;
}
+static struct commit_graph *load_commit_graph_chain(struct repository *r,
+ struct object_directory *odb)
+{
+ char *chain_file = get_commit_graph_chain_filename(odb);
+ struct stat st;
+ int fd;
+ struct commit_graph *g = NULL;
+
+ if (open_commit_graph_chain(chain_file, &fd, &st)) {
+ /* ownership of fd is taken over by load function */
+ g = load_commit_graph_chain_fd_st(r, fd, &st);
+ }
+
+ free(chain_file);
+ return g;
+}
+
/*
* returns 1 if and only if all graphs in the chain have
* corrected commit dates stored in the generation_data chunk.
diff --git a/commit-graph.h b/commit-graph.h
index 5e534f0fcc..3b662fd2b5 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -26,6 +26,7 @@ struct string_list;
char *get_commit_graph_filename(struct object_directory *odb);
char *get_commit_graph_chain_filename(struct object_directory *odb);
int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
+int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st);
/*
* Given a commit struct, try to fill the commit struct info, including:
@@ -105,6 +106,8 @@ struct commit_graph {
struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
int fd, struct stat *st,
struct object_directory *odb);
+struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
+ int fd, struct stat *st);
struct commit_graph *read_commit_graph_one(struct repository *r,
struct object_directory *odb);
--
2.42.0.773.ga6e30199be
^ permalink raw reply related
* [PATCH v2 2/6] commit-graph: check mixed generation validation when loading chain file
From: Jeff King @ 2023-09-28 4:38 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230928043746.GB57926@coredump.intra.peff.net>
In read_commit_graph_one(), we call validate_mixed_generation_chain()
after loading the graph. Even though we don't check the return value,
this has the side effect of clearing the read_generation_data flag,
which is important when working with mixed generation numbers.
But doing this in load_commit_graph_chain_fd_st() makes more sense:
1. We are calling it even when we did not load a chain at all, which
is pointless (you cannot have mixed generations in a single file).
2. For now, all callers load the graph via read_commit_graph_one().
But the point of factoring out the open/load in the previous commit
was to let "commit-graph verify" call them separately. So it needs
to trigger this function as part of the load.
Without this patch, the mixed-generation tests in t5324 would start
failing on "git commit-graph verify" calls, once we switch to using
a separate open/load call there.
Signed-off-by: Jeff King <peff@peff.net>
---
Same as v1.
commit-graph.c | 54 +++++++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 12cdd9af8e..8b29c6de24 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -473,6 +473,31 @@ static struct commit_graph *load_commit_graph_v1(struct repository *r,
return g;
}
+/*
+ * returns 1 if and only if all graphs in the chain have
+ * corrected commit dates stored in the generation_data chunk.
+ */
+static int validate_mixed_generation_chain(struct commit_graph *g)
+{
+ int read_generation_data = 1;
+ struct commit_graph *p = g;
+
+ while (read_generation_data && p) {
+ read_generation_data = p->read_generation_data;
+ p = p->base_graph;
+ }
+
+ if (read_generation_data)
+ return 1;
+
+ while (g) {
+ g->read_generation_data = 0;
+ g = g->base_graph;
+ }
+
+ return 0;
+}
+
static int add_graph_to_chain(struct commit_graph *g,
struct commit_graph *chain,
struct object_id *oids,
@@ -581,6 +606,8 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
}
}
+ validate_mixed_generation_chain(graph_chain);
+
free(oids);
fclose(fp);
strbuf_release(&line);
@@ -605,31 +632,6 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
return g;
}
-/*
- * returns 1 if and only if all graphs in the chain have
- * corrected commit dates stored in the generation_data chunk.
- */
-static int validate_mixed_generation_chain(struct commit_graph *g)
-{
- int read_generation_data = 1;
- struct commit_graph *p = g;
-
- while (read_generation_data && p) {
- read_generation_data = p->read_generation_data;
- p = p->base_graph;
- }
-
- if (read_generation_data)
- return 1;
-
- while (g) {
- g->read_generation_data = 0;
- g = g->base_graph;
- }
-
- return 0;
-}
-
struct commit_graph *read_commit_graph_one(struct repository *r,
struct object_directory *odb)
{
@@ -638,8 +640,6 @@ struct commit_graph *read_commit_graph_one(struct repository *r,
if (!g)
g = load_commit_graph_chain(r, odb);
- validate_mixed_generation_chain(g);
-
return g;
}
--
2.42.0.773.ga6e30199be
^ permalink raw reply related
* [PATCH v2 3/6] t5324: harmonize sha1/sha256 graph chain corruption
From: Jeff King @ 2023-09-28 4:38 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230928043746.GB57926@coredump.intra.peff.net>
In t5324.20, we corrupt a hex character 60 bytes into the graph chain
file. Since the file consists of two hash identifiers, one per line, the
corruption differs between sha1 and sha256. In a sha1 repository, the
corruption is on the second line, and in a sha256 repository, it is on
the first.
We should of course detect the problem with either line. But as the next
few patches will show (and fix), that is not the case (in fact, we
currently do not exit non-zero for either line!). And while at the end
of our series we'll catch all errors, our intermediate states will have
differing behavior between the two hashes.
Let's make sure we test corruption of both the first and second lines,
and do so consistently with either hash by choosing offsets which are
always in the first hash (30 bytes) or in the second (70).
Signed-off-by: Jeff King <peff@peff.net>
---
In v2 we are now testing both first and second lines, instead of just
the first.
t/t5324-split-commit-graph.sh | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 36c4141e67..a9b2428b56 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -312,15 +312,30 @@ test_expect_success 'warn on base graph chunk incorrect' '
)
'
-test_expect_success 'verify after commit-graph-chain corruption' '
- git clone --no-hardlinks . verify-chain &&
+test_expect_success 'verify after commit-graph-chain corruption (base)' '
+ git clone --no-hardlinks . verify-chain-base &&
(
- cd verify-chain &&
- corrupt_file "$graphdir/commit-graph-chain" 60 "G" &&
+ cd verify-chain-base &&
+ corrupt_file "$graphdir/commit-graph-chain" 30 "G" &&
git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "invalid commit-graph chain" err &&
- corrupt_file "$graphdir/commit-graph-chain" 60 "A" &&
+ corrupt_file "$graphdir/commit-graph-chain" 30 "A" &&
+ git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ test_i18ngrep "unable to find all commit-graph files" err
+ )
+'
+
+test_expect_success 'verify after commit-graph-chain corruption (tip)' '
+ git clone --no-hardlinks . verify-chain-tip &&
+ (
+ cd verify-chain-tip &&
+ corrupt_file "$graphdir/commit-graph-chain" 70 "G" &&
+ git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ test_i18ngrep "invalid commit-graph chain" err &&
+ corrupt_file "$graphdir/commit-graph-chain" 70 "A" &&
git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "unable to find all commit-graph files" err
--
2.42.0.773.ga6e30199be
^ permalink raw reply related
* [PATCH v2 4/6] commit-graph: detect read errors when verifying graph chain
From: Jeff King @ 2023-09-28 4:38 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230928043746.GB57926@coredump.intra.peff.net>
Because it's OK to not have a graph file at all, the graph_verify()
function needs to tell the difference between a missing file and a real
error. So when loading a traditional graph file, we call
open_commit_graph() separately from load_commit_graph_chain_fd_st(), and
don't complain if the first one fails with ENOENT.
When the function learned about chain files in 3da4b609bb (commit-graph:
verify chains with --shallow mode, 2019-06-18), we couldn't be as
careful, since the only way to load a chain was with
read_commit_graph_one(), which did both the open/load as a single unit.
So we'll miss errors in chain files we load, thinking instead that there
was just no chain file at all.
Note that we do still report some of these problems to stderr, as the
loading function calls error() and warning(). But we'd exit with a
successful exit code, which is wrong.
We can fix that by using the recently split open/load functions for
chains. That lets us treat the chain file just like a single file with
respect to error handling here.
An existing test (from 3da4b609bb) shows off the problem; we were
expecting "commit-graph verify" to report success, but that makes no
sense. We did not even verify the contents of the graph data, because we
couldn't load it! I don't think this was an intentional exception, but
rather just the test covering what happened to occur.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit-graph.c | 23 ++++++++++++++++-------
t/t5324-split-commit-graph.sh | 4 ++--
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c88389df24..50c15d9bfe 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -69,7 +69,8 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
struct commit_graph *graph = NULL;
struct object_directory *odb = NULL;
char *graph_name;
- int open_ok;
+ char *chain_name;
+ enum { OPENED_NONE, OPENED_GRAPH, OPENED_CHAIN } opened = OPENED_NONE;
int fd;
struct stat st;
int flags = 0;
@@ -102,21 +103,29 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
odb = find_odb(the_repository, opts.obj_dir);
graph_name = get_commit_graph_filename(odb);
- open_ok = open_commit_graph(graph_name, &fd, &st);
- if (!open_ok && errno != ENOENT)
+ chain_name = get_commit_graph_chain_filename(odb);
+ if (open_commit_graph(graph_name, &fd, &st))
+ opened = OPENED_GRAPH;
+ else if (errno != ENOENT)
die_errno(_("Could not open commit-graph '%s'"), graph_name);
+ else if (open_commit_graph_chain(chain_name, &fd, &st))
+ opened = OPENED_CHAIN;
+ else if (errno != ENOENT)
+ die_errno(_("could not open commit-graph chain '%s'"), chain_name);
FREE_AND_NULL(graph_name);
+ FREE_AND_NULL(chain_name);
FREE_AND_NULL(options);
- if (open_ok)
+ if (opened == OPENED_NONE)
+ return 0;
+ else if (opened == OPENED_GRAPH)
graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);
else
- graph = read_commit_graph_one(the_repository, odb);
+ graph = load_commit_graph_chain_fd_st(the_repository, fd, &st);
- /* Return failure if open_ok predicted success */
if (!graph)
- return !!open_ok;
+ return 1;
ret = verify_commit_graph(the_repository, graph, flags);
free_commit_graph(graph);
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index a9b2428b56..a5ac0440f1 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -317,11 +317,11 @@ test_expect_success 'verify after commit-graph-chain corruption (base)' '
(
cd verify-chain-base &&
corrupt_file "$graphdir/commit-graph-chain" 30 "G" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "invalid commit-graph chain" err &&
corrupt_file "$graphdir/commit-graph-chain" 30 "A" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "unable to find all commit-graph files" err
)
--
2.42.0.773.ga6e30199be
^ permalink raw reply related
* [PATCH v2 5/6] commit-graph: tighten chain size check
From: Jeff King @ 2023-09-28 4:39 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230928043746.GB57926@coredump.intra.peff.net>
When we open a commit-graph-chain file, if it's smaller than a single
entry, we just quietly treat that as ENOENT. That make some sense if the
file is truly zero bytes, but it means that "commit-graph verify" will
quietly ignore a file that contains garbage if that garbage happens to
be short.
Instead, let's only simulate ENOENT when the file is truly empty, and
otherwise return EINVAL. The normal graph-loading routines don't care,
but "commit-graph verify" will notice and complain about the difference.
It's not entirely clear to me that the 0-is-ENOENT case actually happens
in real life, so we could perhaps just eliminate this special-case
altogether. But this is how we've always behaved, so I'm preserving it
in the name of backwards compatibility (though again, it really only
matters for "verify", as the regular routines are happy to load what
they can).
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 10 ++++++++--
t/t5324-split-commit-graph.sh | 12 ++++++++++++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 8b29c6de24..b1d3e5bf94 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -548,9 +548,15 @@ int open_commit_graph_chain(const char *chain_file,
close(*fd);
return 0;
}
- if (st->st_size <= the_hash_algo->hexsz) {
+ if (st->st_size < the_hash_algo->hexsz) {
close(*fd);
- errno = ENOENT;
+ if (!st->st_size) {
+ /* treat empty files the same as missing */
+ errno = ENOENT;
+ } else {
+ warning("commit-graph chain file too small");
+ errno = EINVAL;
+ }
return 0;
}
return 1;
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index a5ac0440f1..be58545810 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -342,6 +342,18 @@ test_expect_success 'verify after commit-graph-chain corruption (tip)' '
)
'
+test_expect_success 'verify notices too-short chain file' '
+ git clone --no-hardlinks . verify-chain-short &&
+ (
+ cd verify-chain-short &&
+ git commit-graph verify &&
+ echo "garbage" >$graphdir/commit-graph-chain &&
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ grep "commit-graph chain file too small" err
+ )
+'
+
test_expect_success 'verify across alternates' '
git clone --no-hardlinks . verify-alt &&
(
--
2.42.0.773.ga6e30199be
^ permalink raw reply related
* [PATCH v2 6/6] commit-graph: report incomplete chains during verification
From: Jeff King @ 2023-09-28 4:39 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230928043746.GB57926@coredump.intra.peff.net>
The load_commit_graph_chain_fd_st() function will stop loading chains
when it sees an error. But if it has loaded any graph slice at all, it
will return it. This is a good thing for normal use (we use what data we
can, and this is just an optimization). But it's a bad thing for
"commit-graph verify", which should be careful about finding any
irregularities. We do complain to stderr with a warning(), but the
verify command still exits with a successful return code.
The new tests here cover corruption of both the base and tip slices of
the chain. The corruption of the base file already works (it is the
first file we look at, so when we see the error we return NULL). The
"tip" case is what is fixed by this patch (it complains to stderr but
still returns the base slice).
Likewise the existing tests for corruption of the commit-graph-chain
file itself need to be updated. We already exited non-zero correctly for
the "base" case, but the "tip" case can now do so, too.
Note that this also causes us to adjust a test later in the file that
similarly corrupts a tip (though confusingly the test script calls this
"base"). It checks stderr but erroneously expects the whole "verify"
command to exit with a successful code.
Signed-off-by: Jeff King <peff@peff.net>
---
v2 now covers failed get_oid_hex() call on non-base lines.
builtin/commit-graph.c | 10 +++++++++-
commit-graph.c | 7 +++++--
commit-graph.h | 3 ++-
t/t5324-split-commit-graph.sh | 32 +++++++++++++++++++++++++++++---
4 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 50c15d9bfe..f5e66e9969 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -74,6 +74,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
int fd;
struct stat st;
int flags = 0;
+ int incomplete_chain = 0;
int ret;
static struct option builtin_commit_graph_verify_options[] = {
@@ -122,13 +123,20 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
else if (opened == OPENED_GRAPH)
graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);
else
- graph = load_commit_graph_chain_fd_st(the_repository, fd, &st);
+ graph = load_commit_graph_chain_fd_st(the_repository, fd, &st,
+ &incomplete_chain);
if (!graph)
return 1;
ret = verify_commit_graph(the_repository, graph, flags);
free_commit_graph(graph);
+
+ if (incomplete_chain) {
+ error("one or more commit-graph chain files could not be loaded");
+ ret |= 1;
+ }
+
return ret;
}
diff --git a/commit-graph.c b/commit-graph.c
index b1d3e5bf94..1a56efcf69 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -563,7 +563,8 @@ int open_commit_graph_chain(const char *chain_file,
}
struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
- int fd, struct stat *st)
+ int fd, struct stat *st,
+ int *incomplete_chain)
{
struct commit_graph *graph_chain = NULL;
struct strbuf line = STRBUF_INIT;
@@ -618,6 +619,7 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
fclose(fp);
strbuf_release(&line);
+ *incomplete_chain = !valid;
return graph_chain;
}
@@ -630,8 +632,9 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
struct commit_graph *g = NULL;
if (open_commit_graph_chain(chain_file, &fd, &st)) {
+ int incomplete;
/* ownership of fd is taken over by load function */
- g = load_commit_graph_chain_fd_st(r, fd, &st);
+ g = load_commit_graph_chain_fd_st(r, fd, &st, &incomplete);
}
free(chain_file);
diff --git a/commit-graph.h b/commit-graph.h
index 3b662fd2b5..20ada7e891 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -107,7 +107,8 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
int fd, struct stat *st,
struct object_directory *odb);
struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
- int fd, struct stat *st);
+ int fd, struct stat *st,
+ int *incomplete_chain);
struct commit_graph *read_commit_graph_one(struct repository *r,
struct object_directory *odb);
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index be58545810..06bb897f02 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -285,6 +285,32 @@ test_expect_success 'verify hashes along chain, even in shallow' '
)
'
+test_expect_success 'verify notices chain slice which is bogus (base)' '
+ git clone --no-hardlinks . verify-chain-bogus-base &&
+ (
+ cd verify-chain-bogus-base &&
+ git commit-graph verify &&
+ base_file=$graphdir/graph-$(sed -n 1p $graphdir/commit-graph-chain).graph &&
+ echo "garbage" >$base_file &&
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ grep "commit-graph file is too small" err
+ )
+'
+
+test_expect_success 'verify notices chain slice which is bogus (tip)' '
+ git clone --no-hardlinks . verify-chain-bogus-tip &&
+ (
+ cd verify-chain-bogus-tip &&
+ git commit-graph verify &&
+ tip_file=$graphdir/graph-$(sed -n 2p $graphdir/commit-graph-chain).graph &&
+ echo "garbage" >$tip_file &&
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ grep "commit-graph file is too small" err
+ )
+'
+
test_expect_success 'verify --shallow does not check base contents' '
git clone --no-hardlinks . verify-shallow &&
(
@@ -306,7 +332,7 @@ test_expect_success 'warn on base graph chunk incorrect' '
git commit-graph verify &&
base_file=$graphdir/graph-$(tail -n 1 $graphdir/commit-graph-chain).graph &&
corrupt_file "$base_file" $(test_oid base) "\01" &&
- git commit-graph verify --shallow 2>test_err &&
+ test_must_fail git commit-graph verify --shallow 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "commit-graph chain does not match" err
)
@@ -332,11 +358,11 @@ test_expect_success 'verify after commit-graph-chain corruption (tip)' '
(
cd verify-chain-tip &&
corrupt_file "$graphdir/commit-graph-chain" 70 "G" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "invalid commit-graph chain" err &&
corrupt_file "$graphdir/commit-graph-chain" 70 "A" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "unable to find all commit-graph files" err
)
--
2.42.0.773.ga6e30199be
^ permalink raw reply related
* Re: [PATCH 05/30] loose: add a mapping between SHA-1 and SHA-256 for loose objects
From: Eric Sunshine @ 2023-09-28 7:14 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Junio C Hamano, git, brian m. carlson, Eric W . Biederman
In-Reply-To: <20230927195537.1682-5-ebiederm@gmail.com>
On Wed, Sep 27, 2023 at 3:56 PM Eric W. Biederman <ebiederm@gmail.com> wrote:
> As part of the transition plan, we'd like to add a file in the .git
> directory that maps loose objects between SHA-1 and SHA-256. Let's
> implement the specification in the transition plan and store this data
> on a per-repository basis in struct repository.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
> diff --git a/loose.c b/loose.c
> @@ -0,0 +1,245 @@
> +static int load_one_loose_object_map(struct repository *repo, struct object_directory *dir)
> +{
> + struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
> + FILE *fp;
> +
> + if (!dir->loose_map)
> + loose_object_map_init(&dir->loose_map);
> +
> + insert_oid_pair(dir->loose_map->to_compat, repo->hash_algo->empty_tree, repo->compat_hash_algo->empty_tree);
> + insert_oid_pair(dir->loose_map->to_storage, repo->compat_hash_algo->empty_tree, repo->hash_algo->empty_tree);
> +
> + insert_oid_pair(dir->loose_map->to_compat, repo->hash_algo->empty_blob, repo->compat_hash_algo->empty_blob);
> + insert_oid_pair(dir->loose_map->to_storage, repo->compat_hash_algo->empty_blob, repo->hash_algo->empty_blob);
> +
> + insert_oid_pair(dir->loose_map->to_compat, repo->hash_algo->null_oid, repo->compat_hash_algo->null_oid);
> + insert_oid_pair(dir->loose_map->to_storage, repo->compat_hash_algo->null_oid, repo->hash_algo->null_oid);
> +
> + strbuf_git_common_path(&path, repo, "objects/loose-object-idx");
> + fp = fopen(path.buf, "rb");
> + if (!fp)
> + return 0;
This early return leaks `path`. At minimum, call
`strbuf_release(&path)` before returning.
> + errno = 0;
> + if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header))
> + goto err;
> + while (!strbuf_getline_lf(&buf, fp)) {
> + const char *p;
> + struct object_id oid, compat_oid;
> + if (parse_oid_hex_algop(buf.buf, &oid, &p, repo->hash_algo) ||
> + *p++ != ' ' ||
> + parse_oid_hex_algop(p, &compat_oid, &p, repo->compat_hash_algo) ||
> + p != buf.buf + buf.len)
> + goto err;
> + insert_oid_pair(dir->loose_map->to_compat, &oid, &compat_oid);
> + insert_oid_pair(dir->loose_map->to_storage, &compat_oid, &oid);
> + }
> +
> + strbuf_release(&buf);
> + strbuf_release(&path);
> + return errno ? -1 : 0;
> +err:
> + strbuf_release(&buf);
> + strbuf_release(&path);
> + return -1;
> +}
> +
> +int repo_write_loose_object_map(struct repository *repo)
> +{
> + kh_oid_map_t *map = repo->objects->odb->loose_map->to_compat;
> + struct lock_file lock;
> + int fd;
> + khiter_t iter;
> + struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
> +
> + if (!should_use_loose_object_map(repo))
> + return 0;
> +
> + strbuf_git_common_path(&path, repo, "objects/loose-object-idx");
> + fd = hold_lock_file_for_update_timeout(&lock, path.buf, LOCK_DIE_ON_ERROR, -1);
> + iter = kh_begin(map);
> + if (write_in_full(fd, loose_object_header, strlen(loose_object_header)) < 0)
> + goto errout;
> +
> + for (; iter != kh_end(map); iter++) {
> + if (kh_exist(map, iter)) {
> + if (oideq(&kh_key(map, iter), the_hash_algo->empty_tree) ||
> + oideq(&kh_key(map, iter), the_hash_algo->empty_blob))
> + continue;
> + strbuf_addf(&buf, "%s %s\n", oid_to_hex(&kh_key(map, iter)), oid_to_hex(kh_value(map, iter)));
> + if (write_in_full(fd, buf.buf, buf.len) < 0)
> + goto errout;
> + strbuf_reset(&buf);
> + }
> + }
Nit: If you call strbuf_reset() immediately before strbuf_addf(), then
you save the reader of the code the effort of having to scan backward
through the function to verify that `buf` is empty the first time
through the loop.
> + strbuf_release(&buf);
> + if (commit_lock_file(&lock) < 0) {
> + error_errno(_("could not write loose object index %s"), path.buf);
> + strbuf_release(&path);
> + return -1;
> + }
> + strbuf_release(&path);
> + return 0;
> +errout:
> + rollback_lock_file(&lock);
> + strbuf_release(&buf);
> + error_errno(_("failed to write loose object index %s\n"), path.buf);
> + strbuf_release(&path);
> + return -1;
> +}
> +
> +int repo_loose_object_map_oid(struct repository *repo,
> + const struct object_id *src,
> + const struct git_hash_algo *to,
> + struct object_id *dest)
> +
> +{
Style: unnecessary blank line before opening `{`
> + struct object_directory *dir;
> + kh_oid_map_t *map;
> + khiter_t pos;
> +
> + for (dir = repo->objects->odb; dir; dir = dir->next) {
> + struct loose_object_map *loose_map = dir->loose_map;
> + if (!loose_map)
> + continue;
> + map = (to == repo->compat_hash_algo) ?
> + loose_map->to_compat :
> + loose_map->to_storage;
> + pos = kh_get_oid_map(map, *src);
> + if (pos < kh_end(map)) {
> + oidcpy(dest, kh_value(map, pos));
> + return 0;
> + }
> + }
> + return -1;
> +}
^ permalink raw reply
* Feature Request - Unique commits list
From: Saurabh Sonar @ 2023-09-28 8:16 UTC (permalink / raw)
To: git
After merging the master branch into my local it is hard to find my
own commits in git history.
To solve this issue I suggest, There should be an option to find out
which commits are only present in only current branch and not present
in any other.
The user should be aware that if he deletes the branch some list of
commits are not merged or present in any other branch and he is going
to lose them forever.
^ permalink raw reply
* Re: Feature Request - Unique commits list
From: Kristoffer Haugsbakk @ 2023-09-28 8:21 UTC (permalink / raw)
To: Saurabh Sonar; +Cc: git
In-Reply-To: <CAJ-L=uAjXB67Zx1BPpdJYF_y1RS+UqwpyTMF1F4o3yO+1o76vg@mail.gmail.com>
On Thu, Sep 28, 2023, at 10:16, Saurabh Sonar wrote:
> The user should be aware that if he deletes the branch some list of
> commits are not merged or present in any other branch and he is going
> to lose them forever.
That already happens now, though? (Well, not “forever” because of GC and
whatnot.) You have to force the deletion with `-D` if the branch is not in
the “upstream” branch.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin
From: Oswald Buddenhagen @ 2023-09-28 9:39 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Junio C Hamano, brian m. carlson, git
In-Reply-To: <87msx99b9o.fsf_-_@gmail.froward.int.ebiederm.org>
just language nits on the commit message:
On Tue, Sep 26, 2023 at 10:58:43AM -0500, Eric W. Biederman wrote:
>Not supporting commits, tags, or trees has no downside as it is not
>currently supported now, and commits, tags, and trees being smaller by
>design do not have the problem that the problem that index_bulk_checkin
^^^^^^^^^^^^^^^^
duplicated!
>was built to solve.
>A version of index_bulk_checkin that supports more than just blobs when
>computing both the SHA-1 and the SHA-256 of every object added would
>need a different, and more expensive structure. The structure is more
>expensive because it would be required to temporarily buffering the
^^^
no 'ing' here.
>equivalent object the compatibility hash needs to be computed over.
>A temporary object is needed, because before a hash over an object can
>computed it's
>
"be computed, its"
>object header needs to be computed. One of the members of
regards
^ permalink raw reply
* [PATCH] doc: correct the 50 characters soft limit
From: 謝致邦 (XIE Zhibang) via GitGitGadget @ 2023-09-28 9:59 UTC (permalink / raw)
To: git
Cc: 謝致邦 (XIE Zhibang),
谢致邦 (XIE Zhibang)
From: =?UTF-8?q?=E8=B0=A2=E8=87=B4=E9=82=A6=20=28XIE=20Zhibang=29?=
<Yeking@Red54.com>
The soft limit of the first line of the commit message should be
"no more than 50 characters" or "50 characters or less", but not
"less than 50 character".
Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
---
doc: correct the 50 characters soft limit
The soft limit of the first line of the commit message should be "no
more than 50 characters" or "50 characters or less", but not "less than
50 character".
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1580%2FRed54%2Fdoc-patch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1580/Red54/doc-patch-v1
Pull-Request: https://github.com/git/git/pull/1580
Documentation/git-commit.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 225c6c9f2e5..a6cef5d8203 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -541,7 +541,7 @@ DISCUSSION
----------
Though not required, it's a good idea to begin the commit message
-with a single short (less than 50 character) line summarizing the
+with a single short (no more than 50 characters) line summarizing the
change, followed by a blank line and then a more thorough description.
The text up to the first blank line in a commit message is treated
as the commit title, and that title is used throughout Git.
base-commit: bcb6cae2966cc407ca1afc77413b3ef11103c175
--
gitgitgadget
^ permalink raw reply related
* Re: Feature Request - Unique commits list
From: Junio C Hamano @ 2023-09-28 14:07 UTC (permalink / raw)
To: Saurabh Sonar; +Cc: git
In-Reply-To: <CAJ-L=uAjXB67Zx1BPpdJYF_y1RS+UqwpyTMF1F4o3yO+1o76vg@mail.gmail.com>
Saurabh Sonar <saurabh.sonar120@gmail.com> writes:
> After merging the master branch into my local it is hard to find my
> own commits in git history.
I suspect the above may be the other way around (i.e., after the
upstream takes some but not all of my changes into their 'master'
branch, I want to find out what remaining topics from me are still
not accepted by them), but anyway, doesn't something along the lines
of
$ git log --branches --not origin/master
work for you? It asks
* Please list commits ("git log")
* The commits to be listed should be discovered by traversing the
history starting from my branch tips ("--branches")
* But I am not interested in those commits that can be reached from
the 'master' branch of my upstream ("--not origin/master").
^ permalink raw reply
* Re: [PATCH] doc: correct the 50 characters soft limit
From: Junio C Hamano @ 2023-09-28 14:10 UTC (permalink / raw)
To: 謝致邦 (XIE Zhibang) via GitGitGadget
Cc: git, 謝致邦 (XIE Zhibang)
In-Reply-To: <pull.1580.git.git.1695895155985.gitgitgadget@gmail.com>
"謝致邦 (XIE Zhibang) via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> The soft limit of the first line of the commit message should be
> "no more than 50 characters" or "50 characters or less", but not
> "less than 50 character".
The limit being "soft", I highly doubt that anybody would wonder if
a title that is exactly 50 columns wide is or is not acceptable.
The updated one is more grammatically correct than the original, so
the patch is not without merit, though ;-)
Thanks.
> ...
> Though not required, it's a good idea to begin the commit message
> -with a single short (less than 50 character) line summarizing the
> +with a single short (no more than 50 characters) line summarizing the
> change, followed by a blank line and then a more thorough description.
> The text up to the first blank line in a commit message is treated
> as the commit title, and that title is used throughout Git.
>
> base-commit: bcb6cae2966cc407ca1afc77413b3ef11103c175
^ permalink raw reply
* Supporting `git add -a <exclude submodules>`
From: Joanna Wang @ 2023-09-28 17:06 UTC (permalink / raw)
To: git
Hi,
We're looking for feedback on the idea of adding git support for users
to express `git add -a <exclude submodules>`.
We recently added submodules to our project.
A lot of our users don't like that when their submodules are on
commits that do not match what is pinned in the root superproject,
running `git add -a` and `git commit -a` (which they are so used to),
will now include those submodules that 99% of the time they do not
want to include.
So we think supporting `git add -a <exclude submodules>` and `git
commit -a <exclude submodules>` might be good to do.
(We know `git -c diff.ignoreSubmodules=all commit -a` is an option,
but this does not work for `git add`. However this support gets
implemented for `git add`, I assume it's ideal for the UX of "don't
include submodules" to be the same for both `git commit` and `git
add`, though I don't think this is a requirement)
In a separate high-level conversation, Junio brought up going about
this with magic pathsepc.
So the work would entail:
- adding 'native' submodule attribute support, so `git add -a
':(exclude,attr:submodule)' can work without having to add 'submodule'
for every submodule path in .gitattributes
- add magic pathspec support to `git add`
(It looks like `git commit -a` does not work with pathspec. In this
case I think that would be fine, we can just tell users they can set
aliases for `git -c diff.ignoreSubmodules=all commit -a` and `git add
-a ':(exclude,attr:submodule)' if they want)
Any thoughts from folks on all of the above?
Thanks!
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Junio C Hamano @ 2023-09-28 18:11 UTC (permalink / raw)
To: Joanna Wang; +Cc: git
In-Reply-To: <CAL-HyEbxGqxid3vsvDk3Z0Gd3swbNP4qcXhApadu8ZgvRkr3WA@mail.gmail.com>
Joanna Wang <jojwang@chromium.org> writes:
> So the work would entail:
> - adding 'native' submodule attribute support, so `git add -a
> ':(exclude,attr:submodule)' can work without having to add 'submodule'
> for every submodule path in .gitattributes
I personally do not think it is a great interface that allows you to
only say "all submodules in my tree, are equally special in the same
way, and I do not want any of them" without allowing you to say
"paths in this directory, be they files, symlinks, or submodules,
are special and I do not want any of them". The former smells like
a very narrow hack that does not generally extend. So compared to
"git add -a --exclude-submodules", a solution using magic pathspec
mechanism and syntax would be a great improvement.
The spelling of the attribute (if we were to use the "attr" magic,
that is) may want to allow a bit more flexibility, e.g.,
"attr:type=gitlink", or even "attr:bits=160000", that would allow
natural extension to cover symbolic links and other oddities.
> - add magic pathspec support to `git add`
Yup, I do not offhand remember why "git add" does not want to take
the magic pathspec with an "attr" component. Whoever wants to tip
in to this effort may want to first try what breaks when it is
enabled without changing anything, write tests, and then fix the
fallouts, which can be done separately and in parallel with an
effort to design how the way to express "does this path represent a
submodule? a regular blob? a symbolic link? something else?" with
the magic pathspec syntax (i.e., choose among attr:submodule,
attr:type=<what>, attr:bits=<permbits>, decide what keyword to use),
and implement that design.
> (It looks like `git commit -a` does not work with pathspec.
You should be able to do something like
$ git commit . ':(exclude)t/'
I think.
By the way, I am surprised that "diff.ignoreSubmodules=all" is
abused by anybody that way. It depends on the implementation detail
that we internally use the diff machinery to find which paths are
not modified, and I would even say it is a BUG that "commit -a" pays
attention to the configuration variable that way. I would recommend
strongly to stay away from that approach or your tools will get
broken when the bug gets fixed.
Also, I have doubts on the higher level workflow issue that makes
your people want to keep their submodules (or any subdirectories for
that matter) dirty and out of sync with the superproject, and commit
their work product from that state. It means whatever their local
building and testing gave them cannot be trusted when evaluating the
resulting commit at the superproject level.
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Junio C Hamano @ 2023-09-28 18:16 UTC (permalink / raw)
To: Joanna Wang; +Cc: git
In-Reply-To: <xmqqsf6yjhhm.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> (It looks like `git commit -a` does not work with pathspec.
>
> You should be able to do something like
>
> $ git commit . ':(exclude)t/'
>
> I think.
Correction. You would need the "-i(nclude)" option if you do the
above, as the default is "-o(nly)", which will deliberately ignore
what has been added to the index.
^ permalink raw reply
* Re: Supporting `git add -a <exclude submodules>`
From: Junio C Hamano @ 2023-09-28 18:24 UTC (permalink / raw)
To: Joanna Wang; +Cc: git
In-Reply-To: <xmqqsf6yjhhm.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> By the way, I am surprised that "diff.ignoreSubmodules=all" is
> abused by anybody that way. It depends on the implementation detail
> that we internally use the diff machinery to find which paths are
> not modified, and I would even say it is a BUG that "commit -a" pays
> attention to the configuration variable that way. I would recommend
> strongly to stay away from that approach or your tools will get
> broken when the bug gets fixed.
Sorry, but I take this part back. This is an explicitly documented
feature of the variable, so users and scripts alike should be able
to depend on. It still feels like a layering violation, but it is
too late to "fix" it now.
^ permalink raw reply
* Re: [PATCH 21/30] repository: Implement extensions.compatObjectFormat
From: Junio C Hamano @ 2023-09-28 20:18 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson, Eric W. Biederman
In-Reply-To: <xmqqfs2zl2iy.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>
>> diff --git a/setup.c b/setup.c
>> index deb5a33fe9e1..87b40472dbc5 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -598,6 +598,25 @@ static enum extension_result handle_extension(const char *var,
>> }
>
> This line in the pre-context needed fuzzing, but otherwise the
> series applied cleanly on top of v2.42.0.
>
>> Subject: Re: [PATCH 21/30] repository: Implement extensions.compatObjectFormat
>
> "Implement" -> "implement" (many other patches share the same
> problem, none of which I fixed while queueing).
The topic when merged near the tip of 'seen' seems to break a few CI
jobs here and there. The log from the broken run can be seen at
https://github.com/git/git/actions/runs/6331978214
You may have to log-in there before you can view the details.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] diff --stat: set the width defaults in a helper function
From: Junio C Hamano @ 2023-09-28 20:27 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <0570bd5c26caef1ff66a643558305fb0@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Just a brief reminder about version 2 of this patch...
It still is a bit premature to queue this before the topic, to which
this is a clean-up for, graduates to the 'master' branch, though.
^ permalink raw reply
* Re: [PATCH v2] diff --stat: set the width defaults in a helper function
From: Dragan Simic @ 2023-09-28 20:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7coajb6a.fsf@gitster.g>
On 2023-09-28 22:27, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Just a brief reminder about version 2 of this patch...
>
> It still is a bit premature to queue this before the topic, to which
> this is a clean-up for, graduates to the 'master' branch, though.
Oh, I agree, thanks. Actually, I just wanted to check with you does the
version 2 of this patch require any further cleanups or improvements?
^ permalink raw reply
* Please I want to discuess something with you reply me Urgent
From: M X chris colin @ 2023-09-28 21:35 UTC (permalink / raw)
To: git
Holle,
Please I want to discuess something with you reply me for more informations.
COLIN
^ permalink raw reply
* Re: [silly] loose, pack, and another thing?
From: Jonathan Tan @ 2023-09-28 21:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git
In-Reply-To: <xmqqbkdometi.fsf@gitster.g>
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).
> "fsck" needs to be told about how to verify them. Create the object
> header in-core and hash that, followed by the contents of that file,
> and make sure the result matches the <hex-object-name> part of the
> filename, or something like that.
Yeah, this sounds like what index-pack is doing - the hash algo can take
the contents of one buffer (a header that we synthesize ourselves), and
then take the contents of another buffer (the file contents).
^ permalink raw reply
* Re: [silly] loose, pack, and another thing?
From: Jonathan Tan @ 2023-09-28 21:47 UTC (permalink / raw)
To: Christian Couder; +Cc: Jonathan Tan, Junio C Hamano, git
In-Reply-To: <CAP8UFD0FLsEV4y4W8Vkr5PP_7F4x-nLdHUVx5WRP8BuJnW869A@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> > 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.
> >
> > "fsck" needs to be told about how to verify them. Create the object
> > header in-core and hash that, followed by the contents of that file,
> > and make sure the result matches the <hex-object-name> part of the
> > filename, or something like that.
>
> What happens when they are transferred? Should the remote unpack them
> into the same kind of verbatim object?
I think that the design space is vast and needs to be discussed, perhaps
independently of the local repo case (in which for a start, we could
just detect large blobs being added to the index and put them in our
new object store instead of loose/packed storage, and make sure that we
never repack them). Some concerns during fetch:
- Servers would probably want to serve the large blobs via CDN, so we
probably need something similar to packfile-uris. Would servers also
want to inline these blobs? (If not, we don't need to design this part.)
- Would servers be willing to zlib-compress large blobs (into packfile
format) if the client doesn't support verbatim objects?
And during push:
- Clients probably want to be able to inline large blobs when pushing.
Should it also be possible to specify the large blob via URI, and if
yes, how does the server tell the client what URIs are acceptable?
^ permalink raw reply
* [PATCH] MyFirstContribution: add Discord server link
From: Calvin Wan @ 2023-09-28 21:59 UTC (permalink / raw)
To: git; +Cc: Calvin Wan
Link for new contributors to join the Discord server -- a separate pull
request to add the link to the Community page has also been added.
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
Documentation/MyFirstContribution.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt
index 62d11a5cd7..3751131a00 100644
--- a/Documentation/MyFirstContribution.txt
+++ b/Documentation/MyFirstContribution.txt
@@ -59,6 +59,12 @@ respond to you. It's better to ask your questions in the channel so that you
can be answered if you disconnect and so that others can learn from the
conversation.
+==== https://discord.gg/GRFVkzgxRd[Git Community Discord Server]
+
+The Git Community Discord Server also has many knowledgeable and helpful
+people. Additionally, it provides a space to be able to voice chat about
+patches, designs, or anything else Git related.
+
[[getting-started]]
== Getting Started
--
2.42.0.582.g8ccd20d70d-goog
^ 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