* [PATCH 4/6] commit-graph: detect read errors when verifying graph chain
From: Jeff King @ 2023-09-26 6:00 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@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 e335ef87a6..0ac7bbd1dc 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' '
(
cd verify-chain &&
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.758.gd56856b565
^ permalink raw reply related
* [PATCH 3/6] t5324: harmonize sha1/sha256 graph chain corruption
From: Jeff King @ 2023-09-26 5:58 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@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 this test behave consistently with either hash by always
corrupting the first line. We'll add additional tests that explicitly
cover the second line as we fix those bugs.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t5324-split-commit-graph.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 36c4141e67..e335ef87a6 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -316,11 +316,11 @@ test_expect_success 'verify after commit-graph-chain corruption' '
git clone --no-hardlinks . verify-chain &&
(
cd verify-chain &&
- corrupt_file "$graphdir/commit-graph-chain" 60 "G" &&
+ 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
--
2.42.0.758.gd56856b565
^ permalink raw reply related
* [PATCH 2/6] commit-graph: check mixed generation validation when loading chain file
From: Jeff King @ 2023-09-26 5:57 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@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>
---
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.758.gd56856b565
^ permalink raw reply related
* [PATCH 1/6] commit-graph: factor out chain opening function
From: Jeff King @ 2023-09-26 5:56 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@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>
---
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.758.gd56856b565
^ permalink raw reply related
* [PATCH 0/6] some "commit-graph verify" fixes for chains
From: Jeff King @ 2023-09-26 5:54 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
While yak-shaving another topic, I noticed that there are a bunch of
error cases that "git commit-graph verify" does not notice when checking
chained graph files. This series should make it more robust.
Some of these cases are actually covered in t5324 already, but the tests
expect the verify command not to fail, which just seems wrong. I don't
see anything in the commit message or mailing list explaining it, so I
think it was just "well, this is how it behaves now", and not some
clever scheme.
[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 | 48 +++++++++++++--
4 files changed, 141 insertions(+), 51 deletions(-)
-Peff
PS If you are curious about the yak-shave, it is:
3. "commit-graph verify" fails to propagate some errors (this
series)
2. we are missing a bunch of bounds checks on commit-graph files;
fixing that makes the tests inconsistent because of (3)
1. there are some unused parameters that should be used for bounds
checks, hence (2)
Just in case anybody thought there was a part of my life that was not
motivated by removing -Wunused-parameter warnings.
^ permalink raw reply
* Re: [PATCH v3 0/9] Trailer readability cleanups
From: Linus Arver @ 2023-09-26 5:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Glen Choo, Christian Couder,
Phillip Wood
In-Reply-To: <xmqqo7htww7k.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>>> I could revert and discard [4-6/6] of the previous iteration out of
>>> 'next' and have only the first three (which I thought have been
>>> adequately reviewed without remaining issues) graduate to 'master',
>>> if it makes it easier to fix this update on top, but I'd rather not
>>> to encourage people to form a habit of reverting changes out of
>>> 'next'.
>>>
>>> Thanks.
>>
>> I totally agree that reverting changes out of next is undesirable. I
>> will do a reroll on top of 'next' with only those incremental (new)
>> patches.
>
> OK, so the first 3 patches are now in 'master', and the remainder of
> the previous series have been discarded.
>
> Thanks.
Oh, that simplifies things. I will re-roll on top of 'master' as the
starting point for the other remaining patches instead of using 'next'
as I suggested earlier.
^ permalink raw reply
* Re: [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
From: Linus Arver @ 2023-09-26 5:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Christian Couder, Phillip Wood
In-Reply-To: <xmqqil820z25.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> ... I prefer the
>> WHERE_UNSPECIFIED as in this patch because the WHERE_DEFAULT is
>> ambiguous on its own (i.e., WHERE_DEFAULT could mean that we either use
>> the default value WHERE_END in default_conf_info, or it could mean that
>> we fall back to the configuration variables (where it could be something
>> else)).
>
> Yup. "Turning something that is left UNSPECIFIED after command line
> options and configuration files are processed into the hardcoded
> DEFAULT" is one mental model that is easy to explain.
>
> I however am not sure if it is easier than "Setting something to
> hardcoded DEFAULT before command line options and configuration
> files are allowed to tweak it, and if nobody touches it, then it
> gets the hardcoded DEFAULT value in the end", which is another valid
> mental model, though.
True.
> If both can be used, I'd personally prefer
> the latter, and reserve the "UNSPECIFIED to DEFAULT" pattern to
> signal that we are dealing with a case where the simpler pattern
> without UNSPECIFIED cannot solve.
SGTM.
> The simpler pattern would not work, when the default is defined
> depending on a larger context. Imagine we have two Boolean
> variables, A and B, where A defaults to false, and B defaults to
> some value derived from the value of A (say, opposite of A).
>
> In the most natural implementation, you'd initialize A to false and
> B to unspecified, let command line options and configuration
> variables to set them to true or false, and after all that, you do
> not have to tweak A's value (it will be left to false that is the
> default unless the user or the configuration gave an explicit
> value), but you need to check if B is left unspecified and tweak it
> to true or false using the final value of A.
>
> For a variable with such a need like B, we cannot avoid having
> "unspecified". If you initialize it to false (or true), after the
> command line and the configuration files are read and you find B is
> set to false (or true), you cannot tell if the user or the
> configuration explicitly set B to false (or true), in which case you
> do not want to futz with its value based on what is in A, or it is
> false (or true) only because nobody touched it, in which case you
> need to compute its value based on what is in A.
Thanks for the illustrative example! I don't think we have a case of a
"B" variable here for trailers.
> And that is why I asked if we need to special case "the user did not
> touch and the variable is left untouched" in the trailer subsystem.
I think the answer is "no, we don't need to special case". I'll be
dropping this patch in the next re-roll.
> Thanks.
^ permalink raw reply
* Re: [PATCH] pretty-formats.txt: fix whitespace
From: Josh Soref @ 2023-09-26 2:51 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Josh Soref via GitGitGadget, git, Junio C Hamano
In-Reply-To: <f3081838-ca50-4a7d-b2fc-6f0f1f1364a8@app.fastmail.com>
> From: Josh Soref <jsoref@gmail.com>
> * two spaces after periods for sentences
Kristoffer Haugsbakk wrote:
> Where's the two-spacing convention documented? I didn't find it in
> `CodingGuidelines` or `SubmittingPatches`.
There appeared to be too many instances of two spaces after a period
in the file to be accidental. In some ways, it was an invitation for
people to indicate if they have a preference.
I personally do not have a preference between two spaces after and one
space after, but I do have a preference against a mix of two spaces
after and one space after.
As it (IMO) takes effort (admittedly trivial, and quite likely
habitual) to insert a second space, I assume that this was a desired
thing and the one space was the aberration, hence the patch went in
favor of two spaces.
I'd just as soon submit a patch the other way (and if people have an
opinion one way or the other, I'm happy to include a change to
CodingGuidelines/SubmittingPatches based on people's opinions).
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Junio C Hamano @ 2023-09-26 2:50 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87ttrudkw9.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> P.S. I also figure that maybe our divergence comes from the fact that I
> consider merge commits to be primarily commits (introducing particular
> set of changes, and then having reference to the source of the changes),
> whereas you consider them primarily merges (joining two histories, and
> then maybe some artificial changes that make merges "evil"). That's why
> we often end up agreeing to disagree, as both these points of view seem
> pretty valid.
It rarely is the case that two opposing world views are equally
valid, though.
If there were an option that forbids any comparison output from a
single parent commit (say --ndfnm "no-diff-for-non-merge"), then
those with "merges are the primary thing, single-parent commits on
the merged branches are implementation details" worldview would be
commonly using "--diff-merges=first-parent --patch --ndfnm" and (1)
viewing only the combined effect of merging side branches without
seeing noise from individual commits whose effects are already shown
in these merges, and (2) traversing the side branches as well, so
that merges from side-side branches into the side branches are
viewable the same way as merges into the mainline.
But because no such option exists and nobody asked for such an
option during the whole lifetime of the project, I highly doubt
that it is a valid world view with wide backing from the users.
Even if it were a valid world view with wide backing, the
combination "--diff-merges=first-parent --patch" would be less than
ideal presentation for them (due to lack of "--ndfnm"). And as I
already said, it would not be useful without --first-parent
traversal for the worldview git has supported.
That is why I find it of dubious value to let short-and-sweet '-d'
be squatted by less than ideal "--diff-merges=first-parent --patch"
combination. Shorthands are scarse resources, and we want to be
careful before handing them out.
Thanks.
^ permalink raw reply
* What's cooking in git.git (Sep 2023, #08; Mon, 25)
From: Junio C Hamano @ 2023-09-26 0:50 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
With Contributors' Summit happening this week, the early part of
the week may be slower than usual (my response time will certainly
be longer).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[New Topics]
* jx/remote-archive-over-smart-http (2023-09-25) 3 commits
- archive: support remote archive from stateless transport
- transport-helper: run do_take_over in connect_helper
- transport-helper: no connection restriction in connect_helper
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
Expecting a reroll.
cf. <CANYiYbFkG+CvrNFBkdNewZs7ADROVsjd051SDQsU0zVq8eBhew@mail.gmail.com>
source: <20230923152201.14741-1-worldhello.net@gmail.com>
* jx/sideband-chomp-newline-fix (2023-09-25) 3 commits
- pkt-line: do not chomp newlines for sideband messages
- pkt-line: memorize sideband fragment in reader
- test-pkt-line: add option parser for unpack-sideband
Sideband demultiplexer fixes.
Needs review.
source: <CANYiYbF+Xmk4rCNLMJe+i_CFafg8=QU5vbXWNUZbOVsDLTe5QQ@mail.gmail.com>
* ks/ref-filter-mailmap (2023-09-25) 3 commits
- ref-filter: add mailmap support
- t/t6300: introduce test_bad_atom
- t/t6300: cleanup test_atom
"git for-each-ref" and friends learn to apply mailmap to authorname
and other fields.
Will merge to 'next'.
source: <20230925175050.3498-1-five231003@gmail.com>
* ps/revision-cmdline-stdin-not (2023-09-25) 1 commit
- revision: make pseudo-opt flags read via stdin behave consistently
"git rev-list --stdin" learned to take non-revisions (like "--not")
recently from the standard input, but the way such a "--not" was
handled was quite confusing, which has been rethought. This is
potentially a change that breaks backward compatibility.
Will merge to 'next'?
source: <6221acd2796853144f8e84081655fbc79fdc6634.1695646898.git.ps@pks.im>
* ty/merge-tree-strategy-options (2023-09-25) 1 commit
- merge-tree: add -X strategy option
"git merge-tree" learned to take strategy backend specific options
via the "-X" option, like "git merge" does.
Will merge to 'next'?
source: <pull.1565.v6.git.1695522222723.gitgitgadget@gmail.com>
--------------------------------------------------
[Stalled]
* tk/cherry-pick-sequence-requires-clean-worktree (2023-06-01) 1 commit
- cherry-pick: refuse cherry-pick sequence if index is dirty
"git cherry-pick A" that replays a single commit stopped before
clobbering local modification, but "git cherry-pick A..B" did not,
which has been corrected.
Expecting a reroll.
cf. <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
source: <pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com>
* jc/diff-cached-fsmonitor-fix (2023-09-15) 3 commits
- diff-lib: fix check_removed() when fsmonitor is active
- Merge branch 'jc/fake-lstat' into jc/diff-cached-fsmonitor-fix
- Merge branch 'js/diff-cached-fsmonitor-fix' into jc/diff-cached-fsmonitor-fix
(this branch uses jc/fake-lstat.)
The optimization based on fsmonitor in the "diff --cached"
codepath is resurrected with the "fake-lstat" introduced earlier.
It is unknown if the optimization is worth resurrecting, but in case...
source: <xmqqr0n0h0tw.fsf@gitster.g>
--------------------------------------------------
[Cooking]
* hy/doc-show-is-like-log-not-diff-tree (2023-09-20) 1 commit
(merged to 'next' on 2023-09-22 at 5492c03eae)
+ show doc: redirect user to git log manual instead of git diff-tree
Doc update.
Will merge to 'master'.
source: <20230920132731.1259-1-hanyang.tony@bytedance.com>
* jc/alias-completion (2023-09-20) 1 commit
(merged to 'next' on 2023-09-22 at 1d069e900b)
+ completion: loosen and document the requirement around completing alias
The command line completion script (in contrib/) can be told to
complete aliases by including ": git <cmd> ;" in the alias to tell
it that the alias should be completed similar to how "git <cmd>" is
completed. The parsing code for the alias as been loosened to
allow ';' without an extra space before it.
Will merge to 'master'.
cf. <owlyjzssjro2.fsf@fine.c.googlers.com>
source: <xmqqy1h08zsp.fsf_-_@gitster.g>
* jk/test-pass-ubsan-options-to-http-test (2023-09-21) 1 commit
(merged to 'next' on 2023-09-22 at bbe2f75937)
+ test-lib: set UBSAN_OPTIONS to match ASan
UBSAN options were not propagated through the test framework to git
run via the httpd, unlike ASAN options, which has been corrected.
Will merge to 'master'.
source: <20230921041825.GA2814583@coredump.intra.peff.net>
* ob/am-msgfix (2023-09-21) 1 commit
(merged to 'next' on 2023-09-22 at 7f7589a06a)
+ am: fix error message in parse_opt_show_current_patch()
The parameters to generate an error message have been corrected.
Will merge to 'master'.
source: <20230921110727.789156-1-oswald.buddenhagen@gmx.de>
* js/ci-coverity (2023-09-25) 7 commits
- SQUASH???
- coverity: detect and report when the token or project is incorrect
- coverity: allow running on macOS
- coverity: support building on Windows
- coverity: allow overriding the Coverity project
- coverity: cache the Coverity Build Tool
- ci: add a GitHub workflow to submit Coverity scans
GitHub CI workflow has learned to trigger Coverity check.
Looking good.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
* js/doc-status-with-submodules-mark-up-fix (2023-09-22) 1 commit
(merged to 'next' on 2023-09-25 at 7ed318fc91)
+ Documentation/git-status: add missing line breaks
Docfix.
Will merge to 'master'.
source: <pull.1590.git.1695392082207.gitgitgadget@gmail.com>
* js/config-parse (2023-09-21) 5 commits
- config-parse: split library out of config.[c|h]
- config.c: accept config_parse_options in git_config_from_stdin
- config: report config parse errors using cb
- config: split do_event() into start and flush operations
- config: split out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
source: <cover.1695330852.git.steadmon@google.com>
* ml/git-gui-exec-path-fix (2023-09-18) 3 commits
(merged to 'next' on 2023-09-19 at 0565b0b14b)
+ Merge git-gui into ml/git-gui-exec-path-fix
+ git-gui - use git-hook, honor core.hooksPath
+ git-gui - re-enable use of hook scripts
Fix recent regression in Git-GUI that fails to run hook scripts at
all.
Will merge to 'master'.
* ds/stat-name-width-configuration (2023-09-18) 1 commit
(merged to 'next' on 2023-09-22 at dbf5bd96e8)
+ diff --stat: add config option to limit filename width
"git diff" learned diff.statNameWidth configuration variable, to
give the default width for the name part in the "--stat" output.
Will merge to 'master'.
source: <87badb12f040d1c66cd9b89074d3de5015a45983.1694446743.git.dsimic@manjaro.org>
* jk/fsmonitor-unused-parameter (2023-09-18) 8 commits
(merged to 'next' on 2023-09-19 at bd06505f9e)
+ run-command: mark unused parameters in start_bg_wait callbacks
+ fsmonitor: mark unused hashmap callback parameters
+ fsmonitor/darwin: mark unused parameters in system callback
+ fsmonitor: mark unused parameters in stub functions
+ fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
+ fsmonitor: mark some maybe-unused parameters
+ fsmonitor/win32: drop unused parameters
+ fsmonitor: prefer repo_git_path() to git_pathdup()
Unused parameters in fsmonitor related code paths have been marked
as such.
Will merge to 'master'.
source: <20230918222908.GA2659096@coredump.intra.peff.net>
* jc/fake-lstat (2023-09-15) 1 commit
- cache: add fake_lstat()
(this branch is used by jc/diff-cached-fsmonitor-fix.)
A new helper to let us pretend that we called lstat() when we know
our cache_entry is up-to-date via fsmonitor.
Needs review.
source: <xmqqcyykig1l.fsf@gitster.g>
* kn/rev-list-ignore-missing-links (2023-09-20) 1 commit
- revision: add `--ignore-missing-links` user option
Surface the .ignore_missing_links bit that stops the revision
traversal from stopping and dying when encountering a missing
object to a new command line option of "git rev-list", so that the
objects that are required but are missing can be enumerated.
Waiting for review response.
source: <20230920104507.21664-1-karthik.188@gmail.com>
* kh/range-diff-notes (2023-09-19) 1 commit
(merged to 'next' on 2023-09-22 at ac04978b4b)
+ range-diff: treat notes like `log`
"git range-diff --notes=foo" compared "log --notes=foo --notes" of
the two ranges, instead of using just the specified notes tree.
Will merge to 'master'.
source: <6e114271a2e7d2323193bd58bb307f60101942ce.1695154855.git.code@khaugsbakk.name>
* rs/parse-options-value-int (2023-09-18) 2 commits
- parse-options: use and require int pointer for OPT_CMDMODE
- parse-options: add int value pointer to struct option
A bit of type safety for the "value" pointer used in the
parse-options API.
Comments?
source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
* so/diff-merges-d (2023-09-11) 2 commits
- diff-merges: introduce '-d' option
- diff-merges: improve --diff-merges documentation
Teach a new "-d" option that shows the patch against the first
parent for merge commits (which is "--diff-merges=first-parent -p").
Letting a less useful combination of options squat on short-and-sweet "-d" feels dubious.
source: <20230909125446.142715-1-sorganov@gmail.com>
* cc/repack-sift-filtered-objects-to-separate-pack (2023-09-25) 10 commits
- SQUASH??? t0080 is already taken
- gc: add `gc.repackFilterTo` config option
- repack: implement `--filter-to` for storing filtered out objects
- gc: add `gc.repackFilter` config option
- repack: add `--filter=<filter-spec>` option
- pack-bitmap-write: rebuild using new bitmap when remapping
- repack: refactor finding pack prefix
- repack: refactor finishing pack-objects command
- t/helper: add 'find-pack' test-tool
- pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
Looking better.
source: <20230925152517.803579-1-christian.couder@gmail.com>
* pw/rebase-sigint (2023-09-07) 1 commit
- rebase -i: ignore signals when forking subprocesses
If the commit log editor or other external programs (spawned via
"exec" insn in the todo list) receive internactive signal during
"git rebase -i", it caused not just the spawned program but the
"Git" process that spawned them, which is often not what the end
user intended. "git" learned to ignore SIGINT and SIGQUIT while
waiting for these subprocesses.
Expecting a reroll.
cf. <12c956ea-330d-4441-937f-7885ab519e26@gmail.com>
source: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
* cw/git-std-lib (2023-09-11) 7 commits
- SQUASH???
- git-std-lib: add test file to call git-std-lib.a functions
- git-std-lib: introduce git standard library
- parse: create new library for parsing strings and env values
- config: correct bad boolean env value error message
- wrapper: remove dependency to Git-specific internal file
- hex-ll: split out functionality from hex
Another libification effort.
Needs more work.
cf. <xmqqy1hfrk6p.fsf@gitster.g>
cf. <20230915183927.1597414-1-jonathantanmy@google.com>
source: <20230908174134.1026823-1-calvinwan@google.com>
* cc/git-replay (2023-09-07) 15 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: disallow revision specific options and pathspecs
- replay: use standard revision ranges
- replay: make it a minimal server side command
- replay: remove HEAD related sanity check
- replay: remove progress and info output
- replay: add an important FIXME comment about gpg signing
- replay: don't simplify history
- replay: introduce pick_regular_commit()
- replay: die() instead of failing assert()
- replay: start using parse_options API
- replay: introduce new builtin
- t6429: remove switching aspects of fast-rebase
Waiting for review response.
cf. <52277471-4ddd-b2e0-62ca-c2a5b59ae418@gmx.de>
cf. <58daa706-7efb-51dd-9061-202ef650b96a@gmx.de>
cf. <f0e75d47-c277-9fbb-7bcd-53e4e5686f3c@gmx.de>
May want to wait until tb/repack-existing-packs-cleanup stabilizes.
source: <20230907092521.733746-1-christian.couder@gmail.com>
* la/trailer-test-and-doc-updates (2023-09-07) 13 commits
- trailer doc: <token> is a <key> or <keyAlias>, not both
- trailer doc: separator within key suppresses default separator
- trailer doc: emphasize the effect of configuration variables
- trailer --unfold help: prefer "reformat" over "join"
- trailer --parse docs: add explanation for its usefulness
- trailer --only-input: prefer "configuration variables" over "rules"
- trailer --parse help: expose aliased options
- trailer --no-divider help: describe usual "---" meaning
- trailer: trailer location is a place, not an action
- trailer doc: narrow down scope of --where and related flags
- trailer: add tests to check defaulting behavior with --no-* flags
- trailer test description: this tests --where=after, not --where=before
- trailer tests: make test cases self-contained
Test coverage for trailers has been improved.
source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
* js/doc-unit-tests (2023-08-17) 3 commits
- ci: run unit tests in CI
- unit tests: add TAP unit test framework
- unit tests: Add a project plan document
(this branch is used by js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Waiting for review response.
cf. <xmqq350hw6n7.fsf@gitster.g>
source: <cover.1692297001.git.steadmon@google.com>
* js/doc-unit-tests-with-cmake (2023-09-25) 7 commits
- cmake: handle also unit tests
- cmake: use test names instead of full paths
- cmake: fix typo in variable name
- artifacts-tar: when including `.dll` files, don't forget the unit-tests
- unit-tests: do show relative file paths
- unit-tests: do not mistake `.pdb` files for being executable
- cmake: also build unit tests
(this branch uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
Waiting for the base topic to settle.
source: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
* tb/path-filter-fix (2023-08-30) 15 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Still being discussed.
cf. <20230830200218.GA5147@szeder.dev>
cf. <20230901205616.3572722-1-jonathantanmy@google.com>
cf. <20230924195900.GA1156862@szeder.dev>
source: <cover.1693413637.git.jonathantanmy@google.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
(this branch uses jc/unresolve-removal.)
Code clean-up.
Not ready to be reviewed yet.
source: <20230731224409.4181277-1-gitster@pobox.com>
* jc/unresolve-removal (2023-07-31) 7 commits
(merged to 'next' on 2023-09-25 at 0563c8d8a1)
+ checkout: allow "checkout -m path" to unmerge removed paths
+ checkout/restore: add basic tests for --merge
+ checkout/restore: refuse unmerging paths unless checking out of the index
+ update-index: remove stale fallback code for "--unresolve"
+ update-index: use unmerge_index_entry() to support removal
+ resolve-undo: allow resurrecting conflicted state that resolved to deletion
+ update-index: do not read HEAD and MERGE_HEAD unconditionally
(this branch is used by jc/rerere-cleanup.)
"checkout --merge -- path" and "update-index --unresolve path" did
not resurrect conflicted state that was resolved to remove path,
but now they do.
Will merge to 'master'.
source: <20230731224409.4181277-1-gitster@pobox.com>
* rj/status-bisect-while-rebase (2023-08-01) 1 commit
- status: fix branch shown when not only bisecting
"git status" is taught to show both the branch being bisected and
being rebased when both are in effect at the same time.
Needs review.
cf. <xmqqtttia3vn.fsf@gitster.g>
source: <48745298-f12b-8efb-4e48-90d2c22a8349@gmail.com>
--------------------------------------------------
[Discarded]
* tb/ci-coverity (2023-09-21) 1 commit
. .github/workflows: add coverity action
GitHub CI workflow has learned to trigger Coverity check.
Superseded by the js/ci-coverity topic.
source: <b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com>
^ permalink raw reply
* Re: [PATCH v2 0/3] support remote archive from stateless transport
From: Jiang Xin @ 2023-09-26 0:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jiang Xin
In-Reply-To: <xmqq5y3xrj19.fsf@gitster.g>
On Tue, Sep 26, 2023 at 6:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > Enable stateless-rpc connection in "connect_helper()", and add support
> > for remote archive from a stateless transport.
> > ...
>
> Administrivia. Please make sure that your patches [1..N/N] appear
> as a follow-up to the cover letter [0/N], instead of each of them
> being individually a response to somebody else's message.
>
I will set the "format.thread" configuration variable so that I don't
have to worry about forgetting the "--thread" option when executing
git-format-patch.
git config --global format.thread shallow
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Taylor Blau @ 2023-09-25 23:06 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git
In-Reply-To: <20230924195900.GA1156862@szeder.dev>
On Sun, Sep 24, 2023 at 09:59:00PM +0200, SZEDER Gábor wrote:
> Besides the issue of not reading Bloom filter for root commits, that
> message of mine also includes a test demonstrating that handling split
> commit graph with layers containing different versions of Bloom
> filters is broken. That test still fails with the current version of
> this patch series, i.e. what is currently in seen. Jonathan provided
> a patch that makes that test pass, and also noted that that test did
> pass with his original design:
>
> https://public-inbox.org/git/20230901205616.3572722-1-jonathantanmy@google.com/
I am not sure whether you are pointing out an existing breakage which
remains unfixed, or pointing out a new bug introduced by this series. I
think that it is the former, but I wrote up some of my confusion in:
https://lore.kernel.org/git/ZRIRtlbsYadg7EUx@nand.local/
> I maintain that without test cases thoroughly covering the interaction
> of different Bloom filter versions with split commit graphs this
> series should not be merged.
Agreed. We should wait for the discussion in the linked thread to settle
before merging this down.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 07/15] commit-graph: new filter ver. that fixes murmur3
From: Taylor Blau @ 2023-09-25 23:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: SZEDER Gábor, git, Junio C Hamano, Jeff King
In-Reply-To: <20230901205616.3572722-1-jonathantanmy@google.com>
On Fri, Sep 01, 2023 at 01:56:15PM -0700, Jonathan Tan wrote:
> > > My original design (up to patch 7 in this patch set) defends against
> > > this by taking the very first version detected and rejecting every
> > > other version, and Taylor's subsequent design reads every version, but
> > > annotates filters with its version. So I think we're covered.
> >
> > In the meantime I adapted the test cases from the above linked message
> > to write commit-graph layers with different Bloom filter versions, and
> > it does fail, because commits from the bottom commit-graph layer are
> > omitted from the revision walk's output. And the test case doesn't
> > even need a middle layer without modified path Bloom filters to "hide"
> > the different version in the bottom layer. Merging the layers seems
> > to work, though.
>
> For what it's worth, your test case below (with test_expect_success
> instead of test_expect_failure) passes with my original design. With the
> full patch set, it does fail, but for what it's worth, I did spot this,
> providing an incomplete solution [1] and then a complete one [2]. Your
> test case passes if I also include the following:
>
> diff --git a/bloom.c b/bloom.c
> index ff131893cd..1bafd62a4e 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -344,6 +344,10 @@ struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c)
>
> prepare_repo_settings(r);
> hash_version = r->settings.commit_graph_changed_paths_version;
> + if (hash_version == -1) {
> + struct bloom_filter_settings *s = get_bloom_filter_settings(r);
> + hash_version = (s && s->hash_version == 2) ? 2 : 1;
> + }
>
> if (!(hash_version == -1 || hash_version == filter->version))
> return NULL; /* unusable filter */
>
> [1] https://lore.kernel.org/git/20230824222051.2320003-1-jonathantanmy@google.com/
> [2] https://lore.kernel.org/git/20230829220432.558674-1-jonathantanmy@google.com/
Hmm. I am confused -- are you saying that this series breaks existing
functionality, or merely does not patch an existing breakage? I *think*
that it's the latter, since this test case fails identically on master,
but I am not sure.
If my understanding is correct, I think that patching this would involve
annotating each Bloom filter with a pointer to the bloom_filter_settings
it was written with, and then using those settings when querying it.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
From: Taylor Blau @ 2023-09-25 22:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder
In-Reply-To: <ZRGBo1TsgSYrMt01@tanuki>
On Mon, Sep 25, 2023 at 02:48:35PM +0200, Patrick Steinhardt wrote:
> > > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> > > index a4a0cb93b2..9bf13bac53 100644
> > > --- a/Documentation/rev-list-options.txt
> > > +++ b/Documentation/rev-list-options.txt
> > > @@ -151,6 +151,8 @@ endif::git-log[]
> > > --not::
> > > Reverses the meaning of the '{caret}' prefix (or lack thereof)
> > > for all following revision specifiers, up to the next `--not`.
> > > + When used on the command line before --stdin, the revisions passed
> > > + through stdin will not be affected by it.
> >
> > Hmmph. This seems to change the behavior introduced by 42cabc341c4. Am I
> > reading this correctly that tips on stdin with '--not --stdin' would not
> > be marked as UNINTERESTING?
> >
> > (Reading this back, there are a lot of double-negatives in what I just
> > wrote making it confusing for me at least. What I'm getting at here is
> > that I think `--not --stdin` should mark tips given via stdin as
> > UNINTERESTING).
>
> It does not change the behaviour, it only documents the current state
> such that it's at least spelled out somewhere.
Sorry, I must have been confused when writing this :-<. Looking at it
again, I agree that the current behavior is that "--not --stdin" treats
any tips given over stdin as INTERESTING, so this documentation is
consistent with that.
> > I wonder if we could instead do something like:
> >
> > - inherit the current set of psuedo-opts when processing input over
> > `--stdin`
> > - allow pseudo-opts within the context of `--stdin` arbitrarily
> > - prevent those psuedo-opts applied while processing `--stdin` from
> > leaking over to subsequent command-line arguments.
> >
> > Here's one approach for doing that, where we make a copy of the current
> > set of flags when calling `read_revisions_from_stdin()` instead of
> > passing a pointer to the global state.
>
> That was indeed my first approach. But this change would break behaviour
> that was introduced with 42cabc341c4 (Teach rev-list an option to read
> revs from the standard input., 2006-09-05) almost 17 years ago. If we
> change it now then this is very likely to cause problems somewhere.
Per above, you're absolutely right. I think that the patch that you
proposed here LGTM.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v7 0/9] Repack objects into separate packfiles based on a filter
From: Taylor Blau @ 2023-09-25 22:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, git, John Cai, Jonathan Tan, Jonathan Nieder,
Derrick Stolee, Patrick Steinhardt
In-Reply-To: <xmqqy1gurrpj.fsf@gitster.g>
On Mon, Sep 25, 2023 at 12:14:00PM -0700, Junio C Hamano wrote:
> Thanks, both, for working well together.
Christian made it easy to do so! ;-)
> Will replace and merge to 'seen'. Let's see others supporting the
> change to chime in, and get it merged to 'next' soonish. I gave a
> quick cursory look and changes to rebuild on the "existing packs
> cleanup" topic all looked sensible.
Sounds good. I took a look over the range-diff and the changes were as
expected. Having reviewed earlier rounds of this series in depth, I'm
comfortable merging this down whenever you are.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 0/3] support remote archive from stateless transport
From: Junio C Hamano @ 2023-09-25 22:21 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Jiang Xin
In-Reply-To: <20230923152201.14741-1-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Enable stateless-rpc connection in "connect_helper()", and add support
> for remote archive from a stateless transport.
> ...
Administrivia. Please make sure that your patches [1..N/N] appear
as a follow-up to the cover letter [0/N], instead of each of them
being individually a response to somebody else's message.
Thanks.
^ permalink raw reply
* Re: Request for Curl.exe update included in Git binaries
From: brian m. carlson @ 2023-09-25 21:54 UTC (permalink / raw)
To: Robert Smith; +Cc: git@vger.kernel.org
In-Reply-To: <IA1PR02MB9088F7E0AB55FE03CF0C38B186FCA@IA1PR02MB9088.namprd02.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 904 bytes --]
On 2023-09-25 at 15:37:46, Robert Smith wrote:
> Hello,
Hey,
> Regarding this CVE:
>
> https://curl.se/docs/CVE-2023-38039.html
>
> Is there any plan to update Git for Windows to include the updated 8.3.0 Curl binaries?
The Git project doesn't ship any binaries at all, and we don't ship
curl. Git for Windows does ship a substantial amount of other software,
including curl. You can find their issue tracker at
https://github.com/git-for-windows/git/issues, but I believe this has
already been fixed in https://github.com/git-for-windows/git/issues/4605
and will be included in the next version.
I'm not certain about their release policy, but I seem to recall that
they don't issue updates for dependent packages until a new release
would normally be done. To be certain, you'd have to inquire with them.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages
From: Junio C Hamano @ 2023-09-25 21:51 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Jonathan Tan, Jiang Xin
In-Reply-To: <20230925154144.15213-3-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()"
> to prevent mangling newline characters in sideband messages.
>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> pkt-line.c | 32 ++++++++++++++++++++++++++++++--
> pkt-line.h | 1 +
> t/t0070-fundamental.sh | 2 +-
> 3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 5943777a17..865ad19484 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> }
>
> if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> - len && buffer[len-1] == '\n')
> - len--;
> + len && buffer[len-1] == '\n') {
> + if (options & PACKET_READ_USE_SIDEBAND) {
> + int band = *buffer & 0xff;
> + switch (band) {
> + case 1:
> + /* Chomp newline for payload */
> + len--;
> + break;
> + case 2:
> + /* fallthrough */
> + case 3:
> + /*
> + * Do not chomp newline for progress and error
> + * message.
> + */
> + break;
> + default:
> + /*
> + * Bad sideband, let's leave it to
> + * demultiplex_sideband() to catch this error.
> + */
> + break;
> + }
> + } else {
> + len--;
> + }
> + }
That's a mouthful and we could shorten it a lot, but is very easy to
follow the logic ;-)
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index a927c665d6..138c2becc1 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
> test_cmp expect-err err
> '
>
> -test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
> +test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
> test_when_finished "rm -f expect-out expect-err" &&
> test-tool pkt-line send-split-sideband >split-sideband &&
> test-tool pkt-line unpack-sideband \
We cannot quite see what got fixed that is outside the postimage,
but it is nice that we have one fewer test_expect_failure in the
end.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/3] transport-helper: run do_take_over in connect_helper
From: Junio C Hamano @ 2023-09-25 21:34 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Brandon Williams, Ilari Liusvaara, Jiang Xin
In-Reply-To: <20230923152201.14741-3-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> After successfully connecting to the smart transport by calling
> "process_connect_service()" in "connect_helper()", run "do_take_over()"
> to replace the old vtable with a new one which has methods ready for
> the smart transport connection.
The existing pattern among all callers of process_connect() seems to
be
if (process_connect(...)) {
do_take_over();
... dispatch to the underlying method ...
}
... otherwise implement the fallback ...
where the return value from process_connect() is the return value of
the call it makes to process_connect_service().
And the only other caller of process_connect_service() is
connect_helper(), so in that sense, making a call to do_take_over()
when process_connect_service() succeeds in the helper does make
things consistent. The connect_helper() function being static, the
helper transport is the only transport that gets affected, but how
has it been working without having this do_take_over() step? An
obvious related question is if it has been working so far, would it
break if we have do_take_over() added here?
In any case, this makes me wonder if we should do the following
patch to help developers who may want to add new callers to
process_connect_service() by adding calls to process_connect().
transport-helper.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git c/transport-helper.c w/transport-helper.c
index 91381be622..566f7473df 100644
--- c/transport-helper.c
+++ w/transport-helper.c
@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,
struct helper_data *data = transport->data;
const char *name;
const char *exec;
+ int ret;
name = for_push ? "git-receive-pack" : "git-upload-pack";
if (for_push)
@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,
else
exec = data->transport_options.uploadpack;
- return process_connect_service(transport, name, exec);
+ ret = process_connect_service(transport, name, exec);
+ if (ret)
+ do_take_over(transport);
+ return ret;
}
static int connect_helper(struct transport *transport, const char *name,
@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,
get_helper(transport);
- if (process_connect(transport, 0)) {
- do_take_over(transport);
+ if (process_connect(transport, 0))
return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
- }
/*
* If we reach here, then the server, the client, and/or the transport
@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,
{
struct helper_data *data = transport->data;
- if (process_connect(transport, 1)) {
- do_take_over(transport);
+ if (process_connect(transport, 1))
return transport->vtable->push_refs(transport, remote_refs, flags);
- }
if (!remote_refs) {
fprintf(stderr,
@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
{
get_helper(transport);
- if (process_connect(transport, for_push)) {
- do_take_over(transport);
+ if (process_connect(transport, for_push))
return transport->vtable->get_refs_list(transport, for_push,
transport_options);
- }
return get_refs_list_using_list(transport, for_push);
}
@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)
{
get_helper(transport);
- if (process_connect(transport, 0)) {
- do_take_over(transport);
+ if (process_connect(transport, 0))
return transport->vtable->get_bundle_uri(transport);
- }
return -1;
}
^ permalink raw reply related
* Re: [PATCH v2 1/3] transport-helper: no connection restriction in connect_helper
From: Junio C Hamano @ 2023-09-25 21:11 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Brandon Williams, Ilari Liusvaara, Jiang Xin
In-Reply-To: <20230923152201.14741-2-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Remove the restriction in the "connect_helper()" function and give the
> function "process_connect_service()" the opportunity to establish a
> connection using ".connect" or ".stateless_connect" for protocol v2. So
> we can connect with a stateless-rpc and do something useful. E.g., in a
> later commit, implements remote archive for a repository over HTTP
> protocol.
OK. Given that process_connect_service() does this:
if (data->connect) {
strbuf_addf(&cmdbuf, "connect %s\n", name);
ret = run_connect(transport, &cmdbuf);
} else if (data->stateless_connect &&
(get_protocol_version_config() == protocol_v2) &&
(!strcmp("git-upload-pack", name) ||
!strcmp("git-upload-archive", name))) {
strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
ret = run_connect(transport, &cmdbuf);
if (ret)
transport->stateless_rpc = 1;
}
in the spirit of the original "safety valve", it becomes tempting to
suggest we make sure at least .connect or .stateless_connect exists
in the transport to be safe, but then we will need to keep the logic
of safety valve in connect_helper() and the actual dispatching in
process_connect_service() in sync, which is a maintenance burden.
It however makes me wonder if we should add
else
die(_("operation not supported by protocol"));
at the end of the "if/else if" cascade in process_connect_service(),
so that callers that end up following this callpath with a transport
that defines neither would be caught.
Other than that, the patch is as good as the previous round, and the
explanation is vastly easier to understand.
Thanks.
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> transport-helper.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>
> /* Get_helper so connect is inited. */
> get_helper(transport);
> - if (!data->connect)
> - die(_("operation not supported by protocol"));
>
> if (!process_connect_service(transport, name, exec))
> die(_("can't connect to subservice %s"), name);
^ permalink raw reply
* Re: [PATCH] pretty-formats.txt: fix whitespace
From: Kristoffer Haugsbakk @ 2023-09-25 20:04 UTC (permalink / raw)
To: Josh Soref via GitGitGadget, Josh Soref; +Cc: git, Junio C Hamano
In-Reply-To: <pull.950.git.1695391818917.gitgitgadget@gmail.com>
On Fri, Sep 22, 2023, at 16:10, Josh Soref via GitGitGadget wrote:
> From: Josh Soref <jsoref@gmail.com>
>
> * two spaces after periods for sentences
Where's the two-spacing convention documented? I didn't find it in
`CodingGuidelines` or `SubmittingPatches`.
Thanks
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3] format-patch: add --description-file option
From: Kristoffer Haugsbakk @ 2023-09-25 19:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Oswald Buddenhagen, Jeff King, Taylor Blau, Derrick Stolee, git
In-Reply-To: <xmqq7coet6vm.fsf@gitster.g>
On Mon, Sep 25, 2023, at 21:01, Junio C Hamano wrote:
> Thanks for a positive feedback. The changes is already in 'master'
> since the beginning of this month or so and its way to be part of
> the next release, I believe.
Yes, I tested it yesterday and it works (in conjunction with
`--cover-from-description=subject`) exactly like I want it to. :D
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v7 0/9] Repack objects into separate packfiles based on a filter
From: Junio C Hamano @ 2023-09-25 19:14 UTC (permalink / raw)
To: Christian Couder
Cc: git, John Cai, Jonathan Tan, Jonathan Nieder, Taylor Blau,
Derrick Stolee, Patrick Steinhardt
In-Reply-To: <20230925152517.803579-1-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> # Changes since version 6
>
> Thanks to Junio who reviewed or commented on versions 1, 2, 3, 4 and
> 5, and to Taylor who reviewed or commented on version 1, 3, 4, 5 and
> 6! Thanks also to Robert Coup who participated in the discussions
> related to version 2 and Peff who participated in the discussions
> related to version 4. There are only the following changes since
> version 6:
>
> - This series has been rebased on top of bcb6cae296 (The twelfth
> batch, 2023-09-22) to fix conflicts with a `builtin/repack.c`
> refactoring patch series called tb/repack-existing-packs-cleanup by
> Taylor Blau that recently graduated to 'master':
>
> https://lore.kernel.org/git/cover.1694632644.git.me@ttaylorr.com/
> https://lore.kernel.org/git/xmqqil81wqkx.fsf@gitster.g/
>
> - Patch 6/9 (repack: add `--filter=<filter-spec>` option) has been
> reworked to apply on top of the above mentioned patch series.
> Taylor even posted the fixup patch to apply to this series so that
> it works well on top of his series:
>
> https://lore.kernel.org/git/ZQNKkn0YYLUyN5Ih@nand.local/
Thanks, both, for working well together.
Will replace and merge to 'seen'. Let's see others supporting the
change to chime in, and get it merged to 'next' soonish. I gave a
quick cursory look and changes to rebuild on the "existing packs
cleanup" topic all looked sensible.
^ permalink raw reply
* [PATCH] unicode: update the width tables to Unicode 15.1
From: Beat Bolli @ 2023-09-25 19:07 UTC (permalink / raw)
To: git; +Cc: Beat Bolli
Unicode 15.1 has been announced on 2023-09-12 [0], so update the
character width tables to the new version.
[0] http://blog.unicode.org/2023/09/announcing-unicode-standard-version-151.html
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
unicode-width.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/unicode-width.h b/unicode-width.h
index e15fb0455b..be5bf8c4f2 100644
--- a/unicode-width.h
+++ b/unicode-width.h
@@ -396,14 +396,13 @@ static const struct interval double_width[] = {
{ 0x2E80, 0x2E99 },
{ 0x2E9B, 0x2EF3 },
{ 0x2F00, 0x2FD5 },
-{ 0x2FF0, 0x2FFB },
-{ 0x3000, 0x303E },
+{ 0x2FF0, 0x303E },
{ 0x3041, 0x3096 },
{ 0x3099, 0x30FF },
{ 0x3105, 0x312F },
{ 0x3131, 0x318E },
{ 0x3190, 0x31E3 },
-{ 0x31F0, 0x321E },
+{ 0x31EF, 0x321E },
{ 0x3220, 0x3247 },
{ 0x3250, 0x4DBF },
{ 0x4E00, 0xA48C },
--
2.40.1
^ permalink raw reply related
* Re: [PATCH v3 0/7] CMake(Visual C) support for js/doc-unit-tests
From: Junio C Hamano @ 2023-09-25 19:09 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Johannes Schindelin
In-Reply-To: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> The recent patch series that adds proper unit testing to Git requires a
> couple of add-on patches to make it work with the CMake build on Windows
> (Visual C). This patch series aims to provide that support.
>
> This patch series is based on js/doc-unit-tests.
>
> Changes since v2:
>
> * Thanks to Phillip Wood's prodding, I managed to avoid the "Empty
> Namespace" problem in Visual Studio's Test Explorer.
Thanks. Will replace.
> + ◢ ◈ git
> + ◢ ◈ t
> + ◢ ◈ suite
> + ◈ t0000-basic
> + ◈ t0001-init
> + ◈ t0002-gitfile
> + [...]
I somehow liked this part of the log message very much ;-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox