* [PATCH 11/15] find multi-byte comment chars in unterminated buffers
From: Jeff King @ 2024-03-07 9:26 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Dragan Simic,
Kristoffer Haugsbakk, Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
As with the previous patch, we need to swap out single-byte matching for
something like starts_with() to match all bytes of a multi-byte comment
character. But for cases where the buffer is not NUL-terminated (and we
instead have an explicit size or end pointer), it's not safe to use
starts_with(), as it might walk off the end of the buffer.
Let's introduce a new starts_with_mem() that does the same thing but
also accepts the length of the "haystack" str and makes sure not to walk
past it.
Note that in most cases the existing code did not need a length check at
all, since it was written in a way that knew we had at least one byte
available (and that was all we checked). So I had to read each one to
find the appropriate bounds. The one exception is sequencer.c's
add_commented_lines(), where we can actually get rid of the length
check. Just like starts_with(), our starts_with_mem() handles an empty
haystack variable by not matching (assuming a non-empty prefix).
A few notes on the implementation of starts_with_mem():
- it would be equally correct to take an "end" pointer (and indeed,
many of the callers have this and have to subtract to come up with
the length). I think taking a ptr/size combo is a more usual
interface for our codebase, though, and has the added benefit that
the function signature makes it harder to mix up the three
parameters.
- we could obviously build starts_with() on top of this by passing
strlen(str) as the length. But it's possible that starts_with() is a
relatively hot code path, and it should not pay that penalty (it can
generally return an answer proportional to the size of the prefix,
not the whole string).
- it naively feels like xstrncmpz() should be able to do the same
thing, but that's not quite true. If you pass the length of the
haystack buffer, then strncmp() finds that a shorter prefix string
is "less than" than the haystack, even if the haystack starts with
the prefix. If you pass the length of the prefix, then you risk
reading past the end of the haystack if it is shorter than the
prefix. So I think we really do need a new function.
Signed-off-by: Jeff King <peff@peff.net>
---
Arguably starts_with() and this new function should both be inlined,
like we do for skip_prefix(), but I think that's out of scope for this
series.
And it's possible I was simply too dumb to figure out xstrncmpz() here.
I'm waiting for René to show up and tell me how to do it. ;)
IMHO this is the trickiest commit of the whole series, as it would be
easy to get the length computations subtly wrong.
commit.c | 3 ++-
sequencer.c | 4 ++--
strbuf.c | 11 +++++++++++
strbuf.h | 1 +
trailer.c | 4 ++--
5 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/commit.c b/commit.c
index ef679a0b93..531a666cba 100644
--- a/commit.c
+++ b/commit.c
@@ -1796,7 +1796,8 @@ size_t ignored_log_message_bytes(const char *buf, size_t len)
else
next_line++;
- if (buf[bol] == comment_line_char || buf[bol] == '\n') {
+ if (starts_with_mem(buf + bol, cutoff - bol, comment_line_str) ||
+ buf[bol] == '\n') {
/* is this the first of the run of comments? */
if (!boc)
boc = bol;
diff --git a/sequencer.c b/sequencer.c
index 991a2dbe96..664986e3b2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1840,7 +1840,7 @@ static int is_fixup_flag(enum todo_command command, unsigned flag)
static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
{
const char *s = str;
- while (len > 0 && s[0] == comment_line_char) {
+ while (starts_with_mem(s, len, comment_line_str)) {
size_t count;
const char *n = memchr(s, '\n', len);
if (!n)
@@ -2562,7 +2562,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
/* left-trim */
bol += strspn(bol, " \t");
- if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
+ if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
item->command = TODO_COMMENT;
item->commit = NULL;
item->arg_offset = bol - buf;
diff --git a/strbuf.c b/strbuf.c
index 7c8f582127..291bdc2a65 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -24,6 +24,17 @@ int istarts_with(const char *str, const char *prefix)
return 0;
}
+int starts_with_mem(const char *str, size_t len, const char *prefix)
+{
+ const char *end = str + len;
+ for (; ; str++, prefix++) {
+ if (!*prefix)
+ return 1;
+ else if (str == end || *str != *prefix)
+ return 0;
+ }
+}
+
int skip_to_optional_arg_default(const char *str, const char *prefix,
const char **arg, const char *def)
{
diff --git a/strbuf.h b/strbuf.h
index 58dddf2777..3156d6ea8c 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -673,6 +673,7 @@ char *xstrfmt(const char *fmt, ...);
int starts_with(const char *str, const char *prefix);
int istarts_with(const char *str, const char *prefix);
+int starts_with_mem(const char *str, size_t len, const char *prefix);
/*
* If the string "str" is the same as the string in "prefix", then the "arg"
diff --git a/trailer.c b/trailer.c
index fe18faf6c5..f59c90b4b5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
/* The first paragraph is the title and cannot be trailers */
for (s = buf; s < buf + len; s = next_line(s)) {
- if (s[0] == comment_line_char)
+ if (starts_with_mem(s, buf + len - s, comment_line_str))
continue;
if (is_blank_line(s))
break;
@@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
const char **p;
ssize_t separator_pos;
- if (bol[0] == comment_line_char) {
+ if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
non_trailer_lines += possible_continuation_lines;
possible_continuation_lines = 0;
continue;
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 12/15] sequencer: handle multi-byte comment characters when writing todo list
From: Jeff King @ 2024-03-07 9:27 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
We already match multi-byte comment characters in parse_insn_line(),
thanks to the previous commit, yielding a TODO_COMMENT entry. But in
todo_list_to_strbuf(), we may call command_to_char() to convert that
back into something we can output.
We can't just return comment_line_char anymore, since it may require
multiple bytes. Instead, we'll return "0" for this case, which is the
same thing we'd return for a command which does not have a single-letter
abbreviation (e.g., "revert" or "noop"). In that case the caller then
falls back to outputting the full name via command_to_string(). So we
can handle TODO_COMMENT there, returning the full string.
Note that there are many other callers of command_to_string(), which
will now behave differently if they pass TODO_COMMENT. But we would not
expect that to happen; prior to this commit, the function just calls
die() in this case. And looking at those callers, that makes sense;
e.g., do_pick_commit() will only be called when servicing a pick
command, and should never be called for a comment in the first place.
Signed-off-by: Jeff King <peff@peff.net>
---
sequencer.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 664986e3b2..9e2851428b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1779,14 +1779,16 @@ static const char *command_to_string(const enum todo_command command)
{
if (command < TODO_COMMENT)
return todo_command_info[command].str;
+ if (command == TODO_COMMENT)
+ return comment_line_str;
die(_("unknown command: %d"), command);
}
static char command_to_char(const enum todo_command command)
{
if (command < TODO_COMMENT)
return todo_command_info[command].c;
- return comment_line_char;
+ return 0;
}
static int is_noop(const enum todo_command command)
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 13/15] wt-status: drop custom comment-char stringification
From: Jeff King @ 2024-03-07 9:28 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
In wt_longstatus_print_tracking() we may conditionally show a comment
prefix based on the wt_status->display_comment_prefix flag. We handle
that by creating a local "comment_line_string" that is either the empty
string or the comment character followed by a space.
For a single-byte comment, the maximum length of this string is 2 (plus
a NUL byte). But to handle multi-byte comment characters, it can be
arbitrarily large. One way to handle this is to just call
xstrfmt("%s ", comment_line_str), and then free it when we're done.
But we can simplify things further by just conditionally switching
between our prefix string and an empty string when formatting. We
couldn't just do that with the previous code, because the comment
character was a single byte. There's no way to have a "%c" format switch
between some character and "no character at all". Whereas with "%s" you
can switch between some string and the empty string. So now that we have
a comment string and not a comment char, we can just use it directly
when formatting. Do note that we have to also conditionally add the
trailing space at the same time.
Signed-off-by: Jeff King <peff@peff.net>
---
I had hoped to clean this up as a preparatory commit, but it really is
awkward until we can make use of comment_line_str, for the reasons given
above.
wt-status.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 084bfc584f..823e8e81b0 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1176,8 +1176,6 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
struct strbuf sb = STRBUF_INIT;
const char *cp, *ep, *branch_name;
struct branch *branch;
- char comment_line_string[3];
- int i;
uint64_t t_begin = 0;
assert(s->branch && !s->is_initial);
@@ -1202,16 +1200,11 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
}
}
- i = 0;
- if (s->display_comment_prefix) {
- comment_line_string[i++] = comment_line_char;
- comment_line_string[i++] = ' ';
- }
- comment_line_string[i] = '\0';
-
for (cp = sb.buf; (ep = strchr(cp, '\n')) != NULL; cp = ep + 1)
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s),
- "%s%.*s", comment_line_string,
+ "%s%s%.*s",
+ s->display_comment_prefix ? comment_line_str : "",
+ s->display_comment_prefix ? " " : "",
(int)(ep - cp), cp);
if (s->display_comment_prefix)
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%s",
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 14/15] environment: drop comment_line_char compatibility macro
From: Jeff King @ 2024-03-07 9:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
There is no longer any code which references the single-byte
comment_line_char. Let's drop it, clearing the way for true multi-byte
entries in comment_line_str.
It's possible there are topics in flight that have added new references
to comment_line_char. But we would prefer to fail compilation (and then
fix it) upon merging with this, rather than have them quietly ignore the
bytes after the first.
Signed-off-by: Jeff King <peff@peff.net>
---
I did merge against 'next' and there are no such topics. And likewise
"log -Scomment_line_char next..seen" shows nothing. But as somebody who
maintained a long-running fork for many years, who knows what people are
carrying in their private trees. ;)
environment.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/environment.h b/environment.h
index 3496474cce..a8b06674eb 100644
--- a/environment.h
+++ b/environment.h
@@ -8,7 +8,6 @@ struct strvec;
* The character that begins a commented line in user-editable file
* that is subject to stripspace.
*/
-#define comment_line_char (comment_line_str[0])
extern const char *comment_line_str;
extern int auto_comment_line_char;
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* [PATCH 15/15] config: allow multi-byte core.commentChar
From: Jeff King @ 2024-03-07 9:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307091407.GA2072522@coredump.intra.peff.net>
Now that all of the code handles multi-byte comment characters, it's
safe to allow users to set them.
There is one special case I kept: we still will not allow an empty
string for the commentChar. While it might make sense in some contexts
(e.g., output where you don't want any comment prefix), there are plenty
where it will behave badly (e.g., all of our starts_with() checks will
indicate that every line is a comment!). It might be reasonable to
assign some meaningful semantics, but it would probably involve checking
how each site behaves. In the interim let's forbid it and we can loosen
things later.
Since comment_line_str is used in many parts of the code, it's hard to
cover all possibilities with tests. We can convert the existing
double-semicolon prefix test to show that "git status" works. And we'll
give it a more challenging case in t7507, where we confirm that
git-commit strips out the commit template along with any --verbose text
when reading the edited commit message back in. That covers the basics,
though it's possible there could be issues in more exotic spots (e.g.,
the sequencer todo list uses its own code).
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously everything works using the "str" variant with a single
character, and many tests are already covering that. You can swap out
the default to "foo>" or something and run the test suite, but there are
many spots that hard-code "#" in their expectations.
I do think it's an acceptable risk, though; for the most part you'd only
find new bugs if you set a multi-byte core.commentChar, which was simply
not allowed before. So we're more likely to see bugs in the new feature
than regression of existing cases.
Documentation/config/core.txt | 4 +++-
config.c | 6 +++---
t/t0030-stripspace.sh | 5 +++++
t/t7507-commit-verbose.sh | 10 ++++++++++
t/t7508-status.sh | 4 +++-
5 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 0e8c2832bf..c86b8c8408 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -523,7 +523,9 @@ core.commentChar::
Commands such as `commit` and `tag` that let you edit
messages consider a line that begins with this character
commented, and removes them after the editor returns
- (default '#').
+ (default '#'). Note that this option can take values larger than
+ a byte (whether a single multi-byte character, or you
+ could even go wild with a multi-character sequence).
+
If set to "auto", `git-commit` would select a character that is not
the beginning character of any line in existing commit messages.
diff --git a/config.c b/config.c
index e12ea68f24..4dea34936c 100644
--- a/config.c
+++ b/config.c
@@ -1565,11 +1565,11 @@ static int git_default_core_config(const char *var, const char *value,
return config_error_nonbool(var);
else if (!strcasecmp(value, "auto"))
auto_comment_line_char = 1;
- else if (value[0] && !value[1]) {
- comment_line_str = xstrfmt("%c", value[0]);
+ else if (value[0]) {
+ comment_line_str = xstrdup(value);
auto_comment_line_char = 0;
} else
- return error(_("core.commentChar should only be one ASCII character"));
+ return error(_("core.commentChar must have at least one character"));
return 0;
}
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index d1b3be8725..9cdf2bddbd 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -401,6 +401,11 @@ test_expect_success 'strip comments with changed comment char' '
test -z "$(echo "; comment" | git -c core.commentchar=";" stripspace -s)"
'
+test_expect_success 'empty commentchar is forbidden' '
+ test_must_fail git -c core.commentchar= stripspace -s 2>err &&
+ grep "core.commentChar must have at least one character" err
+'
+
test_expect_success '-c with single line' '
printf "# foo\n" >expect &&
printf "foo" | git stripspace -c >actual &&
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index c3281b192e..4c7db19ce7 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -101,6 +101,16 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
test_grep "Aborting commit due to empty commit message." err
'
+test_expect_success 'verbose diff is stripped with multi-byte comment char' '
+ (
+ GIT_EDITOR=cat &&
+ export GIT_EDITOR &&
+ test_must_fail git -c core.commentchar="foo>" commit -a -v >out 2>err
+ ) &&
+ grep "^foo> " out &&
+ test_grep "Aborting commit due to empty commit message." err
+'
+
test_expect_success 'status does not verbose without --verbose' '
git status >actual &&
! grep "^diff --git" actual
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index a3c18a4fc2..10ed8b32bc 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1403,7 +1403,9 @@ test_expect_success "status (core.commentchar with submodule summary)" '
test_expect_success "status (core.commentchar with two chars with submodule summary)" '
test_config core.commentchar ";;" &&
- test_must_fail git -c status.displayCommentPrefix=true status
+ sed "s/^/;/" <expect >expect.double &&
+ git -c status.displayCommentPrefix=true status >output &&
+ test_cmp expect.double output
'
test_expect_success "--ignore-submodules=all suppresses submodule summary" '
--
2.44.0.463.g71abcb3a9f
^ permalink raw reply related
* Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers
From: Jeff King @ 2024-03-07 9:47 UTC (permalink / raw)
To: M Hickford; +Cc: Bo Anderson, Bo Anderson via GitGitGadget, git
In-Reply-To: <CAGJzqs=wQA=t4CMVu-kap1ga4DX+KnaVMGy71ewmZ7QkFHF8sg@mail.gmail.com>
On Mon, Mar 04, 2024 at 08:00:00AM +0000, M Hickford wrote:
> > It definitely makes sense in principle. Though the concern perhaps
> > will be that any new features added to the credential helpers and
> > thus its test suite would need adding to each credential helper
> > simultaneously to avoid failing CI. Ideally we would do exactly
> > that, though that requires knowledge on each of the keystore APIs
> > used in each of the credential helpers.
>
> Good point.
I think we suffer from that somewhat already. You cannot run t0303
successfully against credential-store anymore, as of 0ce02e2fec
(credential/libsecret: store new attributes, 2023-06-16).
There is some prior art in the GIT_TEST_CREDENTIAL_HELPER_TIMEOUT
variable, as time is not a concept to every helper (like store, for
example). Other new tests like the password-expiry and oauth features
could be gated on similar variables. That would help non-CI users
testing helpers manually, and then CI jobs could set the appropriate
switches for each helper that they cover.
All that said, I'd be surprised if testing osxkeychain in the CI
environment worked. Back when I worked on it in 2011, I found that I had
to actually run the tests in a local terminal; even a remote ssh login
could not access the keychain. It's possible that things have changed
since then, though, or perhaps I was imply ignorant of how to configure
things correctly.
-Peff
^ permalink raw reply
* Re: [PATCH 06/15] strbuf: accept a comment string for strbuf_stripspace()
From: Jeff King @ 2024-03-07 9:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk,
Manlio Perillo
In-Reply-To: <20240307092126.GF2080210@coredump.intra.peff.net>
On Thu, Mar 07, 2024 at 04:21:26AM -0500, Jeff King wrote:
> As part of our transition to multi-byte comment characters, let's take a
> NUL-terminated string pointer for strbuf_stripspace(), rather than a
> single character. We can continue to support its feature of ignoring
> comments by accepting a NULL pointer (as opposed to the current behavior
> of a NUL byte).
>
> All of the callers have to be adjusted, but they can all just pass
> comment_line_str (or NULL).
Bah. I relied on the compiler to tell me the call-sites that needed to
be adjusted. But interestingly gcc is quite happy to allow '\0' to be
passed in place of a pointer, but clang complains:
gpg-interface.c:589:37: error: expression which evaluates to zero treated as a null pointer constant of type 'const char *' [-Werror,-Wnon-literal-null-conversion]
strbuf_stripspace(&ssh_keygen_out, '\0');
^~~~
Likewise there are a few bare "0"'s which do not cause a warning, but
which violate our style standards. So I think we'd want to squash the
patch below in to this step. The other functions don't need the same
treatment because they never treated NUL specially.
---
diff --git a/builtin/am.c b/builtin/am.c
index d1990d7edc..5bc72d7822 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1286,7 +1286,7 @@ static int parse_mail(struct am_state *state, const char *mail)
strbuf_addstr(&msg, "\n\n");
strbuf_addbuf(&msg, &mi.log_message);
- strbuf_stripspace(&msg, '\0');
+ strbuf_stripspace(&msg, NULL);
assert(!state->author_name);
state->author_name = strbuf_detach(&author_name, NULL);
diff --git a/builtin/commit.c b/builtin/commit.c
index 8519a004d0..e04f1236e8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -890,7 +890,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
s->hints = 0;
if (clean_message_contents)
- strbuf_stripspace(&sb, '\0');
+ strbuf_stripspace(&sb, NULL);
if (signoff)
append_signoff(&sb, ignored_log_message_bytes(sb.buf, sb.len), 0);
diff --git a/builtin/notes.c b/builtin/notes.c
index 1a67f01d00..cb011303e6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -264,7 +264,7 @@ static void concat_messages(struct note_data *d)
if ((d->stripspace == UNSPECIFIED &&
d->messages[i]->stripspace == STRIPSPACE) ||
d->stripspace == STRIPSPACE)
- strbuf_stripspace(&d->buf, 0);
+ strbuf_stripspace(&d->buf, NULL);
strbuf_reset(&msg);
}
strbuf_release(&msg);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9c76b62b02..f0aa962cf8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -657,7 +657,7 @@ static int can_use_local_refs(const struct add_opts *opts)
strbuf_add_real_path(&path, get_worktree_git_dir(NULL));
strbuf_addstr(&path, "/HEAD");
strbuf_read_file(&contents, path.buf, 64);
- strbuf_stripspace(&contents, 0);
+ strbuf_stripspace(&contents, NULL);
strbuf_strip_suffix(&contents, "\n");
warning(_("HEAD points to an invalid (or orphaned) reference.\n"
diff --git a/gpg-interface.c b/gpg-interface.c
index 95e764acb1..b5993385ff 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -586,8 +586,8 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
}
}
- strbuf_stripspace(&ssh_keygen_out, '\0');
- strbuf_stripspace(&ssh_keygen_err, '\0');
+ strbuf_stripspace(&ssh_keygen_out, NULL);
+ strbuf_stripspace(&ssh_keygen_err, NULL);
/* Add stderr outputs to show the user actual ssh-keygen errors */
strbuf_add(&ssh_keygen_out, ssh_principals_err.buf, ssh_principals_err.len);
strbuf_add(&ssh_keygen_out, ssh_keygen_err.buf, ssh_keygen_err.len);
^ permalink raw reply related
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Jeff King @ 2024-03-07 9:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq1q8xx38i.fsf@gitster.g>
On Tue, Feb 27, 2024 at 08:48:29AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > There's some prior art there, I think, in how GIT_CEILING_DIRECTORIES
> > works, or even something like "git --literal-pathspecs", neither of
> > which appear in local_repo_env.
>
> Unlike GIT_CEILING_DIRECTORIES that is more or less permanent and a
> part of configuring an everyday environment for real work, I view
> this --no-lazy-fetch thing as solely a debugging aid. A repository
> with promisor wouldn't be able to "fill the gap" by forbidding
> on-demand fetching of promised objects while running say "git fetch"
> from elsewhere and it just will die with "some objects we are
> supposed to have are missing from this repository".
>
> So I do not have a strong opinion either way, if it is more
> convenient to propagate the request out to other repositories when
> we run processes in two or more repositories (e.g. "git clone
> --local"), or if it is more convenient to make sure that the request
> is limited to the target repository. Here is a version without the
> local_repo_env[] change.
Yeah, GIT_CEILING_DIRECTORIES is maybe a bad example. But I do think
LITERAL_PATHSPECS is a better one, and the submodule-fetch example I
gave would be genuinely surprising if it behaved differently than the
superproject, I'd think.
I do agree this is probably going to mostly be a debugging aid, so it
might not matter much. But once in the wild these things tend to take on
a life of their own. ;)
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH v3 3/3] git: extend --no-lazy-fetch to work across subprocesses
So anyway, this version seems good to me.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] mem-pool: add mem_pool_strfmt()
From: Jeff King @ 2024-03-07 9:58 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <3aed87d0-4789-4e96-8c6c-31cab2d996cc@web.de>
On Tue, Feb 27, 2024 at 06:58:26PM +0100, René Scharfe wrote:
> >> It would not allow the shortcut of using the vast pool as a scratch
> >> space to format the string with a single vsnprintf call in most cases.
> >> Or am I missing something?
> >
> > So here it sounds like you do care about some of the performance
> > aspects. So no, it would not allow that. You'd be using the vast pool of
> > heap memory provided by malloc(), and trusting that a call to malloc()
> > is not that expensive in practice. I don't know how true that is, or how
> > much it matters for this case.
>
> In the specific use case we can look at three cases:
>
> 1. xstrfmt() [before 1c56fc2084]
> 2. size calculation + pre-size + strbuf_addf() [1c56fc2084]
> 3. mem_pool_strfmt() [this patch]
>
> The performance of 2 and 3 is basically the same, 1 was worse.
>
> xstrfmt() and strbuf_addf() both wrap strbuf_vaddf(), which uses
> malloc() and vsnprintf(). My conclusion is that malloc() is fast
> enough, but running vsnprintf() twice is slow (first time to determine
> the allocation size, second time to actually build the string). With a
> memory pool we almost always only need to call it once per string, and
> that's why I use it here.
>
> The benefit of this patch (to me) is better code readability (no more
> manual pre-sizing) without sacrificing performance.
>
> The ability to clear (or UNLEAK) all these strings in one go is just a
> bonus.
Ah, OK. I admit I did not read the series all that carefully. Mostly I
am just concerned about a world where there are parallel-universe
versions of every allocating function that takes a mem-pool. If it's
limited to this obvious string formatting variant that may not be too
bad, though.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.
From: Jeff King @ 2024-03-07 10:00 UTC (permalink / raw)
To: rsbecker; +Cc: 'Junio C Hamano', 'Randall S. Becker', git
In-Reply-To: <03d701da69c0$c3430e80$49c92b80$@nexbridge.com>
On Tue, Feb 27, 2024 at 04:05:53PM -0500, rsbecker@nexbridge.com wrote:
> Unfortunately, I do not have sufficient knowledge of the code to
> resolve the originally reported problem without further assistance to
> determine the root case (assuming it still is a problem). Changes in
> master post-2.44.0 appear to have contributed to resolving the
> situation, so I am now getting random pass/fail on the test. I'm going
> to hold 2.44.0 on ia64 and wait for a subsequent release at retest at
> that time.
If you're getting random pass/fail (which does seem like the kind of
thing that could be related to pipe write() sizes), you might try using
the "--stress" argument. That can give you more consistent results while
bisecting (e.g., if "--stress" runs successfully for a few minutes).
That said, given the failing test you mentioned, I kind of assume that
it was not a code change that caused the problem, but rather a new test
exercising new code that happens to tickle your race.
-Peff
^ permalink raw reply
* Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers
From: Jeff King @ 2024-03-07 11:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Dragan Simic,
Kristoffer Haugsbakk, Manlio Perillo
In-Reply-To: <20240307092638.GK2080210@coredump.intra.peff.net>
On Thu, Mar 07, 2024 at 04:26:38AM -0500, Jeff King wrote:
> IMHO this is the trickiest commit of the whole series, as it would be
> easy to get the length computations subtly wrong.
And sure enough...
> diff --git a/trailer.c b/trailer.c
> index fe18faf6c5..f59c90b4b5 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>
> /* The first paragraph is the title and cannot be trailers */
> for (s = buf; s < buf + len; s = next_line(s)) {
> - if (s[0] == comment_line_char)
> + if (starts_with_mem(s, buf + len - s, comment_line_str))
> continue;
> if (is_blank_line(s))
> break;
> @@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
> const char **p;
> ssize_t separator_pos;
>
> - if (bol[0] == comment_line_char) {
> + if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
> non_trailer_lines += possible_continuation_lines;
> possible_continuation_lines = 0;
> continue;
This second hunk needs:
diff --git a/trailer.c b/trailer.c
index f59c90b4b5..fdb0b8137e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
const char **p;
ssize_t separator_pos;
- if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
+ if (starts_with_mem(bol, buf + len - bol, comment_line_str)) {
non_trailer_lines += possible_continuation_lines;
possible_continuation_lines = 0;
continue;
I was trying to bound the size based on the loop, which is:
for (l = last_line(buf, len);
l >= end_of_title;
l = last_line(buf, l)) {
const char *bol = buf + l;
but I misread "end_of_title" as an upper bound, not a lower one. Which
makes sense because we're iterating backwards over the lines. So I
suppose we could bound it by the previous "bol" value. But in practice,
your prefix won't cross such a boundary anyway, as it won't have a
newline in it (maybe that's something we should enforce? I guess you
could set core.commentChar to '\n' even before my series, which would be
slightly insane).
So just bounding ourselves to "buf + len" seems reasonable, as that
makes sure we don't step outside the buffer passed into the function.
Curiously, this was found by the sanitizer job in CI, where UBSan
complains of integer overflow in the pointer computation. I had run with
both ASan/UBSan locally, but just using gcc, which doesn't seem to find
it (the CI job uses clang). So I'll that to my mental tally of "clang
seems to be better with sanitizers".
-Peff
^ permalink raw reply related
* Re: [PATCH] Allow git-config to append a comment
From: Junio C Hamano @ 2024-03-07 12:12 UTC (permalink / raw)
To: Ralph Seichter; +Cc: Ralph Seichter via GitGitGadget, git
In-Reply-To: <2560952c-4495-4a71-9497-aa40032e1d2b@seichter.de>
Ralph Seichter <github@seichter.de> writes:
>> If you are illustrating a sample input, please also explain what
>> output it produces. What do the resulting lines in the config file
>> look like after you run this command?
>
> The result of running the above command looks as follows:
>
> [safe]
> directory = /home/alice/somerepo.git #I changed this. --A. Script
That would have been a crucial piece of information to have in the
proposed log message, as limiting ourselves to a comment that is
tucked after the same line as the value, things can become somewhat
simplified. We may not have to worry about deletion, even though the
point about "we need to look at and typofix them with our viewers
and editors" still stands.
By the way, you may or may not have noticed it, but my example
deliberately had a multi-line comment:
$ git config --global --comment 'the reason why I added ~alice/
is because ...' --add safe.directory /home/alice/somerepo.git
How such a thing is handled also needs to be discussed in the
proposed log message, and perhaps in the documentation as well.
> ... My patch only supports
> single-line comments, and only as a suffix to newly added key-value
> pairs. This is a deliberate design choice.
Such design choices need to be described in the proposed log message
to help future developers who will be updating this feature, once it
gets in.
Thanks for writing quite a lot to answer _my_ questions, but these
questions are samples of things that future developers would wonder
and ask about when they want to fix bugs in, enhance, or otherwise
modify the implementation of this "add comment" feature. They may
even be working on adding other features to complement the "add
comment" feature, by designing support for viewing or typofixing
existing comments. When they do so, it would help them to know how
this existing feature was expected to be used and how it would fit
in a larger picture (which may not have yet existed back when the
feature was invented). Answering these anticipated questions is one
of the greatest things that a commit log message can do to help
them.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/4] reftable/stack: register compacted tables as tempfiles
From: Toon claes @ 2024-03-07 12:38 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <b952d54a05e1c0cf47371f78e3901cfb2119e246.1709549619.git.ps@pks.im>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 977336b7d5..40129da16c 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
>
> static int stack_compact_locked(struct reftable_stack *st,
> size_t first, size_t last,
> - struct strbuf *temp_tab,
> - struct reftable_log_expiry_config *config)
> + struct reftable_log_expiry_config *config,
> + struct tempfile **temp_table_out)
> {
> struct strbuf next_name = STRBUF_INIT;
> - int tab_fd = -1;
> + struct strbuf table_path = STRBUF_INIT;
> struct reftable_writer *wr = NULL;
> + struct tempfile *temp_table;
> + int temp_table_fd;
Just one small nit, if you don't mind? In PATCH 2/4 you use
`struct tempfile *tab_file` and `int tab_fd`. I would like to see
consistency and use similar names. Personally I don't like table being
shortened to "tab", and I think you feel the same as you've renamed the
parameter from this function.
--
Toon
^ permalink raw reply
* Re: [PATCH] Allow git-config to append a comment
From: Ralph Seichter @ 2024-03-07 12:44 UTC (permalink / raw)
To: Junio C Hamano, Ralph Seichter; +Cc: Ralph Seichter via GitGitGadget, git
In-Reply-To: <xmqqplw6nsuz.fsf@gitster.g>
* Junio C. Hamano:
>> My patch only supports single-line comments, and only as a suffix to
>> newly added key-value pairs. This is a deliberate design choice.
>
> Such design choices need to be described in the proposed log message
> to help future developers who will be updating this feature, once it
> gets in.
Just as a brief interjection: I am sorry that my inexperience with Git's
mailing-list based process caused me to leave out details which were
discussed earlier, be it on GitHub or Discord. However, me not
mentioning that I am aiming for single-line suffix comments only was due
to simple forgetfulness, without an excuse behind it. My bad.
I think it best I flesh out the commit message before anything else, and
then resubmit. That should bundle the necessary information.
-Ralph
^ permalink raw reply
* Re: [PATCH 4/4] reftable/stack: register compacted tables as tempfiles
From: Patrick Steinhardt @ 2024-03-07 12:58 UTC (permalink / raw)
To: Toon claes; +Cc: git
In-Reply-To: <87sf12fc7z.fsf@to1.studio>
[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]
On Thu, Mar 07, 2024 at 01:38:56PM +0100, Toon claes wrote:
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index 977336b7d5..40129da16c 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
> >
> > static int stack_compact_locked(struct reftable_stack *st,
> > size_t first, size_t last,
> > - struct strbuf *temp_tab,
> > - struct reftable_log_expiry_config *config)
> > + struct reftable_log_expiry_config *config,
> > + struct tempfile **temp_table_out)
> > {
> > struct strbuf next_name = STRBUF_INIT;
> > - int tab_fd = -1;
> > + struct strbuf table_path = STRBUF_INIT;
> > struct reftable_writer *wr = NULL;
> > + struct tempfile *temp_table;
> > + int temp_table_fd;
>
> Just one small nit, if you don't mind? In PATCH 2/4 you use
> `struct tempfile *tab_file` and `int tab_fd`. I would like to see
> consistency and use similar names. Personally I don't like table being
> shortened to "tab", and I think you feel the same as you've renamed the
> parameter from this function.
Yeah, I'm always a proponent of descriptive, unabbreviated names and
would thus rather want to use "temp_table" or something like that. But
in 2/4 I decided to stick with "tab_file" because there is a bunch of
already existing variables in that function which are called similar --
"temp_tab_file_name", "tab_file_name" and "tab_fd". But renaming all of
them to ensure function-level consistency would create a lot of churn.
I'll thus instead rename variables over here to their abbreviated
versions.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 0/4] reftable/stack: register temporary files
From: Patrick Steinhardt @ 2024-03-07 13:10 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Toon claes, Junio C Hamano
In-Reply-To: <cover.1709549619.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5192 bytes --]
Hi,
this is the second version of my patch series that registers temporary
files written by the reftable library with the tempfile framework. This
ensures that those files get cleaned up even when Git dies or otherwise
gets signalled.
Changes compared to v1:
- Patch 1: Clarify that rolling back a deactivated lockfile will not
result in an error.
- Patch 4: Rename some local variables for improved consistency with
other code.
Patrick
Patrick Steinhardt (4):
lockfile: report when rollback fails
reftable/stack: register new tables as tempfiles
reftable/stack: register lockfiles during compaction
reftable/stack: register compacted tables as tempfiles
lockfile.h | 6 +-
reftable/stack.c | 330 ++++++++++++++++++++++------------------------
reftable/system.h | 2 +
tempfile.c | 21 +--
tempfile.h | 2 +-
5 files changed, 178 insertions(+), 183 deletions(-)
Range-diff against v1:
1: 1acaa9ca1a ! 1: 782e96a678 lockfile: report when rollback fails
@@ Commit message
## lockfile.h ##
@@ lockfile.h: static inline int commit_lock_file_to(struct lock_file *lk, const char *path)
+ * Roll back `lk`: close the file descriptor and/or file pointer and
+ * remove the lockfile. It is a NOOP to call `rollback_lock_file()`
* for a `lock_file` object that has already been committed or rolled
- * back.
+- * back.
++ * back. No error will be returned in this case.
*/
-static inline void rollback_lock_file(struct lock_file *lk)
+static inline int rollback_lock_file(struct lock_file *lk)
2: 02bf41d419 = 2: 5dbc93d5be reftable/stack: register new tables as tempfiles
3: 45b5c3167f = 3: c88c85443e reftable/stack: register lockfiles during compaction
4: b952d54a05 ! 4: 4023d78f08 reftable/stack: register compacted tables as tempfiles
@@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac
- struct strbuf *temp_tab,
- struct reftable_log_expiry_config *config)
+ struct reftable_log_expiry_config *config,
-+ struct tempfile **temp_table_out)
++ struct tempfile **tab_file_out)
{
struct strbuf next_name = STRBUF_INIT;
- int tab_fd = -1;
-+ struct strbuf table_path = STRBUF_INIT;
++ struct strbuf tab_file_path = STRBUF_INIT;
struct reftable_writer *wr = NULL;
-+ struct tempfile *temp_table;
-+ int temp_table_fd;
- int err = 0;
+- int err = 0;
++ struct tempfile *tab_file;
++ int tab_fd, err = 0;
format_name(&next_name,
reftable_reader_min_update_index(st->readers[first]),
reftable_reader_max_update_index(st->readers[last]));
-+ stack_filename(&table_path, st, next_name.buf);
-+ strbuf_addstr(&table_path, ".temp.XXXXXX");
++ stack_filename(&tab_file_path, st, next_name.buf);
++ strbuf_addstr(&tab_file_path, ".temp.XXXXXX");
- stack_filename(temp_tab, st, next_name.buf);
- strbuf_addstr(temp_tab, ".temp.XXXXXX");
-+ temp_table = mks_tempfile(table_path.buf);
-+ if (!temp_table) {
++ tab_file = mks_tempfile(tab_file_path.buf);
++ if (!tab_file) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
+ }
-+ temp_table_fd = get_tempfile_fd(temp_table);
++ tab_fd = get_tempfile_fd(tab_file);
- tab_fd = mkstemp(temp_tab->buf);
if (st->config.default_permissions &&
- chmod(temp_tab->buf, st->config.default_permissions) < 0) {
-+ chmod(get_tempfile_path(temp_table), st->config.default_permissions) < 0) {
++ chmod(get_tempfile_path(tab_file), st->config.default_permissions) < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
@@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac
- wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);
-
+ wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,
-+ &temp_table_fd, &st->config);
++ &tab_fd, &st->config);
err = stack_write_compact(st, wr, first, last, config);
if (err < 0)
goto done;
@@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac
- err = close(tab_fd);
- tab_fd = 0;
-+ err = close_tempfile_gently(temp_table);
++ err = close_tempfile_gently(tab_file);
+ if (err < 0)
+ goto done;
+
-+ *temp_table_out = temp_table;
-+ temp_table = NULL;
++ *tab_file_out = tab_file;
++ tab_file = NULL;
done:
-+ delete_tempfile(&temp_table);
++ delete_tempfile(&tab_file);
reftable_writer_free(wr);
- if (tab_fd > 0) {
- close(tab_fd);
@@ reftable/stack.c: uint64_t reftable_stack_next_update_index(struct reftable_stac
- strbuf_release(temp_tab);
- }
strbuf_release(&next_name);
-+ strbuf_release(&table_path);
++ strbuf_release(&tab_file_path);
return err;
}
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 1/4] lockfile: report when rollback fails
From: Patrick Steinhardt @ 2024-03-07 13:10 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Toon claes, Junio C Hamano
In-Reply-To: <cover.1709816483.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]
We do not report to the caller when rolling back a lockfile fails, which
will be needed by the reftable compaction logic in a subsequent commit.
It also cannot really report on all errors because the function calls
`delete_tempfile()`, which doesn't return an error either.
Refactor the code so that both `delete_tempfile()` and
`rollback_lock_file()` return an error code.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
lockfile.h | 6 +++---
tempfile.c | 21 +++++++++++++--------
tempfile.h | 2 +-
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/lockfile.h b/lockfile.h
index 90af4e66b2..1bb9926497 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -321,11 +321,11 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)
* Roll back `lk`: close the file descriptor and/or file pointer and
* remove the lockfile. It is a NOOP to call `rollback_lock_file()`
* for a `lock_file` object that has already been committed or rolled
- * back.
+ * back. No error will be returned in this case.
*/
-static inline void rollback_lock_file(struct lock_file *lk)
+static inline int rollback_lock_file(struct lock_file *lk)
{
- delete_tempfile(&lk->tempfile);
+ return delete_tempfile(&lk->tempfile);
}
#endif /* LOCKFILE_H */
diff --git a/tempfile.c b/tempfile.c
index ecdebf1afb..ed88cf8431 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -50,15 +50,17 @@
static VOLATILE_LIST_HEAD(tempfile_list);
-static void remove_template_directory(struct tempfile *tempfile,
+static int remove_template_directory(struct tempfile *tempfile,
int in_signal_handler)
{
if (tempfile->directory) {
if (in_signal_handler)
- rmdir(tempfile->directory);
+ return rmdir(tempfile->directory);
else
- rmdir_or_warn(tempfile->directory);
+ return rmdir_or_warn(tempfile->directory);
}
+
+ return 0;
}
static void remove_tempfiles(int in_signal_handler)
@@ -353,16 +355,19 @@ int rename_tempfile(struct tempfile **tempfile_p, const char *path)
return 0;
}
-void delete_tempfile(struct tempfile **tempfile_p)
+int delete_tempfile(struct tempfile **tempfile_p)
{
struct tempfile *tempfile = *tempfile_p;
+ int err = 0;
if (!is_tempfile_active(tempfile))
- return;
+ return 0;
- close_tempfile_gently(tempfile);
- unlink_or_warn(tempfile->filename.buf);
- remove_template_directory(tempfile, 0);
+ err |= close_tempfile_gently(tempfile);
+ err |= unlink_or_warn(tempfile->filename.buf);
+ err |= remove_template_directory(tempfile, 0);
deactivate_tempfile(tempfile);
*tempfile_p = NULL;
+
+ return err ? -1 : 0;
}
diff --git a/tempfile.h b/tempfile.h
index d0413af733..2d2ae5b657 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -269,7 +269,7 @@ int reopen_tempfile(struct tempfile *tempfile);
* `delete_tempfile()` for a `tempfile` object that has already been
* deleted or renamed.
*/
-void delete_tempfile(struct tempfile **tempfile_p);
+int delete_tempfile(struct tempfile **tempfile_p);
/*
* Close the file descriptor and/or file pointer if they are still
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/4] reftable/stack: register new tables as tempfiles
From: Patrick Steinhardt @ 2024-03-07 13:10 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Toon claes, Junio C Hamano
In-Reply-To: <cover.1709816483.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]
We do not register new tables which we're about to add to the stack with
the tempfile API. Those tables will thus not be deleted in case Git gets
killed.
Refactor the code to register tables as tempfiles.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index b64e55648a..81544fbfa0 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,
struct strbuf tab_file_name = STRBUF_INIT;
struct strbuf next_name = STRBUF_INIT;
struct reftable_writer *wr = NULL;
+ struct tempfile *tab_file = NULL;
int err = 0;
- int tab_fd = 0;
+ int tab_fd;
strbuf_reset(&next_name);
format_name(&next_name, add->next_update_index, add->next_update_index);
@@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,
stack_filename(&temp_tab_file_name, add->stack, next_name.buf);
strbuf_addstr(&temp_tab_file_name, ".temp.XXXXXX");
- tab_fd = mkstemp(temp_tab_file_name.buf);
- if (tab_fd < 0) {
+ tab_file = mks_tempfile(temp_tab_file_name.buf);
+ if (!tab_file) {
err = REFTABLE_IO_ERROR;
goto done;
}
if (add->stack->config.default_permissions) {
- if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
+ if (chmod(get_tempfile_path(tab_file),
+ add->stack->config.default_permissions)) {
err = REFTABLE_IO_ERROR;
goto done;
}
}
+ tab_fd = get_tempfile_fd(tab_file);
+
wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,
&add->stack->config);
err = write_table(wr, arg);
@@ -771,14 +775,13 @@ int reftable_addition_add(struct reftable_addition *add,
if (err < 0)
goto done;
- err = close(tab_fd);
- tab_fd = 0;
+ err = close_tempfile_gently(tab_file);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
- err = stack_check_addition(add->stack, temp_tab_file_name.buf);
+ err = stack_check_addition(add->stack, get_tempfile_path(tab_file));
if (err < 0)
goto done;
@@ -789,14 +792,13 @@ int reftable_addition_add(struct reftable_addition *add,
format_name(&next_name, wr->min_update_index, wr->max_update_index);
strbuf_addstr(&next_name, ".ref");
-
stack_filename(&tab_file_name, add->stack, next_name.buf);
/*
On windows, this relies on rand() picking a unique destination name.
Maybe we should do retry loop as well?
*/
- err = rename(temp_tab_file_name.buf, tab_file_name.buf);
+ err = rename_tempfile(&tab_file, tab_file_name.buf);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
@@ -806,14 +808,7 @@ int reftable_addition_add(struct reftable_addition *add,
add->new_tables_cap);
add->new_tables[add->new_tables_len++] = strbuf_detach(&next_name, NULL);
done:
- if (tab_fd > 0) {
- close(tab_fd);
- tab_fd = 0;
- }
- if (temp_tab_file_name.len > 0) {
- unlink(temp_tab_file_name.buf);
- }
-
+ delete_tempfile(&tab_file);
strbuf_release(&temp_tab_file_name);
strbuf_release(&tab_file_name);
strbuf_release(&next_name);
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/4] reftable/stack: register lockfiles during compaction
From: Patrick Steinhardt @ 2024-03-07 13:10 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Toon claes, Junio C Hamano
In-Reply-To: <cover.1709816483.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 10974 bytes --]
We do not register any of the locks we acquire when compacting the
reftable stack via our lockfiles interfaces. These locks will thus not
be released when Git gets killed.
Refactor the code to register locks as lockfiles.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 255 ++++++++++++++++++++++------------------------
reftable/system.h | 2 +
2 files changed, 123 insertions(+), 134 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 81544fbfa0..977336b7d5 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -978,212 +978,199 @@ static int stack_compact_range(struct reftable_stack *st,
size_t first, size_t last,
struct reftable_log_expiry_config *expiry)
{
- char **delete_on_success = NULL, **subtable_locks = NULL, **listp = NULL;
- struct strbuf temp_tab_file_name = STRBUF_INIT;
+ struct strbuf tables_list_buf = STRBUF_INIT;
+ struct strbuf new_table_temp_path = STRBUF_INIT;
struct strbuf new_table_name = STRBUF_INIT;
- struct strbuf lock_file_name = STRBUF_INIT;
- struct strbuf ref_list_contents = STRBUF_INIT;
struct strbuf new_table_path = STRBUF_INIT;
- size_t i, j, compact_count;
- int err = 0;
- int have_lock = 0;
- int lock_file_fd = -1;
- int is_empty_table = 0;
+ struct strbuf table_name = STRBUF_INIT;
+ struct lock_file tables_list_lock = LOCK_INIT;
+ struct lock_file *table_locks = NULL;
+ int is_empty_table = 0, err = 0;
+ size_t i;
if (first > last || (!expiry && first == last)) {
err = 0;
goto done;
}
- compact_count = last - first + 1;
- REFTABLE_CALLOC_ARRAY(delete_on_success, compact_count + 1);
- REFTABLE_CALLOC_ARRAY(subtable_locks, compact_count + 1);
-
st->stats.attempts++;
- strbuf_reset(&lock_file_name);
- strbuf_addstr(&lock_file_name, st->list_file);
- strbuf_addstr(&lock_file_name, ".lock");
-
- lock_file_fd =
- open(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);
- if (lock_file_fd < 0) {
- if (errno == EEXIST) {
+ /*
+ * Hold the lock so that we can read "tables.list" and lock all tables
+ * which are part of the user-specified range.
+ */
+ err = hold_lock_file_for_update(&tables_list_lock, st->list_file,
+ LOCK_NO_DEREF);
+ if (err < 0) {
+ if (errno == EEXIST)
err = 1;
- } else {
+ else
err = REFTABLE_IO_ERROR;
- }
goto done;
}
- /* Don't want to write to the lock for now. */
- close(lock_file_fd);
- lock_file_fd = -1;
- have_lock = 1;
err = stack_uptodate(st);
- if (err != 0)
+ if (err)
goto done;
- for (i = first, j = 0; i <= last; i++) {
- struct strbuf subtab_file_name = STRBUF_INIT;
- struct strbuf subtab_lock = STRBUF_INIT;
- int sublock_file_fd = -1;
-
- stack_filename(&subtab_file_name, st,
- reader_name(st->readers[i]));
-
- strbuf_reset(&subtab_lock);
- strbuf_addbuf(&subtab_lock, &subtab_file_name);
- strbuf_addstr(&subtab_lock, ".lock");
+ /*
+ * Lock all tables in the user-provided range. This is the slice of our
+ * stack which we'll compact.
+ */
+ REFTABLE_CALLOC_ARRAY(table_locks, last - first + 1);
+ for (i = first; i <= last; i++) {
+ stack_filename(&table_name, st, reader_name(st->readers[i]));
- sublock_file_fd = open(subtab_lock.buf,
- O_EXCL | O_CREAT | O_WRONLY, 0666);
- if (sublock_file_fd >= 0) {
- close(sublock_file_fd);
- } else if (sublock_file_fd < 0) {
- if (errno == EEXIST) {
+ err = hold_lock_file_for_update(&table_locks[i - first],
+ table_name.buf, LOCK_NO_DEREF);
+ if (err < 0) {
+ if (errno == EEXIST)
err = 1;
- } else {
+ else
err = REFTABLE_IO_ERROR;
- }
+ goto done;
}
- subtable_locks[j] = subtab_lock.buf;
- delete_on_success[j] = subtab_file_name.buf;
- j++;
-
- if (err != 0)
+ /*
+ * We need to close the lockfiles as we might otherwise easily
+ * run into file descriptor exhaustion when we compress a lot
+ * of tables.
+ */
+ err = close_lock_file_gently(&table_locks[i - first]);
+ if (err < 0) {
+ err = REFTABLE_IO_ERROR;
goto done;
+ }
}
- err = unlink(lock_file_name.buf);
- if (err < 0)
+ /*
+ * We have locked all tables in our range and can thus release the
+ * "tables.list" lock while compacting the locked tables. This allows
+ * concurrent updates to the stack to proceed.
+ */
+ err = rollback_lock_file(&tables_list_lock);
+ if (err < 0) {
+ err = REFTABLE_IO_ERROR;
goto done;
- have_lock = 0;
-
- err = stack_compact_locked(st, first, last, &temp_tab_file_name,
- expiry);
- /* Compaction + tombstones can create an empty table out of non-empty
- * tables. */
- is_empty_table = (err == REFTABLE_EMPTY_TABLE_ERROR);
- if (is_empty_table) {
- err = 0;
}
- if (err < 0)
- goto done;
- lock_file_fd =
- open(lock_file_name.buf, O_EXCL | O_CREAT | O_WRONLY, 0666);
- if (lock_file_fd < 0) {
- if (errno == EEXIST) {
+ /*
+ * Compact the now-locked tables into a new table. Note that compacting
+ * these tables may end up with an empty new table in case tombstones
+ * end up cancelling out all refs in that range.
+ */
+ err = stack_compact_locked(st, first, last, &new_table_temp_path, expiry);
+ if (err < 0) {
+ if (err != REFTABLE_EMPTY_TABLE_ERROR)
+ goto done;
+ is_empty_table = 1;
+ }
+
+ /*
+ * Now that we have written the new, compacted table we need to re-lock
+ * "tables.list". We'll then replace the compacted range of tables with
+ * the new table.
+ */
+ err = hold_lock_file_for_update(&tables_list_lock, st->list_file,
+ LOCK_NO_DEREF);
+ if (err < 0) {
+ if (errno == EEXIST)
err = 1;
- } else {
+ else
err = REFTABLE_IO_ERROR;
- }
goto done;
}
- have_lock = 1;
+
if (st->config.default_permissions) {
- if (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {
+ if (chmod(get_lock_file_path(&tables_list_lock),
+ st->config.default_permissions) < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
}
- format_name(&new_table_name, st->readers[first]->min_update_index,
- st->readers[last]->max_update_index);
- strbuf_addstr(&new_table_name, ".ref");
-
- stack_filename(&new_table_path, st, new_table_name.buf);
-
+ /*
+ * If the resulting compacted table is not empty, then we need to move
+ * it into place now.
+ */
if (!is_empty_table) {
- /* retry? */
- err = rename(temp_tab_file_name.buf, new_table_path.buf);
+ format_name(&new_table_name, st->readers[first]->min_update_index,
+ st->readers[last]->max_update_index);
+ strbuf_addstr(&new_table_name, ".ref");
+ stack_filename(&new_table_path, st, new_table_name.buf);
+
+ err = rename(new_table_temp_path.buf, new_table_path.buf);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
}
- for (i = 0; i < first; i++) {
- strbuf_addstr(&ref_list_contents, st->readers[i]->name);
- strbuf_addstr(&ref_list_contents, "\n");
- }
- if (!is_empty_table) {
- strbuf_addbuf(&ref_list_contents, &new_table_name);
- strbuf_addstr(&ref_list_contents, "\n");
- }
- for (i = last + 1; i < st->merged->stack_len; i++) {
- strbuf_addstr(&ref_list_contents, st->readers[i]->name);
- strbuf_addstr(&ref_list_contents, "\n");
- }
-
- err = write_in_full(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);
- if (err < 0) {
- err = REFTABLE_IO_ERROR;
- unlink(new_table_path.buf);
- goto done;
- }
-
- err = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd);
+ /*
+ * Write the new "tables.list" contents with the compacted table we
+ * have just written. In case the compacted table became empty we
+ * simply skip writing it.
+ */
+ for (i = 0; i < first; i++)
+ strbuf_addf(&tables_list_buf, "%s\n", st->readers[i]->name);
+ if (!is_empty_table)
+ strbuf_addf(&tables_list_buf, "%s\n", new_table_name.buf);
+ for (i = last + 1; i < st->merged->stack_len; i++)
+ strbuf_addf(&tables_list_buf, "%s\n", st->readers[i]->name);
+
+ err = write_in_full(get_lock_file_fd(&tables_list_lock),
+ tables_list_buf.buf, tables_list_buf.len);
if (err < 0) {
err = REFTABLE_IO_ERROR;
unlink(new_table_path.buf);
goto done;
}
- err = close(lock_file_fd);
- lock_file_fd = -1;
+ err = fsync_component(FSYNC_COMPONENT_REFERENCE, get_lock_file_fd(&tables_list_lock));
if (err < 0) {
err = REFTABLE_IO_ERROR;
unlink(new_table_path.buf);
goto done;
}
- err = rename(lock_file_name.buf, st->list_file);
+ err = commit_lock_file(&tables_list_lock);
if (err < 0) {
err = REFTABLE_IO_ERROR;
unlink(new_table_path.buf);
goto done;
}
- have_lock = 0;
- /* Reload the stack before deleting. On windows, we can only delete the
- files after we closed them.
- */
+ /*
+ * Reload the stack before deleting the compacted tables. We can only
+ * delete the files after we closed them on Windows, so this needs to
+ * happen first.
+ */
err = reftable_stack_reload_maybe_reuse(st, first < last);
+ if (err < 0)
+ goto done;
- listp = delete_on_success;
- while (*listp) {
- if (strcmp(*listp, new_table_path.buf)) {
- unlink(*listp);
- }
- listp++;
+ /*
+ * Delete the old tables. They may still be in use by concurrent
+ * readers, so it is expected that unlinking tables may fail.
+ */
+ for (i = first; i <= last; i++) {
+ struct lock_file *table_lock = &table_locks[i - first];
+ char *table_path = get_locked_file_path(table_lock);
+ unlink(table_path);
+ free(table_path);
}
done:
- free_names(delete_on_success);
+ rollback_lock_file(&tables_list_lock);
+ for (i = first; table_locks && i <= last; i++)
+ rollback_lock_file(&table_locks[i - first]);
+ reftable_free(table_locks);
- if (subtable_locks) {
- listp = subtable_locks;
- while (*listp) {
- unlink(*listp);
- listp++;
- }
- free_names(subtable_locks);
- }
- if (lock_file_fd >= 0) {
- close(lock_file_fd);
- lock_file_fd = -1;
- }
- if (have_lock) {
- unlink(lock_file_name.buf);
- }
strbuf_release(&new_table_name);
strbuf_release(&new_table_path);
- strbuf_release(&ref_list_contents);
- strbuf_release(&temp_tab_file_name);
- strbuf_release(&lock_file_name);
+ strbuf_release(&new_table_temp_path);
+ strbuf_release(&tables_list_buf);
+ strbuf_release(&table_name);
return err;
}
diff --git a/reftable/system.h b/reftable/system.h
index 6b74a81514..5d8b6dede5 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -12,7 +12,9 @@ license that can be found in the LICENSE file or at
/* This header glues the reftable library to the rest of Git */
#include "git-compat-util.h"
+#include "lockfile.h"
#include "strbuf.h"
+#include "tempfile.h"
#include "hash-ll.h" /* hash ID, sizes.*/
#include "dir.h" /* remove_dir_recursively, for tests.*/
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 4/4] reftable/stack: register compacted tables as tempfiles
From: Patrick Steinhardt @ 2024-03-07 13:10 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Toon claes, Junio C Hamano
In-Reply-To: <cover.1709816483.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4510 bytes --]
We do not register tables resulting from stack compaction with the
tempfile API. Those tables will thus not be deleted in case Git gets
killed.
Refactor the code to register compacted tables as tempfiles.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 54 +++++++++++++++++++++++++++---------------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 977336b7d5..1ecf1b9751 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -827,51 +827,56 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
static int stack_compact_locked(struct reftable_stack *st,
size_t first, size_t last,
- struct strbuf *temp_tab,
- struct reftable_log_expiry_config *config)
+ struct reftable_log_expiry_config *config,
+ struct tempfile **tab_file_out)
{
struct strbuf next_name = STRBUF_INIT;
- int tab_fd = -1;
+ struct strbuf tab_file_path = STRBUF_INIT;
struct reftable_writer *wr = NULL;
- int err = 0;
+ struct tempfile *tab_file;
+ int tab_fd, err = 0;
format_name(&next_name,
reftable_reader_min_update_index(st->readers[first]),
reftable_reader_max_update_index(st->readers[last]));
+ stack_filename(&tab_file_path, st, next_name.buf);
+ strbuf_addstr(&tab_file_path, ".temp.XXXXXX");
- stack_filename(temp_tab, st, next_name.buf);
- strbuf_addstr(temp_tab, ".temp.XXXXXX");
+ tab_file = mks_tempfile(tab_file_path.buf);
+ if (!tab_file) {
+ err = REFTABLE_IO_ERROR;
+ goto done;
+ }
+ tab_fd = get_tempfile_fd(tab_file);
- tab_fd = mkstemp(temp_tab->buf);
if (st->config.default_permissions &&
- chmod(temp_tab->buf, st->config.default_permissions) < 0) {
+ chmod(get_tempfile_path(tab_file), st->config.default_permissions) < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
- wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);
-
+ wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,
+ &tab_fd, &st->config);
err = stack_write_compact(st, wr, first, last, config);
if (err < 0)
goto done;
+
err = reftable_writer_close(wr);
if (err < 0)
goto done;
- err = close(tab_fd);
- tab_fd = 0;
+ err = close_tempfile_gently(tab_file);
+ if (err < 0)
+ goto done;
+
+ *tab_file_out = tab_file;
+ tab_file = NULL;
done:
+ delete_tempfile(&tab_file);
reftable_writer_free(wr);
- if (tab_fd > 0) {
- close(tab_fd);
- tab_fd = 0;
- }
- if (err != 0 && temp_tab->len > 0) {
- unlink(temp_tab->buf);
- strbuf_release(temp_tab);
- }
strbuf_release(&next_name);
+ strbuf_release(&tab_file_path);
return err;
}
@@ -979,12 +984,12 @@ static int stack_compact_range(struct reftable_stack *st,
struct reftable_log_expiry_config *expiry)
{
struct strbuf tables_list_buf = STRBUF_INIT;
- struct strbuf new_table_temp_path = STRBUF_INIT;
struct strbuf new_table_name = STRBUF_INIT;
struct strbuf new_table_path = STRBUF_INIT;
struct strbuf table_name = STRBUF_INIT;
struct lock_file tables_list_lock = LOCK_INIT;
struct lock_file *table_locks = NULL;
+ struct tempfile *new_table = NULL;
int is_empty_table = 0, err = 0;
size_t i;
@@ -1059,7 +1064,7 @@ static int stack_compact_range(struct reftable_stack *st,
* these tables may end up with an empty new table in case tombstones
* end up cancelling out all refs in that range.
*/
- err = stack_compact_locked(st, first, last, &new_table_temp_path, expiry);
+ err = stack_compact_locked(st, first, last, expiry, &new_table);
if (err < 0) {
if (err != REFTABLE_EMPTY_TABLE_ERROR)
goto done;
@@ -1099,7 +1104,7 @@ static int stack_compact_range(struct reftable_stack *st,
strbuf_addstr(&new_table_name, ".ref");
stack_filename(&new_table_path, st, new_table_name.buf);
- err = rename(new_table_temp_path.buf, new_table_path.buf);
+ err = rename_tempfile(&new_table, new_table_path.buf);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
@@ -1166,9 +1171,10 @@ static int stack_compact_range(struct reftable_stack *st,
rollback_lock_file(&table_locks[i - first]);
reftable_free(table_locks);
+ delete_tempfile(&new_table);
strbuf_release(&new_table_name);
strbuf_release(&new_table_path);
- strbuf_release(&new_table_temp_path);
+
strbuf_release(&tables_list_buf);
strbuf_release(&table_name);
return err;
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Junio C Hamano @ 2024-03-07 13:22 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git
In-Reply-To: <ZelfOQ-XvvOpaOzi@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Mar 06, 2024 at 06:46:33PM -0500, Taylor Blau wrote:
>> On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote:
>> > Patrick Steinhardt <ps@pks.im> writes:
>> > > - `git config --get-urlmatch` -> `git config get-urlmatch`.
>> >
>> > ... is a Meh to me, personally. I'd not actively push it
>> > enthusiastically, but I'd passively accept its existence.
>>
>> I don't have strong feelings about this, but I wonder if `--urlmatch`
>> (or `--url-match`) might be an argument to the "get" mode of this
>> sub-command instead. Something like `git config get --urlmatch` feels
>> much more natural to me than `git config get-urlmatch`.
>
> Agreed. I allude to this somewhere in the patch series that I only see
> this as a first incremental step, and that some of the subcommands
> really should be converted to become options eventually. Specifically:
>
> - `git config get-urlmatch` -> `git config get --urlmatch`
>
> - `git config get-regexp` -> `git config get --regexp`
>
> - `git config get-all` -> `git config get --all`
>
> - `git config set-all` -> `git config set --all`
>
> - `git config unset-all` -> `git config unset --all`
>
> I didn't yet do it as part of this patch series because I didn't want to
> make functional changes at the same time (well, except of course for the
> introduction of subcommands). But if we all agree that this patch series
> is something we want _and_ that we don't want to have an intermediate
> step with the above somewhat weird subcommands then I would be happy to
> pile onto the series.
>
> I wouldn't think that keeping this intermediate step would be too bad
> though. While we would eventually omit the above subcommands, the infra
> to keep them around needs to stay anyway to support old syntax like
> `--get-all`. Thus, we can keep the subcommands themselves almost for
> free even if we eventually decide to hide them and replace them with
> flags.
The "more involved" changes along the lines Taylor suggested take
some thought in designing the end-user experience, so even if you do
not initially plan to support them in the implementation, it would
be better to know how the final command line should look like before
starting, so that we won't surprise users, if anything.
The trickies I anticipate offhand are:
* The urlmatch thing wants name and URL. In the new syntax, I
think the URL part should become an option parameter to the --url
option. E.g. a request with the current UI
$ git config --get-urlmatch SECTION.VARIABLE URL
would become
$ git config get --url=URL SECTION.VARIABLE
* The color thing takes the default value to be used when
unconfigured. That should also become an option parameter to the
--default option.
$ git config --get-color SECTION.VARIABLE DEFAULT
would become
$ git config get --type=color --default=DEFAULT SECTION.VARIABLE
Similarly for --tty=true/false used in --get-colorbool.
* Now, these extended ones with options introduce possibilities to
combine them in ways that weren't possible before. For example,
"--get" and "--get-all" are distinct, and there is no way to tell
"--get-urlmatch" to give all values that would match, but it is
plausible that we may want to handle
$ git config get --all --url=URL SECTION.VARIABLE
to do so. A more interesting one is the --default=DEFAULT
option. What we currently write
$ git config --get no.such.variable || echo 'default value'
can become
$ git config get --default='default value' no.such.variable
HTH.
^ permalink raw reply
* Re: [PATCH] Allow git-config to append a comment
From: Junio C Hamano @ 2024-03-07 13:27 UTC (permalink / raw)
To: Ralph Seichter; +Cc: Ralph Seichter, Ralph Seichter via GitGitGadget, git
In-Reply-To: <87plw6xld7.fsf@ra.horus-it.com>
Ralph Seichter <ralph@seichter.de> writes:
> Just as a brief interjection: I am sorry that my inexperience with Git's
> mailing-list based process caused me to leave out details which were
> discussed earlier, be it on GitHub or Discord. However, me not
> mentioning that I am aiming for single-line suffix comments only was due
> to simple forgetfulness, without an excuse behind it. My bad.
Don't feel too bad. It is not about discord vs mailing list vs
github "reviews on the web". It is about the end-product, the
commit log messages, what future developers will see in their "git
log" output.
> I think it best I flesh out the commit message before anything else, and
> then resubmit. That should bundle the necessary information.
Yup. Please do that.
Communicating with reviewers and other contributors is not the
primary goal of the review process. It is done to help us reach a
better end-product, the commit log messages that will help our future
developers and future selves.
Thanks.
^ permalink raw reply
* Re: [PATCH v4] tests: modernize the test script t0010-racy-git.sh
From: Christian Couder @ 2024-03-07 13:28 UTC (permalink / raw)
To: Aryan Gupta via GitGitGadget
Cc: git, Patrick Steinhardt [ ], Michal Suchánek [ ],
Jean-Noël AVILA [ ], Eric Sunshine, Aryan Gupta
In-Reply-To: <pull.1675.v4.git.1709716446874.gitgitgadget@gmail.com>
On Wed, Mar 6, 2024 at 10:46 AM Aryan Gupta via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Aryan Gupta <garyan447@gmail.com>
>
> Modernize the formatting of the test script to align with current
> standards and improve its overall readability.
>
> Signed-off-by: Aryan Gupta <garyan447@gmail.com>
> ---
> [GSOC][PATCH] Modernize a test script
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1675
>
> Range-diff vs v3:
>
> 1: 05ee9e8a458 = 1: 14c7137baea tests: modernize the test script t0010-racy-git.sh
This tells us that nothing changed in the patch since v3, so we can
only wonder why you sent this v4.
Did you fix some headers? Please explain.
> t/t0010-racy-git.sh | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
Otherwise, the patch looks good to me. Thanks.
^ permalink raw reply
* RE: [PATCH] Allow git-config to append a comment
From: rsbecker @ 2024-03-07 13:53 UTC (permalink / raw)
To: 'Junio C Hamano', 'Ralph Seichter'
Cc: 'Ralph Seichter via GitGitGadget', git
In-Reply-To: <xmqqplw6nsuz.fsf@gitster.g>
On Thursday, March 7, 2024 7:12 AM, Junio C Hamano wrote:
>Ralph Seichter <github@seichter.de> writes:
>
>>> If you are illustrating a sample input, please also explain what
>>> output it produces. What do the resulting lines in the config file
>>> look like after you run this command?
>>
>> The result of running the above command looks as follows:
>>
>> [safe]
>> directory = /home/alice/somerepo.git #I changed this. --A. Script
>
>That would have been a crucial piece of information to have in the proposed
log message, as limiting ourselves to a comment that is
>tucked after the same line as the value, things can become somewhat
simplified. We may not have to worry about deletion, even
>though the point about "we need to look at and typofix them with our
viewers and editors" still stands.
>
>By the way, you may or may not have noticed it, but my example deliberately
had a multi-line comment:
>
> $ git config --global --comment 'the reason why I added ~alice/
> is because ...' --add safe.directory /home/alice/somerepo.git
>
>How such a thing is handled also needs to be discussed in the proposed log
message, and perhaps in the documentation as well.
>
>> ... My patch only supports
>> single-line comments, and only as a suffix to newly added key-value
>> pairs. This is a deliberate design choice.
>
>Such design choices need to be described in the proposed log message to
help future developers who will be updating this feature,
>once it gets in.
>
>Thanks for writing quite a lot to answer _my_ questions, but these
questions are samples of things that future developers would
>wonder and ask about when they want to fix bugs in, enhance, or otherwise
modify the implementation of this "add comment"
>feature. They may even be working on adding other features to complement
the "add comment" feature, by designing support for
>viewing or typofixing existing comments. When they do so, it would help
them to know how this existing feature was expected to be
>used and how it would fit in a larger picture (which may not have yet
existed back when the feature was invented). Answering these
>anticipated questions is one of the greatest things that a commit log
message can do to help them.
>
>Thanks.
I am concerned about the compatibility of this series with the community.
While comments are permitted in .gitconfig files, I am not 100% sure that
all stakeholders, particularly those who parse .gitconfig files in their own
scripts outside of git - sure, it is their own responsibility, but this
might be unexpected. I worry that this might unintentionally introduce
incompatibilities into repository configurations. Is there broad consensus
to do this?
--Randall
^ permalink raw reply
* Re: [PATCH v4] tests: modernize the test script t0010-racy-git.sh
From: Aryan Gupta @ 2024-03-07 14:23 UTC (permalink / raw)
To: Christian Couder
Cc: Aryan Gupta via GitGitGadget, git, Patrick Steinhardt [ ],
Michal Suchánek [ ], Jean-Noël AVILA [ ], Eric Sunshine
In-Reply-To: <CAP8UFD31udQB2e=+G-LpCevuS+JxQdWqwaq=5qvGEn21595faQ@mail.gmail.com>
On Thu, Mar 7, 2024 at 2:28 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Mar 6, 2024 at 10:46 AM Aryan Gupta via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Aryan Gupta <garyan447@gmail.com>
> >
> > Modernize the formatting of the test script to align with current
> > standards and improve its overall readability.
> >
> > Signed-off-by: Aryan Gupta <garyan447@gmail.com>
> > ---
> > [GSOC][PATCH] Modernize a test script
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v4
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v4
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1675
> >
> > Range-diff vs v3:
> >
> > 1: 05ee9e8a458 = 1: 14c7137baea tests: modernize the test script t0010-racy-git.sh
>
> This tells us that nothing changed in the patch since v3, so we can
> only wonder why you sent this v4.
>
> Did you fix some headers? Please explain.
>
Hey. Sorry for making a lot of mistakes in my emails.
I thought that there were some bugs in GGG due to which it sent some
headers which were not syntactically correct. So I tried sending it again.
And that was the whole purpose of this.
> > t/t0010-racy-git.sh | 31 +++++++++++++++----------------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
>
> Otherwise, the patch looks good to me. Thanks.
^ 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