Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()
- From
René Scharfe <l.s.r@web.de>
- Date
- Feb 26, 2024, 20:04 UTC
- Message-ID
- <08db9346-d10d-4b32-ae55-3ee5a8adf89d@web.de>
- In-Reply-To
- <xmqq1q8zduas.fsf@gitster.g>
Am 26.02.24 um 18:11 schrieb Junio C Hamano:
Show 34 quoted lines
> 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.
Show 13 quoted lines
>
> 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);