* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: René Scharfe @ 2024-02-26 17:26 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Phillip Wood, Josh Steadmon, Achu Luma
In-Reply-To: <CAP8UFD0Wi3ot-t0Q7ruMauwj4zkMfd89Xr9SmxYa4eQ3=2VKOw@mail.gmail.com>
Am 26.02.24 um 10:28 schrieb Christian Couder:
> On Sun, Feb 25, 2024 at 12:27 PM René Scharfe <l.s.r@web.de> wrote:
>>
>> Simplify TEST_CHAR_CLASS by using TEST for each character separately.
>> This increases the number of tests to 3598,
>
> Does this mean that when all the tests pass there will be 3598 lines
> of output on the terminal instead of 14 before this patch?
Yes.
> If that's the case, I don't like this.
>
>> but avoids the need for
>> using internal functions and test_msg() for custom messages. The
>> resulting macro has minimal test setup overhead.
>
> Yeah, the code looks definitely cleaner, but a clean output is important too.
The output is clean as well, but there's a lot of it. Perhaps too much.
The success messages are boring, though, and if all checks pass then the
only useful information is the status code. A TAP harness like prove
summarizes that nicely:
$ prove t/unit-tests/bin/t-ctype
t/unit-tests/bin/t-ctype .. ok
All tests successful.
Files=1, Tests=3598, 0 wallclock secs ( 0.08 usr + 0.00 sys = 0.08 CPU)
Result: PASS
Filtering out passing checks e.g. with "| grep -v ^ok" would help when
debugging a test failure. I vaguely miss the --immediate switch from the
regular test library, however.
René
^ permalink raw reply
* Re: [PATCH v3] build: support z/OS (OS/390).
From: Junio C Hamano @ 2024-02-26 17:30 UTC (permalink / raw)
To: Haritha via GitGitGadget; +Cc: git, Kristoffer Haugsbakk, Haritha
In-Reply-To: <pull.1663.v3.git.git.1708841439516.gitgitgadget@gmail.com>
"Haritha via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Haritha D <harithamma.d@ibm.com>
>
> Since the z/OS linker does not support searching dynamic libraries,
> and the current setting of CC_LD_DYNPATH results in a directory
> to be supplied to the link step with no option as the suffix,
> it causes a linker error because the z/OS LD linker
> does not accept directories as input.
> Therefore, -L option is added.
Shouldn't all of the above removed by now, with 07bbe4ca (Merge
branch 'jc/make-libpath-template', 2024-02-06) that merged the
support for platforms whose linker does not have a way to specify
the path used at runtime to locate dynamic libraries and is already
in the Git 2.44 release? The autoconf-generated config.mak.autogen
would leave CC_LD_DYNPATH to empty on such a platform, which then
adds only one "-L $where" (instead of the usual "-L $where -R
$where") on the linker command line, making your earlier workaround
to use "-L $where -L $where" unnecessary?
> Also introduced z/OS (OS/390) as a platform in config.mak.uname
"Introduce z/OS ..." (cf. Documentation/SubmittingPatches)
Or perhaps
Add platform specific settings in config.mak.uname for zOS
(OS/390).
> diff --git a/config.mak.uname b/config.mak.uname
> index dacc95172dc..03ee2b74525 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -638,6 +638,18 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
> SHELL_PATH = /usr/coreutils/bin/bash
> endif
> +ifeq ($(uname_S),OS/390)
> + NO_SYS_POLL_H = YesPlease
> + NO_STRCASESTR = YesPlease
> + NO_REGEX = YesPlease
> + NO_MMAP = YesPlease
> + NO_NSEC = YesPlease
> + NO_STRLCPY = YesPlease
> + NO_MEMMEM = YesPlease
> + NO_GECOS_IN_PWENT = YesPlease
> + HAVE_STRINGS_H = YesPlease
> + NEEDS_MODE_TRANSLATION = YesPlease
All of the above should be indented with TAB to imitate the support
for other platforms, I think.
Thanks.
> +endif
> ifeq ($(uname_S),MINGW)
> ifeq ($(shell expr "$(uname_R)" : '1\.'),2)
> $(error "Building with MSys is no longer supported")
>
> base-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11
^ permalink raw reply
* Re: [PATCH v5 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-26 17:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Michael Lohmann
In-Reply-To: <xmqqa5nnj10v.fsf@gitster.g>
Hi Junio,
Le 2024-02-25 à 23:35, Junio C Hamano a écrit :
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
>
>> + 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 requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
>> +}
>
> Just a minor nit, but reacting to recent "passive-aggressive"
> message change in another thread, perhaps we should stop asking a
> rhetorical question like the new message and instead state what we
> detected and what we consider is an error condition as a fact in
> them.
>
> The last die() in the above helper function used to be such a
> rhetorical question "--merge without HEAD?" but now it reads much
> better. The one about symbolic ref is new in this series, and we
> can avoid making it rhetorical from the get go. Perhaps "%s exists
> but it is a symbolic ref" or something?
Ok, I can make that change. I agree we should maybe keep these rethorical
questions to 'BUG' calls...
Thanks,
Philippe.
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: Junio C Hamano @ 2024-02-26 17:44 UTC (permalink / raw)
To: René Scharfe
Cc: Christian Couder, git, Phillip Wood, Josh Steadmon, Achu Luma
In-Reply-To: <d96aaf45-f073-42d0-b69c-703393634848@web.de>
René Scharfe <l.s.r@web.de> writes:
> Filtering out passing checks e.g. with "| grep -v ^ok" would help when
> debugging a test failure. I vaguely miss the --immediate switch from the
> regular test library, however.
Yeah, "-i" is one of the most useful switches during debugging
tests.
^ permalink raw reply
* Re: [PATCH] upload-pack: don't send null character in abort message to the client
From: Junio C Hamano @ 2024-02-26 17:49 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git
In-Reply-To: <20240225183452.1939334-1-szeder.dev@gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> Since 583b7ea31b (upload-pack/fetch-pack: support side-band
> communication, 2006-06-21) the abort message sent by upload-pack in
> case of possible repository corruption ends with a null character.
It is so so old that makes me wonder if it is safe to "fix" it, but
I cannot think of a sensible way to write a third-party client that
may have been working fine and would break when this fix is made.
> This can be seen in several test cases in 't5530-upload-pack-error.sh'
> where 'grep <pattern> output.err' often reports "Binary file
> output.err matches" because of that null character.
>
> The reason for this is that the abort message is defined as a string
> literal, and we pass its size to the send function as
> sizeof(abort_msg), which also counts the terminating null character.
>
> Use strlen() instead to avoid sending that terminating null character.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> upload-pack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 2537affa90..6e0d441ef5 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -463,7 +463,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>
> fail:
> free(output_state);
> - send_client_data(3, abort_msg, sizeof(abort_msg),
> + send_client_data(3, abort_msg, strlen(abort_msg),
> pack_data->use_sideband);
> die("git upload-pack: %s", abort_msg);
> }
^ permalink raw reply
* Re: [PATCH v5 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Philippe Blain @ 2024-02-26 17:54 UTC (permalink / raw)
To: Jean-Noël Avila, git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann
In-Reply-To: <c46a0a4f-63dc-404a-8ee2-64a34f474c5f@gmail.com>
Hi Jean-Noël,
Le 2024-02-26 à 12:22, Jean-Noël Avila a écrit :
> Hello,
>
> Le 25/02/2024 à 22:56, Philippe Blain a écrit :
>> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>>
>> This is done to
>> (1) ensure MERGE_HEAD is a ref,
>> (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
>> (3) error out when MERGE_HEAD is a symref.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>> revision.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 2424c9bd67..ee26988cc6 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1973,8 +1973,12 @@ 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 (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
>> + 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?"));
>
> Following the thread about being less passive-aggressive, maybe this
> could be rephrased in an assertive mood.
Yes, Junio suggested the same in <xmqqa5nnj10v.fsf@gitster.g>.
>
> By the way, this string is translatable, but not the one 2 lines above.
> What is the policy around translation?
My understanding is that new error messages should be translated, but here the patch
is not touching the message "--merge without HEAD?" so I would think
it is OK to avoid changing these lines to mark it for translation.
But, I could make that change in a preparatory patch (and rephrase it
at the same time).
Thanks,
Philippe.
^ permalink raw reply
* Re: [PATCH] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Junio C Hamano @ 2024-02-26 18:03 UTC (permalink / raw)
To: Josh Triplett; +Cc: git
In-Reply-To: <9c09cea2679e14258720ee63e932e3b9459dbd8c.1708921369.git.josh@joshtriplett.org>
Josh Triplett <josh@joshtriplett.org> writes:
> `git commit --cleanup=scissors -v` currently prints two scissors lines:
> one at the start of the comment lines, and the other right before the
> diff. This is redundant, and pushes the diff further down in the user's
> editor than it needs to be.
Interesting discovery.
> diff --git a/wt-status.c b/wt-status.c
> index b5a29083df..459d399baa 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1143,11 +1143,13 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
> * file (and even the "auto" setting won't work, since it
> * will have checked isatty on stdout). But we then do want
> * to insert the scissor line here to reliably remove the
> - * diff before committing.
> + * diff before committing, if we didn't already include one
> + * before.
> */
> if (s->fp != stdout) {
> rev.diffopt.use_color = 0;
> - wt_status_add_cut_line(s->fp);
> + if (s->cleanup_mode != COMMIT_MSG_CLEANUP_SCISSORS)
> + wt_status_add_cut_line(s->fp);
> }
The machinery to populate the log message buffer should ideally be
taught to remember if it already has added a scissors-line and to
refrain from adding redundant ones. That way, we do not have to
rely on the order of places that make wt_status_add_cut_line() calls
or what condition they use to decide to make these calls.
This hunk for example knows not just this one produces cut-line
after the other one potentially added one, but also the logic used
by the other one to decide to add one, which is even worse. I find
the solution presented here a bit unsatisfactory, for this reason,
but for now it may be OK, as we probably are not adding any more
places and conditions to emit a scissors line.
> builtin/commit.c | 2 ++
> sequencer.h | 7 -------
> wt-status.c | 6 ++++--
> wt-status.h | 8 ++++++++
> 4 files changed, 14 insertions(+), 9 deletions(-)
If this change did not break any existing tests that checked the
combination of options and output when they are used together, it
means we have a gap in the test coverage. We needs a test or two
to protect this fix from future breakages.
Thanks.
^ permalink raw reply
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: rsbecker @ 2024-02-26 18:03 UTC (permalink / raw)
To: phillip.wood, 'Torsten Bögershausen'
Cc: git, 'Patrick Steinhardt'
In-Reply-To: <76962a0c-adfd-47a5-a017-a117ba14ae09@gmail.com>
>From: Phillip Wood <phillip.wood123@gmail.com>
On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote:
>To: rsbecker@nexbridge.com; 'Torsten Bögershausen' <tboegi@web.de>
>Cc: git@vger.kernel.org; Patrick Steinhardt <ps@pks.im>
>Subject: Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
>
>On 26/02/2024 15:32, Phillip Wood wrote:
>> Hi Randal
>>
>> [cc'ing Patrick for the reftable writer]
>>
>> On 25/02/2024 20:36, rsbecker@nexbridge.com wrote:
>>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>>>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>>>> To: git@vger.kernel.org
>>>> But I think that this should be used:
>>>> write_in_full()
>>>
>>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
>>> although a bit redundant since xwrite() does similar things and is
>>> used by write_in_full().
>>
>> Note that unlike write_in_full(), xwrite() does not guarantee to write
>> the whole buffer passed to it. In general unless a caller is writing a
>> single byte or writing less than PIPE_BUF bytes to a pipe it should
>> use write_in_full().
>>
>>> The question is which call is bad? The cruft stuff is relatively new
>>> and I don't know the code.
>
>I should have been clearer that I do not think any of these calls are the likely
>problem for the cruft pack code. I do think the reftable writers are worth looking at
>though for git in general.
>
>For the cruft pack problem you might want to look for suspect xwrite() calls where
>the caller does not handle a short write correctly for example under builtin/ we have
>
>builtin/index-pack.c: err = xwrite(1, input_buffer +
>input_offset, input_len);
>builtin/receive-pack.c: xwrite(2, msg, sz);
>builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid),
>the_hash_algo->hexsz);
>builtin/repack.c: xwrite(cmd->in, "\n", 1);
>builtin/unpack-objects.c: int ret = xwrite(1, buffer +
>offset, len);
>
>Best Wishes
>
>Phillip
>
>>>>> reftable/writer.c: int n = w->write(w->write_arg,
>>>>> zeroed,
>>>>> w->pending_padding);
>>>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>>
>> Neither of these appear to check for short writes and
>> reftable_fd_write() is a thin wrapper around write(). Maybe
>> reftable_fd_write() should be using write_in_full()?
>>
>>>>> run-command.c: len = write(io->fd, io->u.out.buf,
>>
>> This call to write() looks correct as it is in the io pump loop.
>>
>>>>> t/helper/test-path-utils.c: if (write(1,
>>>>> buffer,
>>> count)
>>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1,
>>>>> buf, nbr);
>>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>>
>> In principle these all look like they are prone to short writes.
>>
>>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf,
>>>>> buf_line->len);
>>
>> This caller explicitly says it prefers short writes over retrying
I'm getting caught a bit behind the curve. After rebuilding from master, I'm now getting:
+ test 1708960150 -lt 1708970156
+ test_line_count = 3 cruft.before
+ test_line_count = 2 cruft.after
test_line_count: line count for cruft.after != 2
This is looking more like a different problem than xwrite().
^ permalink raw reply
* Re: [PATCH 2/2] name-rev: use mem_pool_strfmt()
From: René Scharfe @ 2024-02-26 18:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqfrxghtef.fsf@gitster.g>
Am 26.02.24 um 03:05 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> 1c56fc2084 (name-rev: pre-size buffer in get_parent_name(), 2020-02-04)
>> got a big performance boost in an unusual repository by calculating the
>> name length in advance. This is a bit awkward, as it references the
>> name components twice.
>>
>> Use a memory pool to store the strings for the struct rev_name member
>> tip_name. Using mem_pool_strfmt() allows efficient allocation without
>> explicit size calculation. This simplifies the formatting part of the
>> code without giving up performance:
>>
>> Benchmark 1: ./git_2.44.0 -C ../chromium/src name-rev --all
>> Time (mean ± σ): 1.231 s ± 0.013 s [User: 1.082 s, System: 0.136 s]
>> Range (min … max): 1.214 s … 1.252 s 10 runs
>>
>> Benchmark 2: ./git -C ../chromium/src name-rev --all
>> Time (mean ± σ): 1.220 s ± 0.020 s [User: 1.083 s, System: 0.130 s]
>> Range (min … max): 1.197 s … 1.254 s 10 runs
>>
>> Don't bother discarding the memory pool just before exiting. The effort
>> for that would be very low, but actually measurable in the above
>> example, with no benefit to users. At least UNLEAK it to calm down leak
>> checkers. This addresses the leaks that 45a14f578e (Revert "name-rev:
>> release unused name strings", 2022-04-22) brought back.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> This doesn't make any test script leak-free, though.
>
> Hmph, is the root cause of the leak because no sensible ownership
> rules are applied to .tip_name? Sometimes it is allocated for the
> paritcular rev_name, but some other times the pointer is copied from
> another rev_name.tip_name? As the way currently the code uses the
> .tip_name member seems to be "allocate and use without any freeing",
> I tend to agree that throwing them into mem-pool does make sense.
Yes, the tip_name string is shared between child and first parent (and
its first parent and so on, until a better name is found. Without this
sharing the peak memory footprint of "git name-rev --all" in Git's repo
goes from 40011328 to 46286528 for me currently.
I'm not too worried about the leak, though, as we can't release the
memory until we're done anyway. The ability to build strings without
having to calculate their sizes in advance (either by running vsnprintf
twice, which is slow, or by doing format-specific calculations) is more
interesting to me.
The reason why none of the test scripts become leak-free is other
commands (i.e. not git name-rev) that are still leaking.
René
^ permalink raw reply
* Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string
From: Junio C Hamano @ 2024-02-26 18:10 UTC (permalink / raw)
To: Christian Couder; +Cc: Achu Luma, git, Christian Couder
In-Reply-To: <CAP8UFD0Qhy78=v9+hCekMJPkcH2KmeZeQ0xUx8kqrByQ4PO3Xg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> Unfortunately strbuf_add() calls strbuf_grow() itself which is not
> needed as we have already called strbuf_grow() to avoid repeated
> reallocation.
Why is it unfortunate? If the current allocation is sufficient, it
is a cheap no-op, isn't it (if not, we should make it so)?
^ permalink raw reply
* Re: Git For Windows, not installing right
From: Eric Sunshine @ 2024-02-26 18:12 UTC (permalink / raw)
To: Chaython Meredith; +Cc: git@vger.kernel.org
In-Reply-To: <MW4PR10MB6324B0F27AE4025F111E2547CE5A2@MW4PR10MB6324.namprd10.prod.outlook.com>
On Mon, Feb 26, 2024 at 4:39 AM Chaython Meredith <chaython@live.ca> wrote:
> Git for windows, is not setting environmental variables, despite being asked to during installation.
> After setting up environmental variables, errors still occur.
Git-for-Windows and the Git-for-Windows installer are maintained and
produced by gitforwindows.org which has a dedicated bug tracker[*] for
Windows-specific issues. Submitting this report to the Git-for-Windows
bug tracker will likely lead to a more timely response.
[*] https://github.com/git-for-windows/git/issues
^ permalink raw reply
* Re: [PATCH v2] merge-ort: turn submodule conflict suggestions into an advice
From: Philippe Blain @ 2024-02-26 18:14 UTC (permalink / raw)
To: Junio C Hamano, Philippe Blain via GitGitGadget
Cc: git, Calvin Wan, Glen Choo, Elijah Newren,
Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqqh6hvfb6c.fsf@gitster.g>
Hi Junio,
Le 2024-02-26 à 11:22, Junio C Hamano a écrit :
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> Add a new advice type 'submoduleMergeConflict' for the error message
>> shown when a non-trivial submodule conflict is encountered, which was
>> added in 4057523a40 (submodule merge: update conflict error message,
>> 2022-08-04). That commit mentions making this message an advice as
>> possible future work. Only show the advice if it is enabled.
>
> When specifically called out like this, it makes it sound as if this
> one is disabled by default, while everybody else is enabled unless it
> is disabled.
Yes, re-reading myself, this did not come out as I wanted. I'll update
the message to read more like "Adjust merge-ort.c::print_submodule_conflict_suggestion
to use the advice API" or something like this.
Thanks,
Philippe.
^ permalink raw reply
* Re: [PATCH 1/2] mem-pool: add mem_pool_strfmt()
From: René Scharfe @ 2024-02-26 18:17 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240226070844.GB780982@coredump.intra.peff.net>
Am 26.02.24 um 08:08 schrieb Jeff King:
> On Sun, Feb 25, 2024 at 12:39:44PM +0100, René Scharfe wrote:
>
>> +static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,
>> + va_list ap)
>> +{
>> + struct mp_block *block = pool->mp_block;
>> + char *next_free = block ? block->next_free : NULL;
>> + size_t available = block ? block->end - block->next_free : 0;
>> + va_list cp;
>> + int len, len2;
>> + char *ret;
>> +
>> + va_copy(cp, ap);
>> + len = vsnprintf(next_free, available, fmt, cp);
>> + va_end(cp);
>> + if (len < 0)
>> + BUG("your vsnprintf is broken (returned %d)", len);
>> +
>> + ret = mem_pool_alloc(pool, len + 1); /* 1 for NUL */
>> +
>> + /* Shortcut; relies on mem_pool_alloc() not touching buffer contents. */
>> + if (ret == next_free)
>> + return ret;
>> +
>> + len2 = vsnprintf(ret, len + 1, fmt, ap);
>> + if (len2 != len)
>> + BUG("your vsnprintf is broken (returns inconsistent lengths)");
>> + return ret;
>> +}
>
> This is pulling heavily from strbuf_vaddf(). This might be a dumb idea,
> but... would it be reasonable to instead push a global flag that causes
> xmalloc() to use a memory pool instead of the regular heap?
>
> Then you could do something like:
>
> push_mem_pool(pool);
> str = xstrfmt("%.*s~%d^%d", ...etc...);
> pop_mem_pool(pool);
>
> It's a little more involved at the caller, but it means that it now
> works for all allocations, not just this one string helper.
That would allow to keep track of allocations that would otherwise leak.
We can achieve that more easily by pushing the pointer to a global array
and never freeing it. Hmm.
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?
René
^ permalink raw reply
* Re: [PATCH v2] merge-ort: turn submodule conflict suggestions into an advice
From: Junio C Hamano @ 2024-02-26 18:34 UTC (permalink / raw)
To: Philippe Blain
Cc: Philippe Blain via GitGitGadget, git, Calvin Wan, Glen Choo,
Elijah Newren, Ævar Arnfjörð Bjarmason
In-Reply-To: <c00617b0-b673-b52c-db15-278a67370e75@gmail.com>
Philippe Blain <levraiphilippeblain@gmail.com> writes:
> Hi Junio,
>
> Le 2024-02-26 à 11:22, Junio C Hamano a écrit :
>> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>>
>>> Add a new advice type 'submoduleMergeConflict' for the error message
>>> shown when a non-trivial submodule conflict is encountered, which was
>>> added in 4057523a40 (submodule merge: update conflict error message,
>>> 2022-08-04). That commit mentions making this message an advice as
>>> possible future work. Only show the advice if it is enabled.
>>
>> When specifically called out like this, it makes it sound as if this
>> one is disabled by default, while everybody else is enabled unless it
>> is disabled.
>
> Yes, re-reading myself, this did not come out as I wanted. I'll update
> the message to read more like "Adjust merge-ort.c::print_submodule_conflict_suggestion
> to use the advice API" or something like this.
I've replaced the last sentence with """The message can now be
disabled with the advice mechanism."""
^ permalink raw reply
* Re: [PATCH v2 09/16] fsmonitor: move untracked invalidation into helper functions
From: Jeff Hostetler @ 2024-02-26 18:45 UTC (permalink / raw)
To: Junio C Hamano, Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler
In-Reply-To: <xmqqy1bbuloo.fsf@gitster.g>
On 2/23/24 12:36 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Jeff Hostetler <jeffhostetler@github.com>
>>
>> Move the call to invalidate the untracked cache for the FSEvent
>> pathname into the two helper functions.
>>
>> In a later commit in this series, we will call these helpers
>> from other contexts and it safer to include the UC invalidation
>> in the helper than to remember to also add it to each helper
>> call-site.
>>
>> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
>> ---
>> fsmonitor.c | 26 +++++++++++++++++++-------
>> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> Thanks. The steps in this iteration makes this move much less
> confusing to me than in the previous one. We used to call one of
> "handle path with/without trailing slash" functions and then called
> the invalidation. Now the invalidation happens in these "handle path"
> functions.
>
> The unexplained change in behaviour is that we used to do the rest
> of "handle path" and invalidation was done at the end. Now we do it
> upfront. I think the "rest" works solely based on what is in the
> main in-core index array (i.e. the_index.cache[] aka active_cache[])
> and affects only what is in the in-core index array, while
> untracked_cache_invalidate*() works solely based on what is in the
> untracked cache extension (i.e. the_index.untracked) and affects
> only what is in there, so the order of these two does not matter.
>
> Am I correct?
>
> Or does it affect correctness or performance or whatever in any way?
> IOW, is there a reason why it is better to do the invalidation first
> and then doing the "rest" after (hence this patch flips the order of
> two to _improve_ something)?
>
> Thanks.
The ce_flags invalidation and the untracked-cache invalidation are
independent (as far as I could tell) and it doesn't matter which
order we do them. Moving the UC to the start of the function was
an attempt to avoid the usual "goto the bottom" or the need to guard
against early "return" statements that were present in some of the
original code (or my various refactorings). Moving it to the top
just let me get it out of the way and not have to contrive things.
I'll update the commit message.
Thanks
Jeff
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: Josh Steadmon @ 2024-02-26 18:58 UTC (permalink / raw)
To: René Scharfe; +Cc: Christian Couder, git, Phillip Wood, Achu Luma
In-Reply-To: <d96aaf45-f073-42d0-b69c-703393634848@web.de>
On 2024.02.26 18:26, René Scharfe wrote:
> Am 26.02.24 um 10:28 schrieb Christian Couder:
> > On Sun, Feb 25, 2024 at 12:27 PM René Scharfe <l.s.r@web.de> wrote:
> >>
> >> Simplify TEST_CHAR_CLASS by using TEST for each character separately.
> >> This increases the number of tests to 3598,
> >
> > Does this mean that when all the tests pass there will be 3598 lines
> > of output on the terminal instead of 14 before this patch?
>
> Yes.
>
> > If that's the case, I don't like this.
> >
> >> but avoids the need for
> >> using internal functions and test_msg() for custom messages. The
> >> resulting macro has minimal test setup overhead.
> >
> > Yeah, the code looks definitely cleaner, but a clean output is important too.
>
> The output is clean as well, but there's a lot of it. Perhaps too much.
> The success messages are boring, though, and if all checks pass then the
> only useful information is the status code. A TAP harness like prove
> summarizes that nicely:
>
> $ prove t/unit-tests/bin/t-ctype
> t/unit-tests/bin/t-ctype .. ok
> All tests successful.
> Files=1, Tests=3598, 0 wallclock secs ( 0.08 usr + 0.00 sys = 0.08 CPU)
> Result: PASS
>
> Filtering out passing checks e.g. with "| grep -v ^ok" would help when
> debugging a test failure. I vaguely miss the --immediate switch from the
> regular test library, however.
Yeah, I agree here. It's a lot of output but it's almost always going to
be consumed by a test harness rather than a human, and it's easy to
filter out the noise if someone does need to do some manual debugging.
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Kyle Lippincott @ 2024-02-26 18:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <xmqqh6i0cgyo.fsf@gitster.g>
On Thu, Feb 22, 2024 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Calvin Wan <calvinwan@google.com> writes:
>
> > From: Jonathan Tan <jonathantanmy@google.com>
> >
> > pager.h uses uintmax_t but does not include stdint.h. Therefore, add
> > this include statement.
> >
> > This was discovered when writing a stub pager.c file.
> >
> > Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> > Signed-off-by: Calvin Wan <calvinwan@google.com>
> > ---
> > pager.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/pager.h b/pager.h
> > index b77433026d..015bca95e3 100644
> > --- a/pager.h
> > +++ b/pager.h
> > @@ -1,6 +1,8 @@
> > #ifndef PAGER_H
> > #define PAGER_H
> >
> > +#include <stdint.h>
> > +
> > struct child_process;
> >
> > const char *git_pager(int stdout_is_tty);
>
> This is not going in a sensible direction from our portability
> standard's point of view.
>
> The reason why we do not include these system headers directly to
> our source files, and instead make it a rule to include
> <git-compat-util.h> as the first header instead, is exactly because
> there are curiosities in various platforms that Git wants to run on
> which system include headers give us the declarations for types and
> functions we rely on, in what order they must be included, and after
> what feature macros (the ones that give adjustment to what the
> system headers do, like _POSIX_C_SOURCE) are defined, etc.
>
> Given that in <git-compat-util.h>, inclusion of <stdint.h> is
> conditional behind some #ifdef's, it does not look like a sensible
> change. It is not very likely for <inttypes.h> and <stdint.h> to
> declare uintmax_t in an incompatible way, but on a platform where
> <git-compat-util.h> decides to include <inttypes.h> and use its
> definition of what uintmax_t is, we should follow the same choice
> and be consistent.
Speaking of this specific header file inclusion and the oddities that
have gotten us to where we are:
- Originally, it seems that we were including stdint.h
- 17 years ago, to work around Solaris not providing stdint.h, but
providing inttypes.h, it was switched to being just inttypes.h, with
the explanation being that inttypes is a superset of stdint.
https://github.com/git/git/commit/007e2ba65902b484fc65a313e54594a009841740
- 13 years ago, to work around some platforms not having inttypes.h,
it was made conditional.
(https://github.com/git/git/commit/2844923d62a4c408bd59ddb2caacca4aa7eb86bc)
The condition added 13 years ago was, IMHO, backwards from what it
should have been. The intent is to have stdint.h included. We should
include stdint.h. I suspect that 17 years is enough time for that
platform to start conforming to what is now a 25 year old standard,
and I don't know how we can verify that and have this stop being a
haunted graveyard without just trying it and seeing if the build bots
or maintainers identify it as a continuing issue. If it's still an
issue (and only if), we should reintroduce a conditional, but invert
it: if there's no stdint.h, THEN include inttypes.h.
Oh, no, that doesn't work. I tried that, and the build bots told me
that doesn't work, because we're using things from inttypes.h (PRIuMAX
showed up several times in the errors, there may be others). This
makes me wonder how the platforms with no inttypes.h work at all. I
still think we should do something here because it's a 13-year-old
compatibility fix that "shouldn't" be needed anymore, and causes
confusion/concerns like this thread. Maybe just see if we can get away
with always including inttypes.h in git-compat-util.h, or maybe _both_
inttypes.h and stdint.h (in either order), just to be really obvious
that it's acceptable to include stdint.h.
>
> If there is a feature macro that affects sizes of the integer on a
> platform, this patch will break it even more badly. Perhaps there
> is a platform whose C-library header requires you to define a
> feature macro to use 64-bit, and we may define that feature macro
> in <git-compat-util.h> before including either <inttypes.h> or
> <stdint.h>, but by including <stdint.h> directly like the above
> patch does, only this file and the sources that include only this
> file, refusing to include <git-compat-util.h> as everybody in the
> Git source tree should, will end up using different notion of what
> the integral type with maximum width is from everybody else.
I agree that for pager.h, something like the patch in your next email
would resolve that particular problem. The stub library is of
basically the same stature as git-std-lib: it's code that is provided
by the Git project, compiled by Makefile rules owned and maintained by
the Git project, and should conform to the Git coding standards. The
.c files in the stubs library should include git-compat-util.h,
there's basically no reason not to.
However, I believe that we'll need a good policy for what to do with
libified headers + sources in general. I can see many potential
categorizations of source; there's no need to formally define all of
them and assign files to each category, but the main categories are
basically:
1. files that have code that is an internal part of Git, one of the
helper binaries, or one of its tests, whether it's a library or not.
These should include git-compat-util.h at the top of the .c files like
they do today. The .h files for these translation units are also
considered "internal". These header files should assume that
git-compat-util.h has been included properly, and don't need to be
self-contained, because they're _only_ included by things in this
category.
2. files that have code that define the "library interface", probably
only the ones defining the library interface _as used by external
projects_. I think that we'll likely need to be principled about
defining these, and having them be as minimal and compatible as
possible.
3. code in external projects that directly uses libraries from the Git
project, and thus includes Git headers from category 2
4. the rest of the code in external projects (the code that does not
directly use libraries from the Git project)
A hypothetical git-compat-core.h being included at the top of the .c
files in category 2 is feasible, but needs to be carefully handled due
to potential symbol collision (which we're discussing in another
thread and I may have a possible solution for, at least on some
platforms). On the other hand, a git-compat-core.h being included at
the top of the .h files belonging to category 2 doesn't work, because
when these .h files are included by code in category 3, it's too late.
In this example, gnu-source-header.h below is a system header that
changes behavior depending on _GNU_SOURCE (effectively the same
concern as you were raising in the quoted paragraph):
external-project-category3.c:
#include <stdint.h>
#include <gnu-source-header.h>
#include <git/some-lib-interface.h>
git/some-lib-interface.h:
#include <git/git-compat-core.h>
We can't do anything in git/git-compat-core.h that relies on careful
inclusion order, requiring that various things are #defined prior to
the first inclusion of certain headers, etc. stdint.h and
gnu-source-header.h are already included, and so it's too late for us
to #define things that change their behavior, because that won't have
any effect. I don't think it's reasonable to expect the external
project to #include <git/git-compat-core.h> at the top of their files
that are in category3. It's definitely not reasonable to require the
external project to do that for all their files (category 3 and
category 4). It's slightly more reasonable to have them do some set of
#defines for their binaries and libraries, but still quite awkward and
potentially not feasible (for things like _FILE_OFFSET_BITS). This is
why I split it into 4 categories.
I believe that category 2 files need to be maximally compatible, both
to platforms [where we provide support for libraries, and I think this
probably will end up being a subset of all the platforms, especially
at first] and to the environment they're being #included in and
interacting with. So they need to be self-contained: they can't rely
on stdint.h having been included, but they also can't rely on it _not_
having been included already. The category 2 .h files need to be
minimal: just what we want in this external library interface, and
ideally nothing else. The category 2 .h and .c files need to be
compatible with common #defines being set OR not set. The point of a
category 2 .c file is to bridge the gap between the category 1 and
category 3 environments. This likely means that we need to be careful
about certain structs and typedefs defined by the system (vs. structs
defined by the category 2 headers themselves) being passed between the
different environments. For example, if we were to have a library
interface that included a `struct stat`, and the category 3 files
didn't have _FILE_OFFSET_BITS 64, but the category 1 files do? Instant
breakage.
This aspect of this discussion probably should happen on the next
patch, or in a separate thread :) But since I'm here, I'll summarize
these thoughts: basically, the next patch, imho, doesn't go far
enough, but is a very good first step that we can build on. We need to
define what belongs to the "external interface" of the various
libraries (category 2 above) and what is considered category 1.
pager.h is pretty obviously category 1. strbuf.h, abspath.h, etc? I'm
not sure. git-std-lib is weird, because it's so low level and there's
not really much "internal" code to this library. So maybe we declare
those as category 2. But I don't know how that will actually work in
practice. I'll try to find time to write up these thoughts on that
patch.
>
> What this patch _wants_ to do is of course sympathizable, and we
> have "make hdr-check" rule to enforce "a header must include the
> headers that declare what it uses", except that it lets the header
> files being tested assume that the things made available by
> including <git-compat-util.h> are always available.
>
> I think a sensible direction to go for libification purposes is to
> also make sure that sources that are compiled into gitstdlib.a, and
> the headers that makes what is in gitstdlib.a available, include the
> <git-compat-util.h> header file. There may be things declared in
> the <git-compat-util.h> header that are _too_ specific to what ought
> to be linked into the final "git" binary and unwanted by library
> clients that are not "git" binary, and the right way to deal with it
> is to split <git-compat-util.h> into two parts, i.e. what makes
> system services available like its conditional inclusion of
> <stdint.h> vs <inttypes.h>, definition of feature macros, order in
> which the current <git-compat-util.h> includes system headers, etc.,
> excluding those that made you write this patch to avoid assuming
> that the client code would have included <git-compat-util.h> before
> <pager.h>, would be the new <git-compat-core.h>. And everything
> else will remain in <git-compat-util.h>, which will include the
> <git-compat-core.h>. The <pager.h> header for library clients would
> include <git-compat-core.h> instead, to still allow them to use the
> same types as "git" binary itself that way.
>
>
>
>
>
>
^ permalink raw reply
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: rsbecker @ 2024-02-26 19:02 UTC (permalink / raw)
To: phillip.wood, 'Torsten Bögershausen'
Cc: git, 'Patrick Steinhardt'
In-Reply-To: <76962a0c-adfd-47a5-a017-a117ba14ae09@gmail.com>
On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote:
>On 26/02/2024 15:32, Phillip Wood wrote:
>> Hi Randal
>>
>> [cc'ing Patrick for the reftable writer]
>>
>> On 25/02/2024 20:36, rsbecker@nexbridge.com wrote:
>>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>>>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>>>> To: git@vger.kernel.org
>>>> But I think that this should be used:
>>>> write_in_full()
>>>
>>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
>>> although a bit redundant since xwrite() does similar things and is
>>> used by write_in_full().
>>
>> Note that unlike write_in_full(), xwrite() does not guarantee to write
>> the whole buffer passed to it. In general unless a caller is writing a
>> single byte or writing less than PIPE_BUF bytes to a pipe it should
>> use write_in_full().
>>
>>> The question is which call is bad? The cruft stuff is relatively new
>>> and I don't know the code.
>
>I should have been clearer that I do not think any of these calls are the likely
>problem for the cruft pack code. I do think the reftable writers are worth looking at
>though for git in general.
>
>For the cruft pack problem you might want to look for suspect xwrite() calls where
>the caller does not handle a short write correctly for example under builtin/ we have
>
>builtin/index-pack.c: err = xwrite(1, input_buffer +
>input_offset, input_len);
>builtin/receive-pack.c: xwrite(2, msg, sz);
>builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid),
>the_hash_algo->hexsz);
>builtin/repack.c: xwrite(cmd->in, "\n", 1);
>builtin/unpack-objects.c: int ret = xwrite(1, buffer +
>offset, len);
>
>Best Wishes
>
>Phillip
>
>>>>> reftable/writer.c: int n = w->write(w->write_arg,
>>>>> zeroed,
>>>>> w->pending_padding);
>>>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>>
>> Neither of these appear to check for short writes and
>> reftable_fd_write() is a thin wrapper around write(). Maybe
>> reftable_fd_write() should be using write_in_full()?
>>
>>>>> run-command.c: len = write(io->fd, io->u.out.buf,
>>
>> This call to write() looks correct as it is in the io pump loop.
>>
>>>>> t/helper/test-path-utils.c: if (write(1,
>>>>> buffer,
>>> count)
>>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1,
>>>>> buf, nbr);
>>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>>
>> In principle these all look like they are prone to short writes.
>>
>>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf,
>>>>> buf_line->len);
>>
>> This caller explicitly says it prefers short writes over retrying
Replacing xwrite with write_in_full the above worked correctly. Do you want it or should I write this up?
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a3a37bd215..f80b8d101a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
* the last part of the input buffer to stdout.
*/
while (input_len) {
- err = xwrite(1, input_buffer + input_offset, input_len);
+ err = write_in_full(1, input_buffer + input_offset, input_len);
if (err <= 0)
break;
input_len -= err;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index db65607485..4277c63d08 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
if (use_sideband)
send_sideband(1, 2, msg, sz, use_sideband);
else
- xwrite(2, msg, sz);
+ write_in_full(2, msg, sz);
}
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/repack.c b/builtin/repack.c
index ede36328a3..4916870992 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -314,8 +314,8 @@ static int write_oid(const struct object_id *oid,
die(_("could not start pack-objects to repack promisor objects"));
}
- xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
- xwrite(cmd->in, "\n", 1);
+ write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
+ write_in_full(cmd->in, "\n", 1);
return 0;
}
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index e0a701f2b3..6935c4574e 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
/* Write the last part of the buffer to stdout */
while (len) {
- int ret = xwrite(1, buffer + offset, len);
+ int ret = write_in_full(1, buffer + offset, len);
if (ret <= 0)
break;
len -= ret;
^ permalink raw reply related
* Re: Interactive rebase: using "pick" for merge commits
From: Stefan Haller @ 2024-02-26 19:07 UTC (permalink / raw)
To: phillip.wood, Patrick Steinhardt; +Cc: git
In-Reply-To: <b4781808-f722-4be5-906f-4c3409c3295c@gmail.com>
On 26.02.24 11:56, Phillip Wood wrote:
>> It probably makes more sense to teach lazygit to visualize the
>> .git/sequencer/todo file, and then use git cherry-pick.
>
> If lazygit is generating the todo list for the cherry-pick could it
> check if the commit is a merge and insert "exec cherry-pick -m ..." for
> those commits?
That's a good idea, but it wouldn't buy us very much. We'd still have to
add support for conflicts during a cherry-pick; when there's a conflict
during a rebase, lazygit has this nice visualization of the conflicting
commit (we talked about that in [1], and it turned out to be working
extremely well), so it would have to learn to do the same thing for a
conflicting cherry-pick (although this does seem to be a lot easier).
And then it would have to learn to call "cherry-pick --continue" rather
than "rebase --continue" after resolving. But if we do all these things,
then we're not so far away from being able to just call git cherry-pick
ourselves.
-Stefan
[1] <https://public-inbox.org/git/
961e68d7-5f43-c385-10fa-455b8e2f32d0@haller-berlin.de/>
^ permalink raw reply
* Re: [PATCH] unit-tests: convert t/helper/test-oid-array.c to unit-tests
From: Ghanshyam Thakkar @ 2024-02-26 19:11 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD088GRkVQWjrBFk04_HFfiEk64Saxm2toYsci36oHgkdA@mail.gmail.com>
On Mon Feb 26, 2024 at 8:41 PM IST, Christian Couder wrote:
> It might not be a good idea to start working on a GSoC project we
> propose (Move existing tests to a unit testing framework) right now.
> You can work on it as part of your GSoC application, to show an
> example of how you would do it, and we might review that as part of
> reviewing your application. But for such a project if many candidates
> started working on it and sent patches to the mailing list before they
> get selected, then the project might be nearly finished before the
> GSoC even starts.
>
> So I think it would be better to work on other things instead, like
> perhaps reviewing other people's work or working on other bug fixes or
> features. Anyway now that this is on the mailing list, I might as well
> review it as it could help with your application. But please consider
> working on other things.
I understand and will work on other things.
> > There is only one change in the new testing approach. In the previous
> > testing method, a new repo gets initialized for the test according to
> > GIT_TEST_DEFAULT_HASH algorithm.
>
> It looks like this happens in "t/test-lib-functions.sh", right?
> Telling a bit more about how and where that happens might help
> reviewers who would like to take a look.
Yeah, that is happens in "t/test-lib-functions.sh". I will update the
commit message to describe this better.
>
> > In unit testing however, we do not
> > need to initialize the repo. We can set the length of the hexadecimal
> > strbuf according to the algorithm used directly.
>
> So is your patch doing that or not? It might be better to be explicit.
> Also if 'strbuf's are used, then is it really worth it to set their
> length in advance, instead of just letting them grow to the right
> length as we add hex to them?
I thought of it like this: If we were to just let them grow, then we
would need separate logic for reusing that strbuf or use a different
one everytime since it always grows. By separating allocation
(hex_strbuf_init) and manipulation (fill_hex_strbuf), that same strbuf
can be reused for different hex values.
But, none of the test currently need to reuse the same strbuf, so I
suppose it is better to just let it grow and even if the need arises we
can use strbuf_splice().
> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> > ---
> > [RFC]: I recently saw a series by Eric W. Biederman [1] which enables
> > the use of oid's with different hash algorithms into the same
> > oid_array safely. However, there were no tests added for this. So, I
> > am wondering if we should have a input format which allows us to
> > specify hash algo for each oid with its hex value. i.e. "sha1:55" or
> > "sha256:55", instead of just "55" and relying on GIT_TEST_DEFAULT_HASH
> > for algo. So far, I tried to imitate the existing tests but I suppose
> > this may be useful in the future if that series gets merged.
>
> The fact that there is a series touching the same area might also hint
> that it might not be the right time to work on this.
I understand.
> > diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c
> > new file mode 100644
> > index 0000000000..b4f43c025d
> > --- /dev/null
> > +++ b/t/unit-tests/t-oid-array.c
> > @@ -0,0 +1,222 @@
> > +#include "test-lib.h"
> > +#include "hex.h"
> > +#include "oid-array.h"
> > +#include "strbuf.h"
> > +
> > +#define INPUT "88", "44", "aa", "55"
> > +#define INPUT_DUP \
> > + "88", "44", "aa", "55", "88", "44", "aa", "55", "88", "44", "aa", "55"
>
> Can you reuse INPUT in INPUT_DUP?
Yeah, that would be more clearer.
> > +#define INPUT_ONLY_DUP "55", "55"
> > +#define ENUMERATION_RESULT_SORTED "44", "55", "88", "aa"
> > +
> > +/*
> > + * allocates the memory based on the hash algorithm used and sets the length to
> > + * it.
> > + */
> > +static void hex_strbuf_init(struct strbuf *hex)
> > +{
> > + static int sz = -1;
> > +
> > + if (sz == -1) {
> > + char *algo_env = getenv("GIT_TEST_DEFAULT_HASH");
> > + if (algo_env && !strcmp(algo_env, "sha256"))
> > + sz = GIT_SHA256_HEXSZ;
> > + else
> > + sz = GIT_SHA1_HEXSZ;
> > + }
> > +
> > + strbuf_init(hex, sz);
> > + strbuf_setlen(hex, sz);
> > +}
>
> A strbuf can grow when we add stuff to it. We don't need to know its
> size in advance. So I am not sure this function is actually useful.
Yeah, this was mainly for deciding the hash algorithm but that logic can
be moved to fill_hex_strbuf.
> > +/* fills the hex strbuf with alternating characters from 'c' */
> > +static void fill_hex_strbuf(struct strbuf *hex, char *c)
> > +{
> > + size_t i;
> > + for (i = 0; i < hex->len; i++)
> > + hex->buf[i] = (i & 1) ? c[1] : c[0];
>
> There is strbuf_addch() to add a single char to a strbuf, or
> strbuf_add() and strbuf_addstr() to add many chars at once.
Will update it.
Thank you for the feedback and review.
^ permalink raw reply
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: phillip.wood123 @ 2024-02-26 19:45 UTC (permalink / raw)
To: rsbecker, phillip.wood, 'Torsten Bögershausen'
Cc: git, 'Patrick Steinhardt'
In-Reply-To: <025301da68e6$68956940$39c03bc0$@nexbridge.com>
On 26/02/2024 19:02, rsbecker@nexbridge.com wrote:
> On Monday, February 26, 2024 11:00 AM, Phillip Wood wrote:
>> On 26/02/2024 15:32, Phillip Wood wrote:
>>> Hi Randal
>>>
>>> [cc'ing Patrick for the reftable writer]
>>>
>>> On 25/02/2024 20:36, rsbecker@nexbridge.com wrote:
>>>> On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>>>>> On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>>>>>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>>>>>>> To: git@vger.kernel.org
>>>>> But I think that this should be used:
>>>>> write_in_full()
>>>>
>>>> My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
>>>> although a bit redundant since xwrite() does similar things and is
>>>> used by write_in_full().
>>>
>>> Note that unlike write_in_full(), xwrite() does not guarantee to write
>>> the whole buffer passed to it. In general unless a caller is writing a
>>> single byte or writing less than PIPE_BUF bytes to a pipe it should
>>> use write_in_full().
>>>
>>>> The question is which call is bad? The cruft stuff is relatively new
>>>> and I don't know the code.
>>
>> I should have been clearer that I do not think any of these calls are the likely
>> problem for the cruft pack code. I do think the reftable writers are worth looking at
>> though for git in general.
>>
>> For the cruft pack problem you might want to look for suspect xwrite() calls where
>> the caller does not handle a short write correctly for example under builtin/ we have
>>
>> builtin/index-pack.c: err = xwrite(1, input_buffer +
>> input_offset, input_len);
>> builtin/receive-pack.c: xwrite(2, msg, sz);
>> builtin/repack.c: xwrite(cmd->in, oid_to_hex(oid),
>> the_hash_algo->hexsz);
>> builtin/repack.c: xwrite(cmd->in, "\n", 1);
>> builtin/unpack-objects.c: int ret = xwrite(1, buffer +
>> offset, len);
>>
>> Best Wishes
>>
>> Phillip
>>
>>>>>> reftable/writer.c: int n = w->write(w->write_arg,
>>>>>> zeroed,
>>>>>> w->pending_padding);
>>>>>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>>>
>>> Neither of these appear to check for short writes and
>>> reftable_fd_write() is a thin wrapper around write(). Maybe
>>> reftable_fd_write() should be using write_in_full()?
>>>
>>>>>> run-command.c: len = write(io->fd, io->u.out.buf,
>>>
>>> This call to write() looks correct as it is in the io pump loop.
>>>
>>>>>> t/helper/test-path-utils.c: if (write(1,
>>>>>> buffer,
>>>> count)
>>>>>> < 0) >>> t/helper/test-windows-named-pipe.c: write(1,
>>>>>> buf, nbr);
>>>>>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>>>
>>> In principle these all look like they are prone to short writes.
>>>
>>>>>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf,
>>>>>> buf_line->len);
>>>
>>> This caller explicitly says it prefers short writes over retrying
>
> Replacing xwrite with write_in_full the above worked correctly. Do you want it or should I write this up?
I'm glad that worked for you. If you are able to write it up that would
be great. There are some dodgy looking xwrite() calls outside builtin/
as well. It is probably worth checking the return value of
write_in_full() when you do the conversion for the sites where we ignore
errors at the moment.
Best Wishes
Phillip
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a3a37bd215..f80b8d101a 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
> * the last part of the input buffer to stdout.
> */
> while (input_len) {
> - err = xwrite(1, input_buffer + input_offset, input_len);
> + err = write_in_full(1, input_buffer + input_offset, input_len);
> if (err <= 0)
> break;
> input_len -= err;
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index db65607485..4277c63d08 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -456,7 +456,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
> if (use_sideband)
> send_sideband(1, 2, msg, sz, use_sideband);
> else
> - xwrite(2, msg, sz);
> + write_in_full(2, msg, sz);
> }
>
> __attribute__((format (printf, 1, 2)))
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ede36328a3..4916870992 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -314,8 +314,8 @@ static int write_oid(const struct object_id *oid,
> die(_("could not start pack-objects to repack promisor objects"));
> }
>
> - xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> - xwrite(cmd->in, "\n", 1);
> + write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> + write_in_full(cmd->in, "\n", 1);
> return 0;
> }
>
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index e0a701f2b3..6935c4574e 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>
> /* Write the last part of the buffer to stdout */
> while (len) {
> - int ret = xwrite(1, buffer + offset, len);
> + int ret = write_in_full(1, buffer + offset, len);
> if (ret <= 0)
> break;
> len -= ret;
>
>
^ permalink raw reply
* Re: [PATCH v2 03/11] Start reporting missing commits in `repo_in_merge_bases_many()`
From: Junio C Hamano @ 2024-02-26 20:01 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <c27015da-0867-f417-4cd6-f4eaad3b66a9@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I have an idea. How about pulling out this hunk into its own patch? And
> insert it between 2/11 and 3/11? That would probably make most sense, as
> it would make the patch series still (relatively) easy to review, and it
> would not conflate the purpose of this hunk with the rest of 3/11's hunks.
>
> What do you think?
It does not solve the primary issue that [2/11] feels incomplete and
makes readers wonder what the new parameter that is unused is about.
So unless you are squashing that small bit into [2/11], I do not think
such a churn is worth that much.
^ permalink raw reply
* Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()
From: René Scharfe @ 2024-02-26 20:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Patrick Steinhardt
In-Reply-To: <xmqq1q8zduas.fsf@gitster.g>
Am 26.02.24 um 18:11 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> - !strncmp(rs->items[i].src,
>>> - ref_namespace[NAMESPACE_TAGS].ref,
>>> - strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {
>>> + starts_with(rs->items[i].src,
>>> + ref_namespace[NAMESPACE_TAGS].ref))) {
>>
>> The original tries to check that "namespace" fully matches the
>> initial part of .src string, which is exactly what starts_with()
>> does. Makes sense.
>
> There are two more such instances in the codebase, easily found with
>
> $ cat >contrib/coccinelle/starts_with.cocci <<\-EOF
> @@
> expression string, prefix;
> @@
> - !strncmp(string, prefix, strlen(prefix))
> + starts_with(string, prefix)
>
> @@
> expression string, prefix;
> @@
> - strncmp(string, prefix, strlen(prefix))
> + !starts_with(string, prefix)
> EOF
> $ make contrib/coccinelle/starts_with.cocci.patch
>
> which finds the one you just fixed (naturally, since the Cocci patch
> was written by taking inspiration from your fix).
>
> Here is one in the reftable code. The other one is in xdiff/ I'd
> rather not touch (as starts_with() is probably not available there).
Indeed, starts_with() is not available in xdiff/xpatience.c. That whole
file is a Git-specific extension to libxdiff, though. We could add an
include or copy the function definition.
It might make sense for performance reasons alone if there are lots of
anchors or they are very long, as with starts_with() we no longer would
have to call strlen() on all of them for each line to diff. Never used
the anchored diff algorithm, though, so I don't know whether that's a
problem in practice.
>
> diff -u -p a/reftable/refname.c b/reftable/refname.c
> --- a/reftable/refname.c
> +++ b/reftable/refname.c
> @@ -103,7 +103,7 @@ static int modification_has_ref_with_pre
> }
> }
>
> - if (strncmp(ref.refname, prefix, strlen(prefix))) {
> + if (!starts_with(ref.refname, prefix)) {
> err = 1;
> goto done;
> }
That file has another one with reversed argument order:
diff --git a/reftable/refname.c b/reftable/refname.c
index 7570e4acf9..10fc8b872d 100644
--- a/reftable/refname.c
+++ b/reftable/refname.c
@@ -78,8 +78,7 @@ static int modification_has_ref_with_prefix(struct modification *mod,
.want = prefix,
};
int idx = binsearch(mod->add_len, find_name, &arg);
- if (idx < mod->add_len &&
- !strncmp(prefix, mod->add[idx], strlen(prefix)))
+ if (idx < mod->add_len && starts_with(mod->add[idx], prefix))
goto done;
}
err = reftable_table_seek_ref(&mod->tab, &it, prefix);
^ permalink raw reply related
* Re: [PATCH v2 14/16] fsmonitor: support case-insensitive events
From: Jeff Hostetler @ 2024-02-26 20:41 UTC (permalink / raw)
To: Junio C Hamano, Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler
In-Reply-To: <xmqqjzmvt5d3.fsf@gitster.g>
On 2/23/24 1:14 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +/*
>> + * Use the name-hash to do a case-insensitive cache-entry lookup with
>> + * the pathname and invalidate the cache-entry.
>> + *
>> + * Returns the number of cache-entries that we invalidated.
>> + */
>> +static size_t handle_using_name_hash_icase(
>> + struct index_state *istate, const char *name)
>> +{
>> + struct cache_entry *ce = NULL;
>> +
>> + ce = index_file_exists(istate, name, strlen(name), 1);
>> + if (!ce)
>> + return 0;
>> +
>> + /*
>> + * A case-insensitive search in the name-hash using the
>> + * observed pathname found a cache-entry, so the observed path
>> + * is case-incorrect. Invalidate the cache-entry and use the
>> + * correct spelling from the cache-entry to invalidate the
>> + * untracked-cache. Since we now have sparse-directories in
>> + * the index, the observed pathname may represent a regular
>> + * file or a sparse-index directory.
>> + *
>> + * Note that we should not have seen FSEvents for a
>> + * sparse-index directory, but we handle it just in case.
>> + *
>> + * Either way, we know that there are not any cache-entries for
>> + * children inside the cone of the directory, so we don't need to
>> + * do the usual scan.
>> + */
>> + trace_printf_key(&trace_fsmonitor,
>> + "fsmonitor_refresh_callback MAP: '%s' '%s'",
>> + name, ce->name);
>> +
>> + untracked_cache_invalidate_trimmed_path(istate, ce->name, 0);
>> + ce->ce_flags &= ~CE_FSMONITOR_VALID;
>> + return 1;
>> +}
>
> You first ask the name-hash to turn the incoming "name" into the
> case variant that we know about, i.e. ce->name, and use that to
> access the untracked cache. Clever and makes sense. But if we have
> ce->name, doesn't it mean the name is tracked? Do we find anything
> useful to do in the untracked cache invalidation codepath in that
> case?
>
> An FSmonitor event with case-incorrect pathname for a directory may
> not be this trivial, I presume, and I expect that is what the
> remainder of this patch is about.
We're going to use "handle_using_name_hash_icase()" to lookup both
qualified (with trailing slash provided by the daemon) paths and
unqualified paths (either a file or a directory on a platform that
can't tell), so there are 3 cases to worry about.
If we fail to find a cache-entry in the name-hash, we know nothing
about the path and we still have the three cases to worry about and
we let the caller deal with that.
If we DO find a matching cache-entry, then it is either a tracked
file or one of the new sparse-directories cache-entries. We now know
the correct case-spelling. I don't think it is possible for the UC
to have an entry for this spelling, so you're right, we may not need
to explicitly invalidate the UC here. I'll add a comment to the code
about this.
>
>> +
>> +/*
>> + * Use the dir-name-hash to find the correct-case spelling of the
>> + * directory. Use the canonical spelling to invalidate all of the
>> + * cache-entries within the matching cone.
>> + *
>> + * Returns the number of cache-entries that we invalidated.
>> + */
>> +static size_t handle_using_dir_name_hash_icase(
>> + struct index_state *istate, const char *name)
>
> It is a bit unfortunate that here on the name-hash side we contrast
> the two helper function variants as "dir-name" vs "name", while the
> original handle_path side use "without_slash" vs "with_slash".
>
> If I understand correctly, it is not like there are two distinct
> hashes, "name-hash" vs "dir-name-hash". Both of these helpers use
> the same "name-hash" mechanism, and this function differs from the
> previous one in that it is about a directory, which is why it has
> "dir" in its name. I wonder if we renamed the other one with
> "nondir" in its name, and the other without_slash and with_slash
> pair to match, e.g., handle_nondir_path() vs handle_dir_path(), or
> something like that, the resulting names for these four functions
> become easier to contrast and understand?
name-hash.[ch] has 2 distinct hash-maps inside it. The "name-hash"
that we typically think about. And a well-hidden "dir-name-hash"
in the same source file.
The "name-hash" maps each cache-entry's pathname to its ce* in
the cache-entry[] (case-insensitively).
The "dir-name-hash" maps each unique directory prefix over all
of the cache-entries to the case-correct prefix. That is, if the
index contains "dir1/Dir2/DIR3/file1" and "dir1/dir4/file2", the
dir hash will have 4 entries
{ "dir1", "dir1/Dir2", "dir1/Dir2/DIR3", "dir1/dir4" }.
This lets us do lookups without having to do a linear search on
the entire cache-entry[] every time.
These 2 hashes are demand-loaded only when needed (and usually only
when ignore_case is set IIRC).
When "handle_using_dir_name_hash_icase()" is called we still don't
know if the pathname is actually a file or directory, all we know
is that we did not find a case-sensitive exact match nor a
case-insensitive match against the cache-entry[] using the name-hash.
The pathname could be a (unqualified) directory or just a plain
untracked file. So here, if we find it in the dir-name-hash, we now
know that it is a directory and that there was a case-error and we now
know the directory's correct case-spelling.
So we use that discovered case-correct spelling to invalidate the
untracked-cache.
>
>> +{
>> + struct strbuf canonical_path = STRBUF_INIT;
>> + int pos;
>> + size_t len = strlen(name);
>> + size_t nr_in_cone;
>> +
>> + if (name[len - 1] == '/')
>> + len--;
>> +
>> + if (!index_dir_find(istate, name, len, &canonical_path))
>> + return 0; /* name is untracked */
>> +
>> + if (!memcmp(name, canonical_path.buf, canonical_path.len)) {
>> + strbuf_release(&canonical_path);
>> + /*
>> + * NEEDSWORK: Our caller already tried an exact match
>> + * and failed to find one. They called us to do an
>> + * ICASE match, so we should never get an exact match,
>> + * so we could promote this to a BUG() here if we
>> + * wanted to. It doesn't hurt anything to just return
>> + * 0 and go on becaus we should never get here. Or we
>> + * could just get rid of the memcmp() and this "if"
>> + * clause completely.
>> + */
>> + return 0; /* should not happen */
>> + }
>
> "becaus" -> "because".
>
> If we should never get here, having BUG("we should never get here")
> would not hurt anything, either. On the other hand, silently
> returning 0 will hide the bug under the carpet, and I am not sure it
> is fair to call it "doesn't hurt anything".
I'll make it a BUG().
>
>> +
>> + trace_printf_key(&trace_fsmonitor,
>> + "fsmonitor_refresh_callback MAP: '%s' '%s'",
>> + name, canonical_path.buf);
>> +
>> + /*
>> + * The dir-name-hash only tells us the corrected spelling of
>> + * the prefix. We have to use this canonical path to do a
>> + * lookup in the cache-entry array so that we repeat the
>> + * original search using the case-corrected spelling.
>> + */
>> + strbuf_addch(&canonical_path, '/');
>> + pos = index_name_pos(istate, canonical_path.buf,
>> + canonical_path.len);
>> + nr_in_cone = handle_path_with_trailing_slash(
>> + istate, canonical_path.buf, pos);
>> + strbuf_release(&canonical_path);
>> + return nr_in_cone;
>> +}
>
> Nice. Do we need to give this corrected name to help untracked
> cache invalidation from the caller that called us?
In an earlier commit, I moved the call to invalidate the untracked-cache
into the two handle_path_with[out]_trailing_slash() functions so that
we wouldn't have to worry about it here.
>
>> @@ -319,6 +416,19 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>> else
>> nr_in_cone = handle_path_without_trailing_slash(istate, name, pos);
>>
>> + /*
>> + * If we did not find an exact match for this pathname or any
>> + * cache-entries with this directory prefix and we're on a
>> + * case-insensitive file system, try again using the name-hash
>> + * and dir-name-hash.
>> + */
>> + if (!nr_in_cone && ignore_case) {
>> + nr_in_cone = handle_using_name_hash_icase(istate, name);
>> + if (!nr_in_cone)
>> + nr_in_cone = handle_using_dir_name_hash_icase(
>> + istate, name);
>> + }
>
> It might be interesting to learn how often we go through these
> "fallback" code paths by tracing. Maybe it will become too noisy?
> I dunno.
I'm afraid it will be very noisy. On Windows and Mac we'll probably end
up falling back for anything that is untracked, unfortunately. That is,
if there is no cache-entry for "foo.obj", then we'll look for a
case-error (maybe there is a tracked "FOO.OBJ" file in the index or a
"Foo.Obj" directory), before we can say it is untracked.
I'm not happy about this (and no, I haven't had time to measure the
perf hit we'll take), but right now I'm just worried about the
correctness -- I've had several reports of stale/incomplete status
when IDE tools change file/directory case in unexpected ways....
Thanks
Jeff
^ permalink raw reply
* Re: [PATCH v2 14/16] fsmonitor: support case-insensitive events
From: Jeff Hostetler @ 2024-02-26 20:47 UTC (permalink / raw)
To: Torsten Bögershausen, Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler
In-Reply-To: <20240225131023.GC1952@tb-raspi4>
On 2/25/24 8:10 AM, Torsten Bögershausen wrote:
>> + if (!memcmp(name, canonical_path.buf, canonical_path.len)) {
>> + strbuf_release(&canonical_path);
>> + /*
>> + * NEEDSWORK: Our caller already tried an exact match
>> + * and failed to find one. They called us to do an
>> + * ICASE match, so we should never get an exact match,
>> + * so we could promote this to a BUG() here if we
>> + * wanted to. It doesn't hurt anything to just return
>> + * 0 and go on becaus we should never get here. Or we
>> + * could just get rid of the memcmp() and this "if"
>> + * clause completely.
>> + */
>> + return 0; /* should not happen */
>
> In very very theory, there may be a race-condition,
> when a directory is renamed very fast, more than once.
> I don't think, that the "it did not match exactly, but
> now it matches" is a problem.
> Question: Does it make sense to just remove this ?
> And, may be, find out that the "corrected spelling (tm)"
> of "DIR1" is not "dir1", neither "Dir1", but, exactly, "DIR1" ?
> Would that be a problem ?
>
I just meant that the dir-name-hash that we computed when
we loaded the index found an exact-case match here that
wasn't found when called index_name_pos() and the negative
"pos" didn't point to this exact-case prefix. This should
not happen.
Yeah I didn't think it should be a fatal condition, but
since it shouldn't happen, we can make it a BUG() and see.
Jeff
^ 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