* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Eric Sunshine @ 2023-10-03 21:10 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003220951+0200.221551-stepnem@smrk.net>
On Tue, Oct 3, 2023 at 4:09 PM Štěpán Němec <stepnem@smrk.net> wrote:
> On Tue, 3 Oct 2023 14:30:38 -0400 Eric Sunshine wrote:
> > On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
> >> diff --git a/contrib/README b/contrib/README
> >> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
> >> I expect that things that start their life in the contrib/ area
> >> -to graduate out of contrib/ once they mature, either by becoming
> >> +graduate out of contrib/ once they mature, either by becoming
> >
> > You probably want to add a comma after "area".
>
> That would read awkward to me. How about going the other way,
>
> I expect things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature
>
> instead?
Sounds fine.
> >> @@ -579,11 +579,10 @@ This test harness library does the following things:
> >> -Here are some recommented styles when writing test case.
> >
> > Do you want to fix the spelling error while you're here or is that
> > done in a later patch?
> >
> > s/recommented/recommended/
>
> You really had me double-checking both the branch and the patch I sent
> here. :-D Unless I'm very much missing something, that line is _removed_
> by the patch (seemed redundant given the title immediately preceding
> it).
Ugh, so it is. Sorry for the noise.
> >> - - Keep test title the same line with test helper function itself.
> >> + - Keep test titles and helper function invocations on the same line.
> >
> > This would be clearer if it was switched around. Either:
> >
> > Keep the test_expect_* function call and test title on the same line.
> >
> > or, more verbosely:
> >
> > Place the test title on the same line as the test_expect_* call
> > which precedes it.
>
> I'll take your first suggestion.
I like the first suggestion better too.
^ permalink raw reply
* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Junio C Hamano @ 2023-10-03 21:10 UTC (permalink / raw)
To: Štěpán Němec; +Cc: Eric Sunshine, git
In-Reply-To: <20231003220951+0200.221551-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> Come to think of it, 'doc: fix some typos, grammar and wording issues'
> might have made sense to begin with; I don't suppose C header comments
> are off-limits to doc:.
Yup, they do count as part of developer documentation.
>>> diff --git a/contrib/README b/contrib/README
>>> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
>>> I expect that things that start their life in the contrib/ area
>>> -to graduate out of contrib/ once they mature, either by becoming
>>> +graduate out of contrib/ once they mature, either by becoming
>>
>> You probably want to add a comma after "area".
>
> That would read awkward to me. How about going the other way,
>
> I expect things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature
>
> instead?
That reads well. I do not recall whom "I" in this sentence refers
to, but if this were me talking about my wish, then yes, I expect
them to graduate once they mature, and the ">>>" quoted change to
drop "to" does not sound grammatical.
Thanks for working on improving the documentation, thanks for
reviewing, and thanks for working well together.
^ permalink raw reply
* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Junio C Hamano @ 2023-10-03 21:01 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Štěpán Němec, git
In-Reply-To: <xmqqjzs3xwm1.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Unlike `--stdout` and `jump.grepCmd`, all other things that are not
> quoted, including the one that is added by this patch (i.e.,
> "ls-files -u"), are something the end-user needs to cut and paste in
"are" -> "are NOT". Sorry for the noise.
> reaction to seeing this error message, so as long as they are
> understandable in the sentences they appear in, I think they are
> fine.
>
> If we wanted to standardize, we may start to encourage consistent
> use of quoting, but I do not think it should be part of this topic.
>
> Thanks for being careful and thoughtful.
^ permalink raw reply
* [PATCH] decorate: add color.decorate.symbols config option
From: Andy Koppe @ 2023-10-03 20:54 UTC (permalink / raw)
To: git; +Cc: derrickstolee, Andy Koppe
Add new 'color.decorate.symbols' config option for determining the color
of the prefix, suffix, separator and arrow symbols used in --decorate
output and related log format placeholders, to allow them to be colored
differently from commit hashes.
For backward compatibility, fall back to the commit hash color that can
be specified with the 'color.diff.commit' option if the new option is
not provided.
Add the setting to the color.decorate.<slot> documentation, rewording
that a bit to try to improve readability.
Amend t4207-log-decoration-colors.sh to test the option.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/color.txt | 7 ++--
commit.h | 1 +
log-tree.c | 15 ++++++---
t/t4207-log-decoration-colors.sh | 58 +++++++++++++++++---------------
4 files changed, 45 insertions(+), 36 deletions(-)
diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index 1795b2d16b..5d3612ff09 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -75,9 +75,10 @@ color.diff.<slot>::
color.decorate.<slot>::
Use customized color for 'git log --decorate' output. `<slot>` is one
- of `branch`, `remoteBranch`, `tag`, `stash` or `HEAD` for local
- branches, remote-tracking branches, tags, stash and HEAD, respectively
- and `grafted` for grafted commits.
+ of `HEAD` for the current HEAD ref, `branch` for local branches,
+ `remoteBranch` for remote-tracking branches, `tag` for tags, `stash`
+ for the top stash entry, `grafted` for grafted commits, and `symbols`
+ for the punctuation surrounding the other elements.
color.grep::
When set to `always`, always highlight matches. When `false` (or
diff --git a/commit.h b/commit.h
index 28928833c5..cefcb7c490 100644
--- a/commit.h
+++ b/commit.h
@@ -56,6 +56,7 @@ enum decoration_type {
DECORATION_REF_STASH,
DECORATION_REF_HEAD,
DECORATION_GRAFTED,
+ DECORATION_SYMBOLS,
};
void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
diff --git a/log-tree.c b/log-tree.c
index 504da6b519..a5dd4292fc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -41,6 +41,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
GIT_COLOR_BOLD_MAGENTA, /* REF_STASH */
GIT_COLOR_BOLD_CYAN, /* REF_HEAD */
GIT_COLOR_BOLD_BLUE, /* GRAFTED */
+ GIT_COLOR_NIL, /* SYMBOLS */
};
static const char *color_decorate_slots[] = {
@@ -50,6 +51,7 @@ static const char *color_decorate_slots[] = {
[DECORATION_REF_STASH] = "stash",
[DECORATION_REF_HEAD] = "HEAD",
[DECORATION_GRAFTED] = "grafted",
+ [DECORATION_SYMBOLS] = "symbols",
};
static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
@@ -312,7 +314,7 @@ void format_decorations(struct strbuf *sb,
{
const struct name_decoration *decoration;
const struct name_decoration *current_and_HEAD;
- const char *color_commit, *color_reset;
+ const char *color_symbols, *color_reset;
const char *prefix = " (";
const char *suffix = ")";
@@ -337,7 +339,10 @@ void format_decorations(struct strbuf *sb,
tag = opts->tag;
}
- color_commit = diff_get_color(use_color, DIFF_COMMIT);
+ color_symbols = decorate_get_color(use_color, DECORATION_SYMBOLS);
+ if (color_is_nil(color_symbols))
+ color_symbols = diff_get_color(use_color, DIFF_COMMIT);
+
color_reset = decorate_get_color(use_color, DECORATION_NONE);
current_and_HEAD = current_pointed_by_HEAD(decoration);
@@ -352,7 +357,7 @@ void format_decorations(struct strbuf *sb,
decorate_get_color(use_color, decoration->type);
if (*prefix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbols);
strbuf_addstr(sb, prefix);
strbuf_addstr(sb, color_reset);
}
@@ -369,7 +374,7 @@ void format_decorations(struct strbuf *sb,
if (current_and_HEAD &&
decoration->type == DECORATION_REF_HEAD) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbols);
strbuf_addstr(sb, pointer);
strbuf_addstr(sb, color_reset);
strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
@@ -382,7 +387,7 @@ void format_decorations(struct strbuf *sb,
decoration = decoration->next;
}
if (*suffix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbols);
strbuf_addstr(sb, suffix);
strbuf_addstr(sb, color_reset);
}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index 21986a866d..663ae49d34 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
git config color.decorate.tag "reverse bold yellow" &&
git config color.decorate.stash magenta &&
git config color.decorate.grafted black &&
+ git config color.decorate.symbols white &&
git config color.decorate.HEAD cyan &&
c_reset="<RESET>" &&
@@ -29,6 +30,7 @@ test_expect_success setup '
c_stash="<MAGENTA>" &&
c_HEAD="<CYAN>" &&
c_grafted="<BLACK>" &&
+ c_symbols="<WHITE>" &&
test_commit A &&
git clone . other &&
@@ -53,17 +55,17 @@ cmp_filtered_decorations () {
# to this test since it does not contain any decoration, hence --first-parent
test_expect_success 'commit decorations colored correctly' '
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_commit}, \
-${c_reset}${c_remoteBranch}other/main${c_reset}${c_commit})${c_reset} A1
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_stash}refs/stash${c_reset}${c_commit})${c_reset} On main: Changes to A.t
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbols} -> ${c_reset}${c_branch}main${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbols})${c_reset} B
+${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_symbols}, \
+${c_reset}${c_remoteBranch}other/main${c_reset}${c_symbols})${c_reset} A1
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_stash}refs/stash${c_reset}${c_symbols})${c_reset} On main: Changes to A.t
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbols})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always --all >actual &&
@@ -78,14 +80,14 @@ test_expect_success 'test coloring with replace-objects' '
git replace HEAD~1 HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbols} -> ${c_reset}${c_branch}main${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbols})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_symbols}, \
+${c_reset}${c_grafted}replaced${c_reset}${c_symbols})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbols})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
@@ -104,15 +106,15 @@ test_expect_success 'test coloring with grafted commit' '
git replace --graft HEAD HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbols} -> ${c_reset}${c_branch}main${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbols}, \
+${c_reset}${c_grafted}replaced${c_reset}${c_symbols})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbols})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbols})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
--
2.42.GIT
^ permalink raw reply related
* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Junio C Hamano @ 2023-10-03 20:46 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Štěpán Němec, git
In-Reply-To: <CAPig+cQHAK2_LEG0-6MTF52t2D8b_SRxzB51B_4N35ujtGeb6Q@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
>> There's even an example of such usage in the README.
>>
>> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
>> ---
>> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
>> @@ -9,7 +9,7 @@ The <mode> parameter is one of:
>>
>> diff: elements are diff hunks. Arguments are given to diff.
>>
>> -merge: elements are merge conflicts. Arguments are ignored.
>> +merge: elements are merge conflicts. Arguments are given to ls-files -u.
>
> Should "ls-files -u" be formatted with backticks?
>
> Arguments are passed to `git ls-files -u`.
Probably not in this case, as this is end-user visible help message
that is not formatted but given to the terminal.
The whole preimage looks like this:
usage: git jump [--stdout] <mode> [<args>]
Jump to interesting elements in an editor.
The <mode> parameter is one of:
diff: elements are diff hunks. Arguments are given to diff.
merge: elements are merge conflicts. Arguments are ignored.
grep: elements are grep hits. Arguments are given to git grep or, if
configured, to the command in `jump.grepCmd`.
ws: elements are whitespace errors. Arguments are given to diff --check.
If the optional argument `--stdout` is given, print the quickfix
lines to standard output instead of feeding it to the editor.
and it is already a mixture. "given to `git grep`" is not quoted,
neither is "given to `diff --check`" or "given to `diff`"
I think rule for help/usage messages should be that
- anything that the end-user may want to cut&paste should be left
alone to make it easier to cut,
- but at the same time the message should make it clear which part
of a sentence is what,
Clicking on `--stdout` or `jump.grepCmd` does not include the
surrounding quotes, at least in my environment, so the use of
backquotes in these places satisfy the two goals, it seems.
Unlike `--stdout` and `jump.grepCmd`, all other things that are not
quoted, including the one that is added by this patch (i.e.,
"ls-files -u"), are something the end-user needs to cut and paste in
reaction to seeing this error message, so as long as they are
understandable in the sentences they appear in, I think they are
fine.
If we wanted to standardize, we may start to encourage consistent
use of quoting, but I do not think it should be part of this topic.
Thanks for being careful and thoughtful.
^ permalink raw reply
* Re: batch-command wishlist [was: [TOPIC 02/12] Libification Goals and Progress]
From: Jeff King @ 2023-10-03 20:32 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Junio C Hamano, Taylor Blau
In-Reply-To: <20231003203124.M560967@dcvr>
On Tue, Oct 03, 2023 at 08:31:24PM +0000, Eric Wong wrote:
> Jeff King <peff@peff.net> wrote:
> > I know that you asked for a persistent process, but just for reference,
> > you can hackily access approxidate with:
> >
> > git config --type=expiry-date --default='15 days ago' does.not.exist
>
> --type is too new (trying to support 1.8.x :<). But yeah, using
> `git rev-parse --since=15.days.ago` and extracting the integers
Ah, that is probably less hacky than my suggestion anyway.
-Peff
^ permalink raw reply
* [PATCH 10/10] commit-graph: clear oidset after finishing write
From: Jeff King @ 2023-10-03 20:31 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
In graph_write() we store commits in an oidset, but never clean it up,
leaking the contents. We should clear it in the cleanup section.
The oidset comes from 6830c36077 (commit-graph.h: replace 'commit_hex'
with 'commits', 2020-04-13), but it was just replacing a string_list
that was also leaked. Curiously, we fixed the leak of some adjacent
variables in commit fa8953cb40 (builtin/commit-graph.c: extract
'read_one_commit()', 2020-05-18), but the oidset wasn't included for
some reason.
In combination with the preceding commits, this lets us mark t5324 as
leak-free.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit-graph.c | 1 +
t/t5324-split-commit-graph.sh | 2 ++
2 files changed, 3 insertions(+)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c88389df24..c527a8369e 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix)
FREE_AND_NULL(options);
string_list_clear(&pack_indexes, 0);
strbuf_release(&buf);
+ oidset_clear(&commits);
return result;
}
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 36c4141e67..52e8a3e619 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='split commit graph'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
GIT_TEST_COMMIT_GRAPH=0
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* Re: batch-command wishlist [was: [TOPIC 02/12] Libification Goals and Progress]
From: Eric Wong @ 2023-10-03 20:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Taylor Blau
In-Reply-To: <20231003201048.GD1562@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> I know that you asked for a persistent process, but just for reference,
> you can hackily access approxidate with:
>
> git config --type=expiry-date --default='15 days ago' does.not.exist
--type is too new (trying to support 1.8.x :<). But yeah, using
`git rev-parse --since=15.days.ago` and extracting the integers
^ permalink raw reply
* [PATCH 09/10] commit-graph: free write-context base_graph_name during cleanup
From: Jeff King @ 2023-10-03 20:31 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
Commit 6c622f9f0b (commit-graph: write commit-graph chains, 2019-06-18)
added a base_graph_name string to the write_commit_graph_context struct.
But the end-of-function cleanup forgot to free it, causing a leak.
This (presumably in combination with the preceding leak-fixes) lets us
mark t5328 as leak-free.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 1 +
t/t5328-commit-graph-64bit-time.sh | 2 ++
2 files changed, 3 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 744b7eb1a3..e4d09da090 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2518,6 +2518,7 @@ int write_commit_graph(struct object_directory *odb,
cleanup:
free(ctx->graph_name);
+ free(ctx->base_graph_name);
free(ctx->commits.list);
oid_array_clear(&ctx->oids);
clear_topo_level_slab(&topo_levels);
diff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh
index e9c521c061..ca476e80a0 100755
--- a/t/t5328-commit-graph-64bit-time.sh
+++ b/t/t5328-commit-graph-64bit-time.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='commit graph with 64-bit timestamps'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
if ! test_have_prereq TIME_IS_64BIT || ! test_have_prereq TIME_T_IS_64BIT
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 08/10] commit-graph: free write-context entries before overwriting
From: Jeff King @ 2023-10-03 20:30 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When writing a split graph file, we replace the final element of the
commit_graph_hash_after and commit_graph_filenames_after arrays. But
since these are allocated strings, we need to free them before
overwriting to avoid leaking the old string.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 4aa2f294f1..744b7eb1a3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2065,9 +2065,11 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
free(graph_name);
}
+ free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash));
final_graph_name = get_split_graph_filename(ctx->odb,
ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
+ free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]);
ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
result = rename(ctx->graph_name, final_graph_name);
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 07/10] commit-graph: free graph struct that was not added to chain
From: Jeff King @ 2023-10-03 20:30 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When reading the graph chain file, we open (and allocate) each
individual slice it mentions and then add them to a linked-list chain.
But if adding to the chain fails (e.g., because the base-graph chunk it
contains didn't match what we expected), we leave the function without
freeing the graph struct that caused the failure, leaking it.
We can fix it by calling free_graph_commit().
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 2c72a554c2..4aa2f294f1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -566,6 +566,8 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
if (add_graph_to_chain(g, graph_chain, oids, i)) {
graph_chain = g;
valid = 1;
+ } else {
+ free_commit_graph(g);
}
break;
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 06/10] commit-graph: delay base_graph assignment in add_graph_to_chain()
From: Jeff King @ 2023-10-03 20:30 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When adding a graph to a chain, we do some consistency checks and then
if everything looks good, set g->base_graph to add a link to the chain.
But when we added a new consistency check in 209250ef38 (commit-graph.c:
prevent overflow in add_graph_to_chain(), 2023-07-12), it comes _after_
we've already set g->base_graph. So we might return failure, even though
we actually added to the chain.
This hasn't caused a bug yet, because after failing to add to the chain,
we discard the failed graph struct completely, leaking it. But in order
to fix that, it's important that the struct be in a consistent and
predictable state after the failure.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 2f75ecd9ae..2c72a554c2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -498,8 +498,6 @@ static int add_graph_to_chain(struct commit_graph *g,
cur_g = cur_g->base_graph;
}
- g->base_graph = chain;
-
if (chain) {
if (unsigned_add_overflows(chain->num_commits,
chain->num_commits_in_base)) {
@@ -510,6 +508,8 @@ static int add_graph_to_chain(struct commit_graph *g,
g->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;
}
+ g->base_graph = chain;
+
return 1;
}
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 05/10] commit-graph: free all elements of graph chain
From: Jeff King @ 2023-10-03 20:29 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When running "commit-graph verify", we call free_commit_graph(). That's
sufficient for the case of a single graph file, but if we loaded a chain
of split graph files, they form a linked list via the base_graph
pointers. We need to free all of them, or we leak all but the first
struct.
We can make this work by teaching free_commit_graph() to walk the
base_graph pointers and free each element. This in turn lets us simplify
close_commit_graph(), which does the same thing by recursion (we cannot
just use close_commit_graph() in "commit-graph verify", as the function
takes a pointer to an object store, and the verify command creates a
single one-off graph struct).
While indenting the code in free_commit_graph() for the loop, I noticed
that setting g->data to NULL is rather pointless, as we free the struct
a few lines later. So I cleaned that up while we're here.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index dc54ef4776..2f75ecd9ae 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -723,19 +723,10 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r)
return NULL;
}
-static void close_commit_graph_one(struct commit_graph *g)
-{
- if (!g)
- return;
-
- close_commit_graph_one(g->base_graph);
- free_commit_graph(g);
-}
-
void close_commit_graph(struct raw_object_store *o)
{
clear_commit_graph_data_slab(&commit_graph_data_slab);
- close_commit_graph_one(o->commit_graph);
+ free_commit_graph(o->commit_graph);
o->commit_graph = NULL;
}
@@ -2753,15 +2744,17 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
void free_commit_graph(struct commit_graph *g)
{
- if (!g)
- return;
- if (g->data) {
- munmap((void *)g->data, g->data_len);
- g->data = NULL;
+ while (g) {
+ struct commit_graph *next = g->base_graph;
+
+ if (g->data)
+ munmap((void *)g->data, g->data_len);
+ free(g->filename);
+ free(g->bloom_filter_settings);
+ free(g);
+
+ g = next;
}
- free(g->filename);
- free(g->bloom_filter_settings);
- free(g);
}
void disable_commit_graph(struct repository *r)
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 04/10] commit-graph: move slab-clearing to close_commit_graph()
From: Jeff King @ 2023-10-03 20:27 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
When closing and freeing a commit-graph, the main entry point is
close_commit_graph(), which then uses close_commit_graph_one() to
recurse through the base_graph links and free each one.
Commit 957ba814bf (commit-graph: when closing the graph, also release
the slab, 2021-09-08) put the call to clear the slab into the recursive
function, but this is pointless: there's only a single global slab
variable. It works OK in practice because clearing the slab is
idempotent, but it makes the code harder to reason about and refactor.
Move it into the parent function so it's only called once (and there are
no other direct callers of the recursive close_commit_graph_one(), so we
are not hurting them).
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/commit-graph.c b/commit-graph.c
index 5e8a3a5085..dc54ef4776 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -728,13 +728,13 @@ static void close_commit_graph_one(struct commit_graph *g)
if (!g)
return;
- clear_commit_graph_data_slab(&commit_graph_data_slab);
close_commit_graph_one(g->base_graph);
free_commit_graph(g);
}
void close_commit_graph(struct raw_object_store *o)
{
+ clear_commit_graph_data_slab(&commit_graph_data_slab);
close_commit_graph_one(o->commit_graph);
o->commit_graph = NULL;
}
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 03/10] merge: free result of repo_get_merge_bases()
From: Jeff King @ 2023-10-03 20:27 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
We call repo_get_merge_bases(), which allocates a commit_list, but never
free the result, causing a leak.
The obvious solution is to free it, but we need to look at the contents
of the first item to decide whether to leave the loop. One option is to
free it in both code paths. But since the commit that the list points to
is longer-lived than the list itself, we can just dereference it
immediately, free the list, and then continue with the existing logic.
This is about the same amount of code, but keeps the list management all
in one place.
This lets us mark a number of merge-related test scripts as leak-free.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/merge.c | 5 ++++-
t/t4214-log-graph-octopus.sh | 1 +
t/t4215-log-skewed-merges.sh | 1 +
t/t6009-rev-list-parent.sh | 1 +
t/t6416-recursive-corner-cases.sh | 1 +
t/t6433-merge-toplevel.sh | 1 +
t/t6437-submodule-merge.sh | 1 +
t/t7602-merge-octopus-many.sh | 1 +
t/t7603-merge-reduce-heads.sh | 1 +
t/t7607-merge-state.sh | 1 +
t/t7608-merge-messages.sh | 1 +
11 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index fd21c0d4f4..ac4816c14e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1634,6 +1634,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
for (j = remoteheads; j; j = j->next) {
struct commit_list *common_one;
+ struct commit *common_item;
/*
* Here we *have* to calculate the individual
@@ -1643,7 +1644,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
common_one = repo_get_merge_bases(the_repository,
head_commit,
j->item);
- if (!oideq(&common_one->item->object.oid, &j->item->object.oid)) {
+ common_item = common_one->item;
+ free_commit_list(common_one);
+ if (!oideq(&common_item->object.oid, &j->item->object.oid)) {
up_to_date = 0;
break;
}
diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
index f70c46bbbf..7905597869 100755
--- a/t/t4214-log-graph-octopus.sh
+++ b/t/t4214-log-graph-octopus.sh
@@ -5,6 +5,7 @@ test_description='git log --graph of skewed left octopus merge.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-log-graph.sh
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index 28d0779a8c..b877ac7235 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -2,6 +2,7 @@
test_description='git log --graph of skewed merges'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-log-graph.sh
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 5a67bbc760..ced40157ed 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -5,6 +5,7 @@ test_description='ancestor culling and limiting by parent number'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
check_revlist () {
diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh
index 17b54d625d..5f414abc89 100755
--- a/t/t6416-recursive-corner-cases.sh
+++ b/t/t6416-recursive-corner-cases.sh
@@ -5,6 +5,7 @@ test_description='recursive merge corner cases involving criss-cross merges'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-merge.sh
diff --git a/t/t6433-merge-toplevel.sh b/t/t6433-merge-toplevel.sh
index b16031465f..2b42f095dc 100755
--- a/t/t6433-merge-toplevel.sh
+++ b/t/t6433-merge-toplevel.sh
@@ -5,6 +5,7 @@ test_description='"git merge" top-level frontend'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
t3033_reset () {
diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh
index c9a86f2e94..daa507862c 100755
--- a/t/t6437-submodule-merge.sh
+++ b/t/t6437-submodule-merge.sh
@@ -8,6 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1
export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-merge.sh
diff --git a/t/t7602-merge-octopus-many.sh b/t/t7602-merge-octopus-many.sh
index ff085b086c..3669d33bd5 100755
--- a/t/t7602-merge-octopus-many.sh
+++ b/t/t7602-merge-octopus-many.sh
@@ -4,6 +4,7 @@ test_description='git merge
Testing octopus merge with more than 25 refs.'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 4887ca705b..0e85b21ec8 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -4,6 +4,7 @@ test_description='git merge
Testing octopus merge when reducing parents to independent branches.'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# 0 - 1
diff --git a/t/t7607-merge-state.sh b/t/t7607-merge-state.sh
index 89a62ac53b..9001674f2e 100755
--- a/t/t7607-merge-state.sh
+++ b/t/t7607-merge-state.sh
@@ -4,6 +4,7 @@ test_description="Test that merge state is as expected after failed merge"
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'Ensure we restore original state if no merge strategy handles it' '
diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh
index 0b908ab2e7..2179938c43 100755
--- a/t/t7608-merge-messages.sh
+++ b/t/t7608-merge-messages.sh
@@ -4,6 +4,7 @@ test_description='test auto-generated merge messages'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
check_oneline() {
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 02/10] commit-reach: free temporary list in get_octopus_merge_bases()
From: Jeff King @ 2023-10-03 20:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
We loop over the set of commits to merge, and for each one compute the
merge base against the existing set of merge base candidates we've
found. Then we replace the candidate set with a simple assignment of the
list head, leaking the old list. We should free it first before
assignment.
This makes t5521 leak-free, so mark it as such.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-reach.c | 1 +
t/t5521-pull-options.sh | 1 +
2 files changed, 2 insertions(+)
diff --git a/commit-reach.c b/commit-reach.c
index 4b7c233fd4..a868a575ea 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -173,6 +173,7 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)
for (k = bases; k; k = k->next)
end = k;
}
+ free_commit_list(ret);
ret = new_commits;
}
return ret;
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 264de29c35..079b2f2536 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -5,6 +5,7 @@ test_description='pull options'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* [PATCH 01/10] t6700: mark test as leak-free
From: Jeff King @ 2023-10-03 20:26 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
This test has never leaked since it was added. Let's annotate it to make
sure it stays that way (and to reduce noise when looking for other
leak-free scripts after we fix some leaks).
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously not directly related to the rest; this could be spun off to
its own series, or put atop jk/tree-name-and-depth-limit and merged from
there.
t/t6700-tree-depth.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t6700-tree-depth.sh b/t/t6700-tree-depth.sh
index e410c41234..9e70a7c763 100755
--- a/t/t6700-tree-depth.sh
+++ b/t/t6700-tree-depth.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='handling of deep trees in various commands'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# We'll test against two depths here: a small one that will let us check the
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* Re: [PATCH v2] git-status.txt: fix minor asciidoc format issue
From: Junio C Hamano @ 2023-10-03 20:25 UTC (permalink / raw)
To: cousteau via GitGitGadget; +Cc: git, Javier Mora
In-Reply-To: <pull.1591.v2.git.1696350802820.gitgitgadget@gmail.com>
"cousteau via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Javier Mora <cousteaulecommandant@gmail.com>
>
> The paragraph below the list of short option combinations
> isn't correctly formatted, making the result hard to read.
>
> Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
> ---
The above probably no longer describes the situation the patch
intends to correct, I suspect. It used to be near-impossible hard
to read, but at least with them indented they are legible.
The additional states for submodules are typeset differently
from how the states for paths for normal blobs are listed as
enumeration. Format them in the same way for consistency.
or something like that, perhaps.
> Documentation/git-status.txt | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index b27d127b5e2..48f46eb2047 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -246,10 +246,9 @@ U U unmerged, both modified
>
> Submodules have more state and instead report
>
> - M the submodule has a different HEAD than
> - recorded in the index
> - m the submodule has modified content
> - ? the submodule has untracked files
> +* 'M' = the submodule has a different HEAD than recorded in the index
> +* 'm' = the submodule has modified content
> +* '?' = the submodule has untracked files
>
> since modified content or untracked files in a submodule cannot be added
> via `git add` in the superproject to prepare a commit.
Thanks for making this part of the documentation better.
^ permalink raw reply
* [PATCH 0/10] some commit-graph leak fixes
From: Jeff King @ 2023-10-03 20:25 UTC (permalink / raw)
To: git
I noticed while working on the jk/commit-graph-verify-fix topic that
free_commit_graph() leaks any slices of a commit-graph-chain except for
the first. I naively hoped that fixing that would make t5324 leak-free,
but it turns out there were a number of other leaks, so I fixed those,
too. A couple of them were in the merge code, which in turn means a
bunch of new test scripts are now leak-free.
Even though I saw the problem on that other topic, there's no dependency
here; this series can be applied directly to master (or possibly even
maint, though I didn't try).
[01/10]: t6700: mark test as leak-free
[02/10]: commit-reach: free temporary list in get_octopus_merge_bases()
[03/10]: merge: free result of repo_get_merge_bases()
[04/10]: commit-graph: move slab-clearing to close_commit_graph()
[05/10]: commit-graph: free all elements of graph chain
[06/10]: commit-graph: delay base_graph assignment in add_graph_to_chain()
[07/10]: commit-graph: free graph struct that was not added to chain
[08/10]: commit-graph: free write-context entries before overwriting
[09/10]: commit-graph: free write-context base_graph_name during cleanup
[10/10]: commit-graph: clear oidset after finishing write
builtin/commit-graph.c | 1 +
builtin/merge.c | 5 +++-
commit-graph.c | 40 ++++++++++++++----------------
commit-reach.c | 1 +
t/t4214-log-graph-octopus.sh | 1 +
t/t4215-log-skewed-merges.sh | 1 +
t/t5324-split-commit-graph.sh | 2 ++
t/t5328-commit-graph-64bit-time.sh | 2 ++
t/t5521-pull-options.sh | 1 +
t/t6009-rev-list-parent.sh | 1 +
t/t6416-recursive-corner-cases.sh | 1 +
t/t6433-merge-toplevel.sh | 1 +
t/t6437-submodule-merge.sh | 1 +
t/t6700-tree-depth.sh | 2 ++
t/t7602-merge-octopus-many.sh | 1 +
t/t7603-merge-reduce-heads.sh | 1 +
t/t7607-merge-state.sh | 1 +
t/t7608-merge-messages.sh | 1 +
18 files changed, 42 insertions(+), 22 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Junio C Hamano @ 2023-10-03 20:20 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <ZRxBlrjyuBmJnx3p@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
>> I've had this sitting in my patch queue for a while now. It's a
>> non-critical performance fix that avoids the repack/MIDX machinery from
>> ever choosing a cruft pack as preferred when writing a MIDX bitmap
>> without a given --preferred-pack.
>>
>> There is no correctness issue here, but choosing a pack with few/no
>> reachable objects means that our pack reuse mechanism will rarely kick
>> in, resulting in performance degradation.
>>
>> builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
>> t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 85 insertions(+), 1 deletion(-)
>
> Oops, I should have mentioned that this is meant to be applied on top of
> 'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry
> about that.
Sorry, but I do not follow. tb/multi-cruft-pack was merged to
'master' at c0b5d46d (Documentation/gitformat-pack.txt: drop mixed
version section, 2023-08-28) but back then t7704 did not exist. Do
you mean the other topic in-flight from you about max-cruft-size?
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Jeff King @ 2023-10-03 20:16 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com>
On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
> Note that this behavior is usually just a performance regression. But
> it's possible it could be a correctness issue.
>
> Suppose an object was duplicated among the cruft and non-cruft pack. The
> MIDX will pick the one from the pack with the lowest mtime, which will
> always be the cruft one. But if the non-cruft pack happens to sort
> earlier in lexical order, we'll treat that one as preferred, but not all
> duplicates will be resolved in favor of that pack.
>
> So if we happened to have an object which appears in both packs
> (e.g., due to a cruft object being freshened, causing it to appear
> loose, and then repacking it via the `--geometric` repack) it's possible
> the duplicate would be picked from the non-preferred pack.
I'm not sure I understand how that is a correctness issue. The contents
of the object are the same in either pack. Or do you mean that the
pack-reuse code in pack-objects.c may get confused and try to use the
wrong pack/offset when sending the object out? I would think it would
always be coming from the preferred pack for that code (so the outcome
is just that we fail to do the pack-reuse optimization very well, but we
don't generate a wrong answer).
Other than that, the explanation and patch make perfect sense to me, and
the patch looks good.
-Peff
^ permalink raw reply
* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Štěpán Němec @ 2023-10-03 20:15 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cQHAK2_LEG0-6MTF52t2D8b_SRxzB51B_4N35ujtGeb6Q@mail.gmail.com>
On Tue, 3 Oct 2023 14:33:39 -0400
Eric Sunshine wrote:
> On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
>> There's even an example of such usage in the README.
>>
>> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
>> ---
>> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
>> @@ -9,7 +9,7 @@ The <mode> parameter is one of:
>>
>> diff: elements are diff hunks. Arguments are given to diff.
>>
>> -merge: elements are merge conflicts. Arguments are ignored.
>> +merge: elements are merge conflicts. Arguments are given to ls-files -u.
>
> Should "ls-files -u" be formatted with backticks?
>
> Arguments are passed to `git ls-files -u`.
My preference is against this (I don't like markup in plain-text files),
although, unlike the contrib README case in 1/5, here there _are_ some
pre-existing backquotes; but the usage is inconsistent. If we want to
unify one way or the other, I'd leave that for another patch, in any
case.
Thanks,
Štěpán
^ permalink raw reply
* Re: batch-command wishlist [was: [TOPIC 02/12] Libification Goals and Progress]
From: Jeff King @ 2023-10-03 20:10 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Junio C Hamano, Taylor Blau
In-Reply-To: <20231003005251.M353509@dcvr>
On Tue, Oct 03, 2023 at 12:52:51AM +0000, Eric Wong wrote:
> Not sure if cat-file is the place for it, but a persistent
> process to deal with:
>
> * `git config -f FILENAME ...' (especially --get-urlmatch --type=FOO)
> * approxidate parsing for other tools[2]
>
> [2] I did recently license the code of a standalone C++ executable
> as GPL-2+ so I can copy approxidate parsing from git and
> perhaps figure out enough C++ to use Xapian query parser
> bindings instead of the hacky `git rev-parse --since=' thing
> I do.
I know that you asked for a persistent process, but just for reference,
you can hackily access approxidate with:
git config --type=expiry-date --default='15 days ago' does.not.exist
-Peff
^ permalink raw reply
* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Štěpán Němec @ 2023-10-03 20:09 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cR2wgAsGCTphqHsf_pbM0q_xLcMx=acVD6MDKhpEt3HNA@mail.gmail.com>
On Tue, 3 Oct 2023 14:30:38 -0400
Eric Sunshine wrote:
> On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
>> Fix some typos, grammar or wording issues in the documentation
>
> SubmittingPatches suggests: s/Fix/fix
I think that only applies to subjects of the form "area: description".
Come to think of it, 'doc: fix some typos, grammar and wording issues'
might have made sense to begin with; I don't suppose C header comments
are off-limits to doc:.
>> diff --git a/Documentation/git.txt b/Documentation/git.txt
>> @@ -96,7 +96,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config
>> This is useful for cases where you want to pass transitory
>> -configuration options to git, but are doing so on OS's where
>> +configuration options to git, but are doing so on OSes where
>
> Since you're touching this anyhow, it could probably be made clearer
> by simply spelling it out: "operating systems"
Probably. Checking for other occurrences now I see I haven't done my
homework, anyway: there are "OS's" as plural in config/transfer.txt and
fsmonitor--daemon.h, too. I'll include those and use "operating
systems" in the next version, thanks.
>> other processes might be able to read your cmdline
>> (e.g. `/proc/self/cmdline`), but not your environ
>> (e.g. `/proc/self/environ`). That behavior is the default on
>
> I wonder if "cmdline" and "environ" would be better spelled out, as
> well, since the examples which immediately follow them give the
> necessary context.
Agreed.
> other processes might be able to read your command-line
> (e.g. `/proc/self/cmdline`), but not your environment
> (e.g. `/proc/self/environ`).
>
> But perhaps that's outside the scope of this patch?
I don't think so; I'll include those.
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> @@ -1151,7 +1151,7 @@ will be stored via placeholder `%P`.
>> This attribute controls the length of conflict markers left in
>> -the work tree file during a conflicted merge. Only setting to
>> +the work tree file during a conflicted merge. Only setting
>> the value to a positive integer has any meaningful effect.
>
> Or, more simply:
>
> Only a positive integer has a meaningful effect.
That's better, thanks.
>> diff --git a/contrib/README b/contrib/README
>> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
>> I expect that things that start their life in the contrib/ area
>> -to graduate out of contrib/ once they mature, either by becoming
>> +graduate out of contrib/ once they mature, either by becoming
>
> You probably want to add a comma after "area".
That would read awkward to me. How about going the other way,
I expect things that start their life in the contrib/ area
to graduate out of contrib/ once they mature
instead?
> Do we want to correct the formatting of this pathname:
>
> ...in the `contrib/` area...
> ...out of `contrib/` once...
>
> or is that outside the scope of this patch?
I'd prefer not to: this is a plain-text README, no other markup present,
and I don't think the backquotes would help here.
>> diff --git a/strbuf.h b/strbuf.h
>> @@ -12,9 +12,9 @@
>> - * strbuf's are meant to be used with all the usual C string and memory
>> + * strbufs are meant to be used with all the usual C string and memory
>
> Alternatively:
>
> strbuf is meant to be used...
>
>> * APIs. Given that the length of the buffer is known, it's often better to
>> - * use the mem* functions than a str* one (memchr vs. strchr e.g.).
>> + * use the mem* functions than a str* one (e.g., memchr vs. strchr).
>> * Though, one has to be careful about the fact that str* functions often
>> * stop on NULs and that strbufs may have embedded NULs.
>
> Similar:
>
> ... and that a strbuf may have...
Hm, not sure about this one. I think I'd prefer keeping the plurals,
but it's not a strong preference.
>> diff --git a/t/README b/t/README
>> @@ -262,8 +262,8 @@ The argument for --run, <test-selector>, is a list of description
>> suite to include (or exclude, if negated) in the run. A range is two
>> -numbers separated with a dash and matches a range of tests with both
>> -ends been included. You may omit the first or the second number to
>> +numbers separated with a dash and matches an inclusive range of tests
>> +to run. You may omit the first or the second number to
>
> Not the fault of this patch, but "matches" seems an odd choice.
> Perhaps "specifies" would feel more natural.
Indeed, will fix.
>> The argument to --run is split on commas into separate strings,
>> @@ -579,11 +579,10 @@ This test harness library does the following things:
>> -Here are some recommented styles when writing test case.
>
> Do you want to fix the spelling error while you're here or is that
> done in a later patch?
>
> s/recommented/recommended/
You really had me double-checking both the branch and the patch I sent
here. :-D Unless I'm very much missing something, that line is _removed_
by the patch (seemed redundant given the title immediately preceding
it).
>> - - Keep test title the same line with test helper function itself.
>> + - Keep test titles and helper function invocations on the same line.
>
> This would be clearer if it was switched around. Either:
>
> Keep the test_expect_* function call and test title on the same line.
>
> or, more verbosely:
>
> Place the test title on the same line as the test_expect_* call
> which precedes it.
I'll take your first suggestion.
Thank you for the review!
--
Štěpán
^ permalink raw reply
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Jeff King @ 2023-10-03 20:06 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git, avarab
In-Reply-To: <20231003082513.3003520-1-stepnem@smrk.net>
On Tue, Oct 03, 2023 at 10:25:13AM +0200, Štěpán Němec wrote:
> The DESCRIPTION's "first form" is actually the 1st, 2nd, 3rd and 5th
> form in SYNOPSIS, the "second form" is the 4th one.
>
> Interestingly, this state of affairs was introduced in
> 97fe7250753b (cat-file docs: fix SYNOPSIS and "-h" output, 2021-12-28)
> with the claim of "Now the two will match again." ("the two" being
> DESCRIPTION and SYNOPSIS)...
>
> Ordinals are hard, let's try talking about batch and non-batch mode
> instead.
Thanks, I think this is a good direction. Two things I noticed:
> DESCRIPTION
> -----------
> -In its first form, the command provides the content or the type of an object in
> +This command can operate in two modes, depending on whether an option from
> +the `--batch` family is specified.
> +
> +In non-batch mode, the command provides the content or the type of an object in
> the repository. The type is required unless `-t` or `-p` is used to find the
> object type, or `-s` is used to find the object size, or `--textconv` or
> `--filters` is used (which imply type "blob").
The existing text here is already a bit vague, considering the number of
operations it covers (like "-e", for example, which is not showing "the
content or the type" at all). But that is not new in your patch, and it
is maybe even OK to be a bit vague here, and let the OPTIONS section
cover the specifics.
> -In the second form, a list of objects (separated by linefeeds) is provided on
> +In batch mode, a list of objects (separated by linefeeds) is provided on
> stdin, and the SHA-1, type, and size of each object is printed on stdout. The
> output format can be overridden using the optional `<format>` argument. If
> either `--textconv` or `--filters` was specified, the input is expected to
I think this got a bit inaccurate with "--batch-command", which is a
whole different mode itself compared to --batch and --batch-check. I
don't think your patch is really making anything worse, but arguably
there are three "major modes" here.
-Peff
^ permalink raw reply
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