* Re: [PATCH] column: disallow negative padding
From: Kristoffer Haugsbakk @ 2024-02-09 18:26 UTC (permalink / raw)
To: git; +Cc: Tiago Pascoal
In-Reply-To: <76688ed2cc20031d70823d9f5d214f42b3bd1409.1707501064.git.code@khaugsbakk.name>
I forgot tests.
--
Kristoffer
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Karthik Nayak @ 2024-02-09 18:27 UTC (permalink / raw)
To: Junio C Hamano, Patrick Steinhardt; +Cc: Phillip Wood, phillip.wood, git
In-Reply-To: <xmqq8r3timnr.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> Depending on the answer, I think we can go one of two ways:
>>
>> - Accept the diverging behaviour and add `--include-all-refs`. The
>> "files" backend does a best effort and only includes root refs, the
>> "reftable" backend lists all refs.
>>
>> - Double down on the fact that refs must either be pseudo refs or
>> start with "refs/" and treat any ref that doesn't fit this bill as
>> corrupted. The consequence here would be that we instead go with
>> `--include-root-refs` that can be implemented the same for both
>> backends. In addition, we add checks to git-fsck(1) to surface and
>> flag refs with bogus names for the "reftable" backend.
>>
>> While I seem to have convinced you that `--include-all-refs` might not
>> be a bad idea after all, you have convinced me by now that the second
>> option would be preferable. I'd be okay with either of these options as
>> both of them address the issue at hand.
>
> For now my tentative preference is the latter. If ref/head/foo is
> an end-user mistake with one backend, it is cleaner if it is
> considered a mistake with other backends.
>
> Doesn't our ref enumeration/iteration API have "include broken ones
> as well" bit? I wonder if this issue becomes easier to solve by
> (re|ab)using that bit.
I'll then go ahead with point 2 then.
I'll modify my patch series for now to fit in and will follow up "checks
to git-fsck(1) to surface and flag refs with bogus names for the
"reftable" backend" in a follow up series.
- Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Junio C Hamano @ 2024-02-09 19:56 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Marcus Tillmanns, git@vger.kernel.org, Phillip Wood
In-Reply-To: <d59a0e25-81c4-4ecd-826e-ef4b23423575@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Fri, Feb 9, 2024, at 18:30, Junio C Hamano wrote:
>> So, now, let's be productive. When somebody who does not know much
>> about Git tries to commit without configuring anything and hits the
>> error, what is a more appropriate message to guide who does not know
>> what he or she does not know?
>>
>> The user claims that "committer identity unknown, please tell me who
>> you are" were not helpful enough. Would it make it more helpful if
>> we append how to "tell who they are" after that message, perhaps
>> with "git config" on user.email and user.name variables, or
>> something?
>>
>> Or do we need three-way switch that does
>>
>> if (neither is known) {
>> printf("neither author or committer is known");
>> } else if (author is known but committer is not known) {
>> printf("author is known but committer is not"):
>> } else if (author is not known but committer is known) {
>> printf("committer is known but author is not"):
>> } else {
>> return happy;
>> }
>>
>> printf("please tell us who you are...");
>>
>> perhaps?
>
> I think a three-way switch looks good. With the amendment that it steers
> you towards `user.*` instead of setting both `author.*` and
> `committer.*`.
>
> Something like
>
> • Author is set, not committer
> • Message: author is set but not committer: you might want to set
> *user* instead (prints suggested config)
>
> I can try to make a patch later.
Wait. I didn't realize this when I wrote the message you are
responding to, but we *do* already suggest settig user.* variables.
If the user chose to ignore that, then there isn't much we can do to
help, is there?
Puzzled, but I'll stop here.
^ permalink raw reply
* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Kyle Lippincott @ 2024-02-09 19:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <5fd95ae0-cd50-ddc4-5095-ee953c2640b3@gmx.de>
On Fri, Feb 9, 2024 at 12:06 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Kyle,
>
> On Mon, 5 Feb 2024, Kyle Lippincott wrote:
>
> > On Mon, Feb 5, 2024 at 6:36 AM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
> > > diff --git a/builtin/notes.c b/builtin/notes.c
> > > index e65cae0bcf7..caf20fd5bdd 100644
> > > --- a/builtin/notes.c
> > > +++ b/builtin/notes.c
> > > @@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> > > struct strbuf buf = STRBUF_INIT;
> > > char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);
> > >
> > > - if (prev_buf && size)
> > > + if (!prev_buf)
> > > + die(_("unable to read %s"), oid_to_hex(note));
> >
> > This changes the behavior of this function. Previously, it would not
> > add prev_buf output, but still succeed. This now dies.
>
> It does change behavior. The previous behavior looked up the note OID,
> then tried to read it, and if it was missing just pretended that there had
> not been a note.
>
> I'm not quite sure whether we should keep that behavior, as it is unclear
> in which scenarios it would be desirable to paper over missing objects.
>
> In GitGitGadget, I am a heavy user of notes and it wouldn't do any good to
> have this behavior: It would lose information.
>
> And even in scenarios where the `notes` ref is fetch shallowly, I would
> expect all of the actual notes blobs to be present, and I would _want_ the
> `git note edit ...` command to error out when that blob is not found.
>
> Does that reasoning make sense to you?
Sounds good, thanks for talking through it with me.
>
> Ciao,
> Johannes
^ permalink raw reply
* [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()
From: René Scharfe @ 2024-02-09 20:36 UTC (permalink / raw)
To: Git List
Cc: Chandra Pratap, Chandra Pratap, Jeff King, Kyle Lippincott,
John Cai, Junio C Hamano
Use the public function find_commit_header() instead of find_header() to
simplify the code. This is possible and safe because we're operating on
a strbuf, which is always NUL-terminated, so there is no risk of running
over the end of the buffer. It cannot contain NUL within the buffer, as
it is built using strbuf_addstr(), only.
The string comparison becomes more complicated because we need to check
for NUL explicitly after comparing the length-limited option, but on the
flip side we don't need to clean up allocations or track the remaining
buffer length.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/receive-pack.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e36b1d619f..8ebb3a872f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -718,35 +718,29 @@ static const char *check_nonce(const char *buf, size_t len)
static int check_cert_push_options(const struct string_list *push_options)
{
const char *buf = push_cert.buf;
- int len = push_cert.len;
- char *option;
- const char *next_line;
+ const char *option;
+ size_t optionlen;
int options_seen = 0;
int retval = 1;
- if (!len)
+ if (!*buf)
return 1;
- while ((option = find_header(buf, len, "push-option", &next_line))) {
- len -= (next_line - buf);
- buf = next_line;
+ while ((option = find_commit_header(buf, "push-option", &optionlen))) {
+ buf = option + optionlen + 1;
options_seen++;
if (options_seen > push_options->nr
- || strcmp(option,
- push_options->items[options_seen - 1].string)) {
- retval = 0;
- goto leave;
- }
- free(option);
+ || strncmp(push_options->items[options_seen - 1].string,
+ option, optionlen)
+ || push_options->items[options_seen - 1].string[optionlen])
+ return 0;
}
if (options_seen != push_options->nr)
retval = 0;
-leave:
- free(option);
return retval;
}
--
2.43.0
^ permalink raw reply related
* [PATCH 2/2] receive-pack: use find_commit_header() in check_nonce()
From: René Scharfe @ 2024-02-09 20:41 UTC (permalink / raw)
To: Git List
Cc: Chandra Pratap, Chandra Pratap, Jeff King, Kyle Lippincott,
John Cai, Junio C Hamano
In-Reply-To: <ff0db7e3-abce-44ea-a1e3-16e1fdaf4c75@web.de>
Use the public function find_commit_header() and remove find_header(),
as it becomes unused. This is safe and appropriate because we pass the
NUL-terminated payload buffer to check_nonce() instead of its start and
length. The underlying strbuf push_cert cannot contain NULs, as it is
built using strbuf_addstr(), only.
We no longer need to call strlen(), as find_commit_header() returns the
length of nonce already.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Formatted with -U5 for easier review.
Removing find_header_mem(), which basically becomes unused, left as an
exercise for the reader. ;)
builtin/receive-pack.c | 25 +++++--------------------
1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8ebb3a872f..dbee508775 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -591,25 +591,10 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
/* RFC 2104 5. HMAC-SHA1 or HMAC-SHA256 */
strbuf_addf(&buf, "%"PRItime"-%.*s", stamp, (int)the_hash_algo->hexsz, hash_to_hex(hash));
return strbuf_detach(&buf, NULL);
}
-static char *find_header(const char *msg, size_t len, const char *key,
- const char **next_line)
-{
- size_t out_len;
- const char *val = find_header_mem(msg, len, key, &out_len);
-
- if (!val)
- return NULL;
-
- if (next_line)
- *next_line = val + out_len + 1;
-
- return xmemdupz(val, out_len);
-}
-
/*
* Return zero if a and b are equal up to n bytes and nonzero if they are not.
* This operation is guaranteed to run in constant time to avoid leaking data.
*/
static int constant_memequal(const char *a, const char *b, size_t n)
@@ -620,17 +605,18 @@ static int constant_memequal(const char *a, const char *b, size_t n)
for (i = 0; i < n; i++)
res |= a[i] ^ b[i];
return res;
}
-static const char *check_nonce(const char *buf, size_t len)
+static const char *check_nonce(const char *buf)
{
- char *nonce = find_header(buf, len, "nonce", NULL);
+ size_t noncelen;
+ const char *found = find_commit_header(buf, "nonce", &noncelen);
+ char *nonce = found ? xmemdupz(found, noncelen) : NULL;
timestamp_t stamp, ostamp;
char *bohmac, *expect = NULL;
const char *retval = NONCE_BAD;
- size_t noncelen;
if (!nonce) {
retval = NONCE_MISSING;
goto leave;
} else if (!push_cert_nonce) {
@@ -668,11 +654,10 @@ static const char *check_nonce(const char *buf, size_t len)
if (bohmac == nonce || bohmac[0] != '-') {
retval = NONCE_BAD;
goto leave;
}
- noncelen = strlen(nonce);
expect = prepare_push_cert_nonce(service_dir, stamp);
if (noncelen != strlen(expect)) {
/* This is not even the right size. */
retval = NONCE_BAD;
goto leave;
@@ -765,11 +750,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)
sigcheck.payload = xmemdupz(push_cert.buf, bogs);
sigcheck.payload_len = bogs;
check_signature(&sigcheck, push_cert.buf + bogs,
push_cert.len - bogs);
- nonce_status = check_nonce(push_cert.buf, bogs);
+ nonce_status = check_nonce(sigcheck.payload);
}
if (!is_null_oid(&push_cert_oid)) {
strvec_pushf(&proc->env, "GIT_PUSH_CERT=%s",
oid_to_hex(&push_cert_oid));
strvec_pushf(&proc->env, "GIT_PUSH_CERT_SIGNER=%s",
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4 19/28] trailer: make trailer_info struct private
From: Junio C Hamano @ 2024-02-09 21:53 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <f5f0d06613f2701af9bd3a1a9274aae232e03d8f.1707196348.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This patch believes that the benefits outweight the advantages for
> designing APIs, as explained below.
"outweigh the disadvantages"?
^ permalink raw reply
* Re: [PATCH v4 16/28] trailer: finish formatting unification
From: Junio C Hamano @ 2024-02-09 21:53 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <31725832224e3d6b14066af8a87eaf4ab589179e.1707196348.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Rename format_trailer_info() to format_trailers(). Finally, both
> interpret-trailers and format_trailers_from_commit() can call
> "format_trailers()"!
>
> Update the comment in <trailer.h> to remove the (now obsolete) caveats
> about format_trailers_from_commit().
Nice.
^ permalink raw reply
* Re: [PATCH v4 12/28] format_trailer_info(): append newline for non-trailer lines
From: Junio C Hamano @ 2024-02-09 21:53 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <a72eca301f7f9016ef3a8063f79790ce00f41ffe.1707196348.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> And before we made the transition to use trailer_item objects for it,
> format_trailer_info() called parse_trailer() (which trims newlines) for
> trailer lines but did _not_ call parse_trailer() for non-trailer lines.
> So for trailer lines it had to add back the trimmed newline like this
>
> if (!opts->separator)
> strbuf_addch(out, '\n');
>
> But for non-trailer lines it didn't have to add back the newline because
> it could just reuse same string in the "trailers" string array (which
> again, already included the trailing newline).
>
> Now that format_trailer_info() uses trailer_item objects for all cases,
> it can't rely on "trailers" string array anymore. And so it must be
> taught to add a newline back when printing non-trailer lines, just like
> it already does for trailer lines. Do so now.
Very nicely explained.
> The test suite passes again, so format_trailer_info() is in better shape
> supersede format_trailers(), which we'll do in the next patch.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> trailer.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index 12cae5b73d2..0774a544c4f 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1125,9 +1125,10 @@ static void format_trailer_info(const struct process_trailer_options *opts,
> strbuf_addbuf(out, opts->separator);
> }
> strbuf_addstr(out, item->value);
> - if (opts->separator) {
> + if (opts->separator)
> strbuf_rtrim(out);
> - }
> + else
> + strbuf_addch(out, '\n');
> }
> }
> }
^ permalink raw reply
* Re: [PATCH v4 11/28] format_trailer_info(): drop redundant unfold_value()
From: Junio C Hamano @ 2024-02-09 21:54 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <457f2a839d5da9da225e842275bbf8b15f194f1f.1707196348.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> This is another preparatory refactor to unify the trailer formatters.
>
> In the last patch we made format_trailer_info() use trailer_item objects
> instead of the "trailers" string array. This means that the call to
> unfold_value() here is redundant because the trailer_item objects are
> already unfolded in parse_trailers() which is a dependency of our
> caller, format_trailers_from_commit().
>
> Remove the redundant call.
OK. The previous step had this hunk:
- parse_trailer(&tok, &val, NULL, trailer, separator_pos);
if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
if (opts->unfold)
unfold_value(&val);
where parse_trailers() already has a call to parse_trailer()
followed by a call to unfold_value(), so in a sense, switching
to use the result of calling parse_trailers() by the caller instead
of duplicating our own parsing in format_trailer_info() that started
at step [09/28] made both parse_trailer() call (removed in step [10/28])
and unfold_value() call (removed in this step [11/28]).
So it would have also made sense if this were done as part of
[10/28], but it is also OK to keep them separated.
In either way, breaking the transition into these steps does make
them easier to follow.
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> trailer.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index 6333dfe1c11..12cae5b73d2 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1102,9 +1102,6 @@ static void format_trailer_info(const struct process_trailer_options *opts,
> strbuf_addstr(&val, item->value);
>
> if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
> - if (opts->unfold)
> - unfold_value(&val);
> -
> if (opts->separator && out->len != origlen)
> strbuf_addbuf(out, opts->separator);
> if (!opts->value_only)
^ permalink raw reply
* Re: [PATCH v4 10/28] format_trailer_info(): use trailer_item objects
From: Junio C Hamano @ 2024-02-09 21:53 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <f5b7ba08aa7c80a3bd5bcbf5563eac8896fe7054.1707196348.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> This is another preparatory refactor to unify the trailer formatters.
>
> Make format_trailer_info() operate on trailer_item objects, not the raw
> string array.
>
> This breaks t4205 and t6300. We will continue to make improvements until
> the test suite passes again, ultimately renaming format_trailer_info()
> to format_trailers(), at which point the unification of these formatters
> will be complete.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> trailer.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
I would have expected a (tentative) flip from test_expect_success to
test_expect_failure in the affected tests, to illustrate what behaviour
change this step introduces and why.
But the huge single step in the previous round broken out into
smaller steps like this round makes them much easier to follow.
Thanks.
> diff --git a/trailer.c b/trailer.c
> index e6665c99cc3..6333dfe1c11 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1086,21 +1086,21 @@ void trailer_info_release(struct trailer_info *info)
> }
>
> static void format_trailer_info(const struct process_trailer_options *opts,
> - const struct trailer_info *info,
> + struct list_head *trailers,
> struct strbuf *out)
> {
> size_t origlen = out->len;
> - size_t i;
> -
> - for (i = 0; i < info->trailer_nr; i++) {
> - char *trailer = info->trailers[i];
> - ssize_t separator_pos = find_separator(trailer, separators);
> + struct list_head *pos;
> + struct trailer_item *item;
>
> - if (separator_pos >= 1) {
> + list_for_each(pos, trailers) {
> + item = list_entry(pos, struct trailer_item, list);
> + if (item->token) {
> struct strbuf tok = STRBUF_INIT;
> struct strbuf val = STRBUF_INIT;
> + strbuf_addstr(&tok, item->token);
> + strbuf_addstr(&val, item->value);
>
> - parse_trailer(&tok, &val, NULL, trailer, separator_pos);
> if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
> if (opts->unfold)
> unfold_value(&val);
> @@ -1127,13 +1127,12 @@ static void format_trailer_info(const struct process_trailer_options *opts,
> if (opts->separator && out->len != origlen) {
> strbuf_addbuf(out, opts->separator);
> }
> - strbuf_addstr(out, trailer);
> + strbuf_addstr(out, item->value);
> if (opts->separator) {
> strbuf_rtrim(out);
> }
> }
> }
> -
> }
>
> void format_trailers_from_commit(const struct process_trailer_options *opts,
> @@ -1152,7 +1151,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
> strbuf_add(out, msg + info.trailer_block_start,
> info.trailer_block_end - info.trailer_block_start);
> } else
> - format_trailer_info(opts, &info, out);
> + format_trailer_info(opts, &trailers, out);
>
> free_trailers(&trailers);
> trailer_info_release(&info);
^ permalink raw reply
* Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()
From: Junio C Hamano @ 2024-02-09 22:11 UTC (permalink / raw)
To: René Scharfe
Cc: Git List, Chandra Pratap, Chandra Pratap, Jeff King,
Kyle Lippincott, John Cai
In-Reply-To: <ff0db7e3-abce-44ea-a1e3-16e1fdaf4c75@web.de>
René Scharfe <l.s.r@web.de> writes:
> The string comparison becomes more complicated because we need to check
> for NUL explicitly after comparing the length-limited option, but on the
> flip side we don't need to clean up allocations or track the remaining
> buffer length.
Yeah, the strncmp() followed by the termination check indeed is
trickier but not having to worry about allocation is nice.
> if (options_seen > push_options->nr
> - || strcmp(option,
> - push_options->items[options_seen - 1].string)) {
We used to allocate option[] with NUL termination, but ...
> - retval = 0;
> - goto leave;
> - }
> - free(option);
> + || strncmp(push_options->items[options_seen - 1].string,
> + option, optionlen)
> + || push_options->items[options_seen - 1].string[optionlen])
... now option[] is a borrowed memory, option[optionlen] would have
been NUL if we were allocating. So to see if the last-seen string[]
is different from option[], we have to see that they match up to
optionlen and the last-seen string[] ends there. Trickier than
before, but is correct.
> + return 0;
> }
>
> if (options_seen != push_options->nr)
> retval = 0;
>
> -leave:
> - free(option);
> return retval;
> }
>
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH 2/2] receive-pack: use find_commit_header() in check_nonce()
From: Junio C Hamano @ 2024-02-09 22:18 UTC (permalink / raw)
To: René Scharfe
Cc: Git List, Chandra Pratap, Chandra Pratap, Jeff King,
Kyle Lippincott, John Cai
In-Reply-To: <8b350cae-2180-4ac7-a911-d40043576445@web.de>
René Scharfe <l.s.r@web.de> writes:
> @@ -620,17 +605,18 @@ static int constant_memequal(const char *a, const char *b, size_t n)
> for (i = 0; i < n; i++)
> res |= a[i] ^ b[i];
> return res;
> }
>
> -static const char *check_nonce(const char *buf, size_t len)
> +static const char *check_nonce(const char *buf)
> {
> - char *nonce = find_header(buf, len, "nonce", NULL);
> + size_t noncelen;
> + const char *found = find_commit_header(buf, "nonce", &noncelen);
> + char *nonce = found ? xmemdupz(found, noncelen) : NULL;
OK, the changes to this function are all quite trivially correct.
> timestamp_t stamp, ostamp;
> char *bohmac, *expect = NULL;
> const char *retval = NONCE_BAD;
> - size_t noncelen;
>
> if (!nonce) {
> retval = NONCE_MISSING;
> goto leave;
> } else if (!push_cert_nonce) {
> @@ -668,11 +654,10 @@ static const char *check_nonce(const char *buf, size_t len)
> if (bohmac == nonce || bohmac[0] != '-') {
> retval = NONCE_BAD;
> goto leave;
> }
>
> - noncelen = strlen(nonce);
> expect = prepare_push_cert_nonce(service_dir, stamp);
> if (noncelen != strlen(expect)) {
> /* This is not even the right size. */
> retval = NONCE_BAD;
> goto leave;
> @@ -765,11 +750,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)
> sigcheck.payload = xmemdupz(push_cert.buf, bogs);
> sigcheck.payload_len = bogs;
> check_signature(&sigcheck, push_cert.buf + bogs,
> push_cert.len - bogs);
>
> - nonce_status = check_nonce(push_cert.buf, bogs);
> + nonce_status = check_nonce(sigcheck.payload);
Hmph. sigc->payload is used as a read-only member in
check_signature(), and the xmemdupz() copy we made earlier is
readily available as a replacement for the counted (push_cert.buf,
bogs) string. Very nice finding.
> }
> if (!is_null_oid(&push_cert_oid)) {
> strvec_pushf(&proc->env, "GIT_PUSH_CERT=%s",
> oid_to_hex(&push_cert_oid));
> strvec_pushf(&proc->env, "GIT_PUSH_CERT_SIGNER=%s",
> --
> 2.43.0
^ permalink raw reply
* [PATCH 1/1] imap-send: include strbuf.h
From: Christian Hesse @ 2024-02-09 22:26 UTC (permalink / raw)
To: Git Mailing List; +Cc: Christian Hesse
From: Christian Hesse <mail@eworm.de>
We had this fixed in 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2,
and it broke again in eea0e59ffbed6e33d171ace5be13cde9faa41639.
We make liberal use of the strbuf API functions and types, but the
inclusion of <strbuf.h> comes indirectly by including <http.h>,
which does not happen if you build with NO_CURL.
This time make the include conditional... Does that prevent from
loosing it again?
Signed-off-by: Christian Hesse <mail@eworm.de>
---
imap-send.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/imap-send.c b/imap-send.c
index f2e1947e63..cae494c663 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -34,6 +34,8 @@ typedef void *SSL;
#endif
#ifdef USE_CURL_FOR_IMAP_SEND
#include "http.h"
+#else
+#include "strbuf.h"
#endif
#if defined(USE_CURL_FOR_IMAP_SEND)
--
2.43.1
^ permalink raw reply related
* Re: [PATCH 1/1] imap-send: include strbuf.h
From: Junio C Hamano @ 2024-02-09 22:42 UTC (permalink / raw)
To: Christian Hesse, Philippe Blain; +Cc: Git Mailing List, Christian Hesse
In-Reply-To: <20240209222622.102208-1-list@eworm.de>
Christian Hesse <list@eworm.de> writes:
> From: Christian Hesse <mail@eworm.de>
>
> We had this fixed in 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2,
> and it broke again in eea0e59ffbed6e33d171ace5be13cde9faa41639.
Thanks, already reported and fixed, I believe?
^ permalink raw reply
* Re: [PATCH 1/1] imap-send: include strbuf.h
From: Junio C Hamano @ 2024-02-09 22:54 UTC (permalink / raw)
To: Christian Hesse; +Cc: Philippe Blain, Git Mailing List, Christian Hesse
In-Reply-To: <xmqqil2xcl9e.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Christian Hesse <list@eworm.de> writes:
>
>> From: Christian Hesse <mail@eworm.de>
>>
>> We had this fixed in 3307f7dde2ae8f5281d0782f7291a073c9b1cdc2,
>> and it broke again in eea0e59ffbed6e33d171ace5be13cde9faa41639.
>
> Thanks, already reported and fixed, I believe?
Oops, missing link:
https://lore.kernel.org/git/pull.1664.git.git.1706833113569.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Junio C Hamano @ 2024-02-09 23:54 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Elijah Newren, Michael Lohmann, git, phillip.wood123
In-Reply-To: <dfb582cf-b1e4-414d-bfe1-0f93d910ec54@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> Also, what about cases where users do a cherry-pick in the middle of a
>> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What
>> then?
>
> Good point! IMO, REBASE_HEAD should have lower precedence than all the
> other *_HEADs. It would mean to reorder the entries:
>
> static const char *const other_head[] = {
> "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
> };
>
> (and perhaps adjust the error message, too).
I've tweaked this change into the commit.
Thanks, all.
------- >8 ------------- >8 ------------- >8 ------------- >8 -------
From: Michael Lohmann <mi.al.lohmann@gmail.com>
Date: Wed, 17 Jan 2024 09:14:05 +0100
Subject: [PATCH] revision: implement `git log --merge` also for
rebase/cherry_pick/revert
Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
[jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
revision.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/revision.c b/revision.c
index aa4c4dc778..36dc2f94f7 100644
--- a/revision.c
+++ b/revision.c
@@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,
}
}
+static const char *lookup_other_head(struct object_id *oid)
+{
+ int i;
+ static const char *const other_head[] = {
+ "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
+ };
+
+ for (i = 0; i < ARRAY_SIZE(other_head); i++)
+ if (!read_ref_full(other_head[i],
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ oid, NULL)) {
+ if (is_null_oid(oid))
+ die("%s is a symbolic ref???", other_head[i]);
+ return other_head[i];
+ }
+
+ die("--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?");
+}
+
static void prepare_show_merge(struct rev_info *revs)
{
struct commit_list *bases;
struct commit *head, *other;
struct object_id oid;
+ const char *other_name;
const char **prune = NULL;
int i, prune_num = 1; /* counting terminating NULL */
struct index_state *istate = revs->repo->index;
@@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs)
if (repo_get_oid(the_repository, "HEAD", &oid))
die("--merge without HEAD?");
head = lookup_commit_or_die(&oid, "HEAD");
- if (read_ref_full("MERGE_HEAD",
- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- &oid, NULL))
- die("--merge without MERGE_HEAD?");
- if (is_null_oid(&oid))
- die("MERGE_HEAD is a symbolic ref???");
- other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+ other_name = lookup_other_head(&oid);
+ other = lookup_commit_or_die(&oid, other_name);
add_pending_object(revs, &head->object, "HEAD");
- add_pending_object(revs, &other->object, "MERGE_HEAD");
+ add_pending_object(revs, &other->object, other_name);
bases = repo_get_merge_bases(the_repository, head, other);
add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
--
2.44.0-rc0
^ permalink raw reply related
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Brian Lyles @ 2024-02-10 4:34 UTC (permalink / raw)
To: phillip.wood; +Cc: git, me, newren, gitster
In-Reply-To: <8ff4650c-f84f-41bd-a46c-3b845ff29b70@gmail.com>
Hi Phillip
On Thu, Feb 1, 2024 at 4:57 AM Phillip Wood <phillip.wood123@gmail.com>
wrote:
> That sounds like a good strategy. I'm wondering if we should not change
> the behavior of `--keep-redundant-commits` to avoid breaking existing
> users but have `--empty=keep|drop` not imply `--allow-empty`. What ever
> we do we'll annoy someone. It is confusing to have subtly different
> behaviors for `--keep-redundant-commits` and `--empty=keep` but it
> avoids breaking existing users. If we change `--keep-redundant-commits`
> we potentially upset existing users but we don't confuse others with the
> subtle difference between the two.
I am starting to come around to this approach for this particular series
just to avoid anything potentially controversial holding it up.
>>> Do you have a practical example of where you want to keep the commits
>>> that become empty but not the ones that start empty? I agree there is a
>>> distinction but I think the common case is that the user wants to keep
>>> both types of empty commit or none. I'm not against giving the user the
>>> option to keep one or the other if it is useful but I'm wary of changing
>>> the default.
>>
>> That practical example is documented in the initial discussion[1], which
>> I should have ought to have linked in a cover letter for this series
>> (and will do so in v2). I'll avoid copying the details here, but we'd
>> very much like to be able to programmatically drop the commits that
>> become empty when doing the automated cherry-pick described there.
>>
>> [1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com/
>
> Maybe I've missed something but that seems to be an argument for
> implementing `--empty=drop` which is completely reasonable but doesn't
> explain why someone using `--keep-redundant-commits` would want to keep
> the commits that become empty while dropping the commits that start empty.
Nope, you didn't miss something -- I just didn't read properly.
I don't have a concrete example here, but the behavior *feels* quite odd
to me. But I suppose that's not a good enough reason to make a breaking
change.
--
Thank you,
Brian Lyles
^ permalink raw reply
* git status became very slow after upgrading git
From: Vijay Raghavan Aravamudhan @ 2024-02-10 4:49 UTC (permalink / raw)
To: git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
1. brew update which pulled in latest version of git
2. git status in a repository (without submodules)
What did you expect to happen? (Expected behavior)
git status should have been fast
What happened instead? (Actual behavior)
git status takes almost 5s to complete.
What's different between what you expected and what actually happened?
Time for the status reporting
Anything else you want to add:
i have the `status.submoduleSummary` set to `true` in my global git config
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.43.1
cpu: arm64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 23.4.0 Darwin Kernel Version 23.4.0: Sat Jan 27 14:29:57
PST 2024; root:xnu-10063.100.633~14/RELEASE_ARM64_T8112 arm64
compiler info: clang: 15.0.0 (clang-1500.1.0.2.5)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh
[Enabled Hooks]
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Brian Lyles @ 2024-02-10 5:50 UTC (permalink / raw)
To: phillip.wood; +Cc: git, me, newren, gitster
In-Reply-To: <b5213705-4cd6-40ef-8c5f-32b214534b8b@gmail.com>
On Mon, Jan 29, 2024 at 4:55 AM Phillip Wood <phillip.wood123@gmail.com>
wrote:
>>>> I'm not sure that is a good idea as it is hiding an error that we didn't
>>>> hit before because we returned early.
>>>
>>> I think you're right -- Previously the error could not have been hit,
>>> but now it can. An error is still an error, and we should handle it
>>> regardless of how `allow_empty` was set. I'll address this in v2 by
>>> simply returning the error.
>>
>> As I dig into this more, I'm noticing that this may have unintended side
>> effects that I'm unsure of. After making this change, I noticed a couple
>> of failures in the cherry-pick test suite. The others may be a knock-on
>> of this initial failure:
>>
>> expecting success of 3501.8 'cherry-pick on unborn branch':
>> git checkout --orphan unborn &&
>> git rm --cached -r . &&
>> rm -rf * &&
>> git cherry-pick initial &&
>> git diff --quiet initial &&
>> test_cmp_rev ! initial HEAD
>>
>> A extra_file
>> Switched to a new branch 'unborn'
>> rm 'extra_file'
>> rm 'spoo'
>> error: could not resolve HEAD commit
>> fatal: cherry-pick failed
>> not ok 8 - cherry-pick on unborn branch
>> #
>> # git checkout --orphan unborn &&
>> # git rm --cached -r . &&
>> # rm -rf * &&
>> # git cherry-pick initial &&
>> # git diff --quiet initial &&
>> # test_cmp_rev ! initial HEAD
>> #
>>
>> It looks like this is caused specifically by not hiding the error from
>> `index_unchanged`
>
> Oh dear, that's a pain. I haven't checked but suspect we already hit
> this when running
>
> git cherry-pick --allow-empty
>
> on an orphan checkout. In do_pick_commit() we treat an error reading
> HEAD as an unborn branch so I think we could do the same here. If the
> branch is unborn then we can use the_hash_algo->empty_tree as the tree
> to compare to.
You're correct that `git cherry-pick --allow-empty` on an unborn branch
hits the same error today, and that using `empty_tree` seems to resolve
it. Thank you for this tip, I'll include a fix and corresponding test as
a separate commit in v2.
--
Thank you,
Brian Lyles
^ permalink raw reply
* Re: Hello question on Git for Windows 2.43.0 - GUID and/or SWID tag for this title
From: Matthias Aßhauer @ 2024-02-10 7:40 UTC (permalink / raw)
To: Christian Castro; +Cc: git@vger.kernel.org
In-Reply-To: <LV8PR13MB6560538530A2A7D1C1FD89C19C422@LV8PR13MB6560.namprd13.prod.outlook.com>
On Fri, 2 Feb 2024, Christian Castro wrote:
> Hello Git for Windows,
>
>
> I have a question on the GUID and/or SWID tag for Git for Windows 2.43.0.
>
> Can you tell me where in the product the GUID and/or SWID tag would be stored in Windows for Git for Windows 2.43.0?
> Our scanning software has detected both 2.39.2 and 2.43.0 on the same Windows but this is not so, only 2.43.0 is installed.
> This was an upgrade from 2.39.2 though so not sure if that is messed up the results somehow.
>
> I've looked in C:\ProgramData and there are no regid folders not regid.xyz files exist for this product (for the SWID tag).
>
> The Windows registry also does not have a GUID information for this product located under:
> HKLM\Software\GitForWindows
> HKLM\Software\Microsoft\Windows\CurrentVersion\Uninstall\Git_is1
>
>
> If you've included a GUID and/or SWID tag for your product somewhere in the installation process, can you please tell me where these are stored under Windows so I can fix? I'd appreciate it.
>
Neither of those things really exist in Git for Windows. Well there are
GUIDs, but no GUID that is used in the way you'd expect from an MSI
installer.
The Git for Windows installer is an innosetup based EXE installer.
Innosetup doesn't have these concepts. There have been efforts to
introduce an MSI installer over the years, but they've all fizzled out.
As for SWID tags, you're the first person to mention them in almost 15
years of ISO/IEC 19770-2 existing.
>
> Thank you,
>
> Christian Castro
> Sr. Systems Administrator
> Office: 301-628-3551
> DLHcorp.com
>
>
>
> ****WARNING**** This email message (including any attachments) are to be treated as confidential/proprietary and may contain copyrighted or other legally protected information. It is intended only for the addressee(s) identified above. If you are not the addressee(s), or an employee or agent of the addressee(s), please note that any dissemination, distribution, or copying of this communication is strictly prohibited. If you have received this information in error, please destroy the information and notify the sender of the error. Thank you.
>
^ permalink raw reply
* [PATCH] use xstrncmpz()
From: René Scharfe @ 2024-02-10 7:43 UTC (permalink / raw)
To: Git List
Add and apply a semantic patch for calling xstrncmpz() to compare a
NUL-terminated string with a buffer of a known length instead of using
strncmp() and checking the terminating NUL explicitly. This simplifies
callers by reducing code duplication.
I had to adjust remote.c manually because Coccinelle inexplicably
changed the indent of the else branches.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
archive-tar.c | 2 +-
builtin/fast-export.c | 3 +--
builtin/merge.c | 3 +--
builtin/reflog.c | 3 +--
contrib/coccinelle/xstrncmpz.cocci | 28 ++++++++++++++++++++++++++++
convert.c | 2 +-
merge-ll.c | 2 +-
object.c | 3 +--
remote.c | 5 ++---
userdiff.c | 3 +--
10 files changed, 38 insertions(+), 16 deletions(-)
create mode 100644 contrib/coccinelle/xstrncmpz.cocci
diff --git a/archive-tar.c b/archive-tar.c
index f2a0ed7752..8ae30125f8 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -365,7 +365,7 @@ static struct archiver *find_tar_filter(const char *name, size_t len)
int i;
for (i = 0; i < nr_tar_filters; i++) {
struct archiver *ar = tar_filters[i];
- if (!strncmp(ar->name, name, len) && !ar->name[len])
+ if (!xstrncmpz(ar->name, name, len))
return ar;
}
return NULL;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index f18f0809f9..4693d18cc9 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -136,8 +136,7 @@ static int anonymized_entry_cmp(const void *cmp_data UNUSED,
a = container_of(eptr, const struct anonymized_entry, hash);
if (keydata) {
const struct anonymized_entry_key *key = keydata;
- int equal = !strncmp(a->orig, key->orig, key->orig_len) &&
- !a->orig[key->orig_len];
+ int equal = !xstrncmpz(a->orig, key->orig, key->orig_len);
return !equal;
}
diff --git a/builtin/merge.c b/builtin/merge.c
index 8f819781cc..935c8a57dd 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -192,8 +192,7 @@ static struct strategy *get_strategy(const char *name)
int j, found = 0;
struct cmdname *ent = main_cmds.names[i];
for (j = 0; !found && j < ARRAY_SIZE(all_strategy); j++)
- if (!strncmp(ent->name, all_strategy[j].name, ent->len)
- && !all_strategy[j].name[ent->len])
+ if (!xstrncmpz(all_strategy[j].name, ent->name, ent->len))
found = 1;
if (!found)
add_cmdname(¬_strategies, ent->name, ent->len);
diff --git a/builtin/reflog.c b/builtin/reflog.c
index a5a4099f61..2c3369fca5 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -96,8 +96,7 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
reflog_expire_cfg_tail = &reflog_expire_cfg;
for (ent = reflog_expire_cfg; ent; ent = ent->next)
- if (!strncmp(ent->pattern, pattern, len) &&
- ent->pattern[len] == '\0')
+ if (!xstrncmpz(ent->pattern, pattern, len))
return ent;
FLEX_ALLOC_MEM(ent, pattern, pattern, len);
diff --git a/contrib/coccinelle/xstrncmpz.cocci b/contrib/coccinelle/xstrncmpz.cocci
new file mode 100644
index 0000000000..ccb39e2bc0
--- /dev/null
+++ b/contrib/coccinelle/xstrncmpz.cocci
@@ -0,0 +1,28 @@
+@@
+expression S, T, L;
+@@
+(
+- strncmp(S, T, L) || S[L]
++ !!xstrncmpz(S, T, L)
+|
+- strncmp(S, T, L) || S[L] != '\0'
++ !!xstrncmpz(S, T, L)
+|
+- strncmp(S, T, L) || T[L]
++ !!xstrncmpz(T, S, L)
+|
+- strncmp(S, T, L) || T[L] != '\0'
++ !!xstrncmpz(T, S, L)
+|
+- !strncmp(S, T, L) && !S[L]
++ !xstrncmpz(S, T, L)
+|
+- !strncmp(S, T, L) && S[L] == '\0'
++ !xstrncmpz(S, T, L)
+|
+- !strncmp(S, T, L) && !T[L]
++ !xstrncmpz(T, S, L)
+|
+- !strncmp(S, T, L) && T[L] == '\0'
++ !xstrncmpz(T, S, L)
+)
diff --git a/convert.c b/convert.c
index a8870baff3..35b25eb3cb 100644
--- a/convert.c
+++ b/convert.c
@@ -1028,7 +1028,7 @@ static int read_convert_config(const char *var, const char *value,
if (parse_config_key(var, "filter", &name, &namelen, &key) < 0 || !name)
return 0;
for (drv = user_convert; drv; drv = drv->next)
- if (!strncmp(drv->name, name, namelen) && !drv->name[namelen])
+ if (!xstrncmpz(drv->name, name, namelen))
break;
if (!drv) {
CALLOC_ARRAY(drv, 1);
diff --git a/merge-ll.c b/merge-ll.c
index 5ffb045efb..61e0ae5398 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -292,7 +292,7 @@ static int read_merge_config(const char *var, const char *value,
* after seeing merge.<name>.var1.
*/
for (fn = ll_user_merge; fn; fn = fn->next)
- if (!strncmp(fn->name, name, namelen) && !fn->name[namelen])
+ if (!xstrncmpz(fn->name, name, namelen))
break;
if (!fn) {
CALLOC_ARRAY(fn, 1);
diff --git a/object.c b/object.c
index 2c61e4c862..e6a1c4d905 100644
--- a/object.c
+++ b/object.c
@@ -47,8 +47,7 @@ int type_from_string_gently(const char *str, ssize_t len, int gentle)
len = strlen(str);
for (i = 1; i < ARRAY_SIZE(object_type_strings); i++)
- if (!strncmp(str, object_type_strings[i], len) &&
- object_type_strings[i][len] == '\0')
+ if (!xstrncmpz(object_type_strings[i], str, len))
return i;
if (gentle)
diff --git a/remote.c b/remote.c
index e07b316eac..9090632e96 100644
--- a/remote.c
+++ b/remote.c
@@ -105,7 +105,7 @@ static int remotes_hash_cmp(const void *cmp_data UNUSED,
b = container_of(entry_or_key, const struct remote, ent);
if (key)
- return strncmp(a->name, key->str, key->len) || a->name[key->len];
+ return !!xstrncmpz(a->name, key->str, key->len);
else
return strcmp(a->name, b->name);
}
@@ -189,8 +189,7 @@ static int branches_hash_cmp(const void *cmp_data UNUSED,
b = container_of(entry_or_key, const struct branch, ent);
if (key)
- return strncmp(a->name, key->str, key->len) ||
- a->name[key->len];
+ return !!xstrncmpz(a->name, key->str, key->len);
else
return strcmp(a->name, b->name);
}
diff --git a/userdiff.c b/userdiff.c
index e399543823..2b1dab2649 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -323,8 +323,7 @@ static int userdiff_find_by_namelen_cb(struct userdiff_driver *driver,
{
struct find_by_namelen_data *cb_data = priv;
- if (!strncmp(driver->name, cb_data->name, cb_data->len) &&
- !driver->name[cb_data->len]) {
+ if (!xstrncmpz(driver->name, cb_data->name, cb_data->len)) {
cb_data->driver = driver;
return 1; /* tell the caller to stop iterating */
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()
From: René Scharfe @ 2024-02-10 7:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Chandra Pratap, Chandra Pratap, Jeff King,
Kyle Lippincott, John Cai
In-Reply-To: <xmqqsf21cmp4.fsf@gitster.g>
Am 09.02.24 um 23:11 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> The string comparison becomes more complicated because we need to check
>> for NUL explicitly after comparing the length-limited option, but on the
>> flip side we don't need to clean up allocations or track the remaining
>> buffer length.
>
> Yeah, the strncmp() followed by the termination check indeed is
> trickier but not having to worry about allocation is nice.
>
>> if (options_seen > push_options->nr
>> - || strcmp(option,
>> - push_options->items[options_seen - 1].string)) {
>
> We used to allocate option[] with NUL termination, but ...
>
>> - retval = 0;
>> - goto leave;
>> - }
>> - free(option);
>> + || strncmp(push_options->items[options_seen - 1].string,
>> + option, optionlen)
>> + || push_options->items[options_seen - 1].string[optionlen])
>
> ... now option[] is a borrowed memory, option[optionlen] would have
> been NUL if we were allocating. So to see if the last-seen string[]
> is different from option[], we have to see that they match up to
> optionlen and the last-seen string[] ends there. Trickier than
> before, but is correct.
I just discovered 14570dc67d (wrapper: add function to compare strings
with different NUL termination, 2020-05-25). Perhaps squash this in to
simplify?
---
builtin/receive-pack.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index dbee508775..db65607485 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -717,9 +717,8 @@ static int check_cert_push_options(const struct string_list *push_options)
buf = option + optionlen + 1;
options_seen++;
if (options_seen > push_options->nr
- || strncmp(push_options->items[options_seen - 1].string,
- option, optionlen)
- || push_options->items[options_seen - 1].string[optionlen])
+ || xstrncmpz(push_options->items[options_seen - 1].string,
+ option, optionlen))
return 0;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/8] cherry-pick: add `--empty`
From: Brian Lyles @ 2024-02-10 7:43 UTC (permalink / raw)
To: git; +Cc: Brian Lyles, newren, me, phillip.wood123, gitster
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
The ultimate goal of this series is to allow git-cherry-pick(1) to
automatically drop redundant commits. The mechanism chosen is an
`--empty` option that provides the same flexibility as the `--empty`
options for git-rebase(1) and git-am(1).
Some secondary goals are to improve the consistency in the values and
documentation for this option across the three commands.
See "Does extending `--empty` to git-cherry-pick make sense?" [1] for
some context for why this option is desired in git-cherry-pick(1).
[1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com
Along the way, I (with some help from Elijah and Phillip) found a few
other things in the docs and related sequencer code to clean up.
Brian Lyles (8):
docs: address inaccurate `--empty` default with `--exec`
docs: clean up `--empty` formatting in git-rebase(1) and git-am(1)
rebase: update `--empty=ask` to `--empty=drop`
sequencer: treat error reading HEAD as unborn branch
sequencer: do not require `allow_empty` for redundant commit options
cherry-pick: decouple `--allow-empty` and `--keep-redundant-commits`
cherry-pick: enforce `--keep-redundant-commits` incompatibility
cherry-pick: add `--empty` for more robust redundant commit handling
Documentation/git-am.txt | 20 ++++---
Documentation/git-cherry-pick.txt | 30 +++++++---
Documentation/git-rebase.txt | 26 ++++++---
builtin/rebase.c | 16 +++--
builtin/revert.c | 40 +++++++++++--
sequencer.c | 65 +++++++++++----------
t/t3424-rebase-empty.sh | 55 ++++++++++++++++-
t/t3501-revert-cherry-pick.sh | 11 ++++
t/t3505-cherry-pick-empty.sh | 29 ++++++++-
t/t3510-cherry-pick-sequence.sh | 40 +++++++++++++
t/t3515-cherry-pick-incompatible-options.sh | 48 +++++++++++++++
11 files changed, 312 insertions(+), 68 deletions(-)
create mode 100755 t/t3515-cherry-pick-incompatible-options.sh
--
2.43.0
^ permalink raw reply
* [PATCH v2 1/8] docs: address inaccurate `--empty` default with `--exec`
From: Brian Lyles @ 2024-02-10 7:43 UTC (permalink / raw)
To: git; +Cc: Brian Lyles, newren, me, phillip.wood123, gitster, Phillip Wood
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
The documentation for git-rebase(1) indicates that using the `--exec`
option will use `--empty=drop`. This is inaccurate: when `--interactive`
is not explicitly provided, `--exec` results in `--empty=keep`
behaviors.
Correctly indicate the behavior of `--exec` using `--empty=keep` when
`--interactive` is not specified.
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
This commit was not present in v1. It is a fix to some existing
documentation that Phillip noticed was incorrect.
Documentation/git-rebase.txt | 10 +++++-----
t/t3424-rebase-empty.sh | 38 ++++++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 150734fb39..9d7397b696 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -295,11 +295,11 @@ See also INCOMPATIBLE OPTIONS below.
empty after rebasing (because they contain a subset of already
upstream changes). With drop (the default), commits that
become empty are dropped. With keep, such commits are kept.
- With ask (implied by `--interactive`), the rebase will halt when
- an empty commit is applied allowing you to choose whether to
- drop it, edit files more, or just commit the empty changes.
- Other options, like `--exec`, will use the default of drop unless
- `-i`/`--interactive` is explicitly specified.
+ With ask, the rebase will halt when an empty commit is applied
+ allowing you to choose whether to drop it, edit files more, or just
+ commit the empty changes.
+ When the `-i`/`--interactive` option is used, the default becomes ask.
+ Otherwise, when the `--exec` option is used, the default becomes keep.
+
Note that commits which start empty are kept (unless `--no-keep-empty`
is specified), and commits which are clean cherry-picks (as determined
diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
index 5e1045a0af..73ff35ced2 100755
--- a/t/t3424-rebase-empty.sh
+++ b/t/t3424-rebase-empty.sh
@@ -167,4 +167,42 @@ test_expect_success 'rebase --merge does not leave state laying around' '
test_path_is_missing .git/MERGE_MSG
'
+test_expect_success 'rebase --exec --empty=drop' '
+ git checkout -B testing localmods &&
+ git rebase --exec "true" --empty=drop upstream &&
+
+ test_write_lines D C B A >expect &&
+ git log --format=%s >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rebase --exec --empty=keep' '
+ git checkout -B testing localmods &&
+ git rebase --exec "true" --empty=keep upstream &&
+
+ test_write_lines D C2 C B A >expect &&
+ git log --format=%s >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rebase --exec uses default of --empty=keep' '
+ git checkout -B testing localmods &&
+ git rebase --exec "true" upstream &&
+
+ test_write_lines D C2 C B A >expect &&
+ git log --format=%s >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rebase --exec --empty=ask' '
+ git checkout -B testing localmods &&
+ test_must_fail git rebase --exec "true" --empty=ask upstream &&
+
+ git rebase --skip &&
+
+ test_write_lines D C B A >expect &&
+ git log --format=%s >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.43.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox