{"thread":{"id":"60988","subject":"[PATCH] fetch: convert strncmp() with strlen() to starts_with()","startedAt":"2024-02-24T21:47:08Z","lastAt":"2024-02-27T07:58:14Z","messageCount":5,"participants":["René Scharfe","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489292","messageId":"cb94b938-03f9-4dd3-84c1-f5244ca81be3@web.de","threadId":"60988","inReplyTo":null,"subject":"[PATCH] fetch: convert strncmp() with strlen() to starts_with()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-24T21:47:06Z","receivedAt":"2024-02-24T21:47:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Using strncmp() and strlen() to check whether a string starts with\nanother one requires repeating the prefix candidate.  Use starts_with()\ninstead, which reduces repetition and is more readable.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/fetch.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3aedfd1bb6..0a7a1a3476 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -448,9 +448,8 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t\t\tcontinue;\n \t\tif (!rs->items[i].dst ||\n \t\t    (rs->items[i].src &&\n-\t\t     !strncmp(rs->items[i].src,\n-\t\t\t      ref_namespace[NAMESPACE_TAGS].ref,\n-\t\t\t      strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {\n+\t\t     starts_with(rs->items[i].src,\n+\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n \t\t\tint j;\n\n \t\t\tfree(rs->items[i].src);\n--\n2.44.0\n"},{"id":"489298","messageId":"xmqqfrxhjujr.fsf@gitster.g","threadId":"60988","inReplyTo":"cb94b938-03f9-4dd3-84c1-f5244ca81be3@web.de","subject":"Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-24T23:45:28Z","receivedAt":"2024-02-24T23:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Using strncmp() and strlen() to check whether a string starts with\n> another one requires repeating the prefix candidate.  Use starts_with()\n> instead, which reduces repetition and is more readable.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  builtin/fetch.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 3aedfd1bb6..0a7a1a3476 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -448,9 +448,8 @@ static void filter_prefetch_refspec(struct refspec *rs)\n>  \t\t\tcontinue;\n>  \t\tif (!rs->items[i].dst ||\n>  \t\t    (rs->items[i].src &&\n> -\t\t     !strncmp(rs->items[i].src,\n> -\t\t\t      ref_namespace[NAMESPACE_TAGS].ref,\n> -\t\t\t      strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {\n> +\t\t     starts_with(rs->items[i].src,\n> +\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n\nThe original tries to check that \"namespace\" fully matches the\ninitial part of .src string, which is exactly what starts_with()\ndoes.  Makes sense.\n\n>  \t\t\tint j;\n>\n>  \t\t\tfree(rs->items[i].src);\n> --\n> 2.44.0\n"},{"id":"489387","messageId":"xmqq1q8zduas.fsf@gitster.g","threadId":"60988","inReplyTo":"xmqqfrxhjujr.fsf@gitster.g","subject":"Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T17:11:55Z","receivedAt":"2024-02-26T17:12:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> -\t\t     !strncmp(rs->items[i].src,\n>> -\t\t\t      ref_namespace[NAMESPACE_TAGS].ref,\n>> -\t\t\t      strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {\n>> +\t\t     starts_with(rs->items[i].src,\n>> +\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n>\n> The original tries to check that \"namespace\" fully matches the\n> initial part of .src string, which is exactly what starts_with()\n> does.  Makes sense.\n\nThere are two more such instances in the codebase, easily found with\n\n        $ cat >contrib/coccinelle/starts_with.cocci <<\\-EOF\n        @@\n        expression string, prefix;\n        @@\n        - !strncmp(string, prefix, strlen(prefix))\n        + starts_with(string, prefix)\n\n        @@\n        expression string, prefix;\n        @@\n        - strncmp(string, prefix, strlen(prefix))\n        + !starts_with(string, prefix)\n        EOF\n        $ make contrib/coccinelle/starts_with.cocci.patch\n\nwhich finds the one you just fixed (naturally, since the Cocci patch\nwas written by taking inspiration from your fix).\n\nHere is one in the reftable code.  The other one is in xdiff/ I'd\nrather not touch (as starts_with() is probably not available there).\n\ndiff -u -p a/reftable/refname.c b/reftable/refname.c\n--- a/reftable/refname.c\n+++ b/reftable/refname.c\n@@ -103,7 +103,7 @@ static int modification_has_ref_with_pre\n \t\t\t}\n \t\t}\n \n-\t\tif (strncmp(ref.refname, prefix, strlen(prefix))) {\n+\t\tif (!starts_with(ref.refname, prefix)) {\n \t\t\terr = 1;\n \t\t\tgoto done;\n \t\t}\n"},{"id":"489415","messageId":"08db9346-d10d-4b32-ae55-3ee5a8adf89d@web.de","threadId":"60988","inReplyTo":"xmqq1q8zduas.fsf@gitster.g","subject":"Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-26T20:04:27Z","receivedAt":"2024-02-26T20:04:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.02.24 um 18:11 schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>> -\t\t     !strncmp(rs->items[i].src,\n>>> -\t\t\t      ref_namespace[NAMESPACE_TAGS].ref,\n>>> -\t\t\t      strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {\n>>> +\t\t     starts_with(rs->items[i].src,\n>>> +\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n>>\n>> The original tries to check that \"namespace\" fully matches the\n>> initial part of .src string, which is exactly what starts_with()\n>> does.  Makes sense.\n>\n> There are two more such instances in the codebase, easily found with\n>\n>         $ cat >contrib/coccinelle/starts_with.cocci <<\\-EOF\n>         @@\n>         expression string, prefix;\n>         @@\n>         - !strncmp(string, prefix, strlen(prefix))\n>         + starts_with(string, prefix)\n>\n>         @@\n>         expression string, prefix;\n>         @@\n>         - strncmp(string, prefix, strlen(prefix))\n>         + !starts_with(string, prefix)\n>         EOF\n>         $ make contrib/coccinelle/starts_with.cocci.patch\n>\n> which finds the one you just fixed (naturally, since the Cocci patch\n> was written by taking inspiration from your fix).\n>\n> Here is one in the reftable code.  The other one is in xdiff/ I'd\n> rather not touch (as starts_with() is probably not available there).\n\nIndeed, starts_with() is not available in xdiff/xpatience.c.  That whole\nfile is a Git-specific extension to libxdiff, though.  We could add an\ninclude or copy the function definition.\n\nIt might make sense for performance reasons alone if there are lots of\nanchors or they are very long, as with starts_with() we no longer would\nhave to call strlen() on all of them for each line to diff.  Never used\nthe anchored diff algorithm, though, so I don't know whether that's a\nproblem in practice.\n\n>\n> diff -u -p a/reftable/refname.c b/reftable/refname.c\n> --- a/reftable/refname.c\n> +++ b/reftable/refname.c\n> @@ -103,7 +103,7 @@ static int modification_has_ref_with_pre\n>  \t\t\t}\n>  \t\t}\n>\n> -\t\tif (strncmp(ref.refname, prefix, strlen(prefix))) {\n> +\t\tif (!starts_with(ref.refname, prefix)) {\n>  \t\t\terr = 1;\n>  \t\t\tgoto done;\n>  \t\t}\n\n\nThat file has another one with reversed argument order:\n\ndiff --git a/reftable/refname.c b/reftable/refname.c\nindex 7570e4acf9..10fc8b872d 100644\n--- a/reftable/refname.c\n+++ b/reftable/refname.c\n@@ -78,8 +78,7 @@ static int modification_has_ref_with_prefix(struct modification *mod,\n \t\t\t.want = prefix,\n \t\t};\n \t\tint idx = binsearch(mod->add_len, find_name, &arg);\n-\t\tif (idx < mod->add_len &&\n-\t\t    !strncmp(prefix, mod->add[idx], strlen(prefix)))\n+\t\tif (idx < mod->add_len && starts_with(mod->add[idx], prefix))\n \t\t\tgoto done;\n \t}\n \terr = reftable_table_seek_ref(&mod->tab, &it, prefix);\n\n"},{"id":"489467","messageId":"Zd2WEeR4fBmbbZFo@tanuki","threadId":"60988","inReplyTo":"08db9346-d10d-4b32-ae55-3ee5a8adf89d@web.de","subject":"Re: [PATCH] fetch: convert strncmp() with strlen() to starts_with()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-27T07:58:09Z","receivedAt":"2024-02-27T07:58:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 26, 2024 at 09:04:27PM +0100, René Scharfe wrote:\n> Am 26.02.24 um 18:11 schrieb Junio C Hamano:\n> > Junio C Hamano <gitster@pobox.com> writes:\n> >\n> >>> -\t\t     !strncmp(rs->items[i].src,\n> >>> -\t\t\t      ref_namespace[NAMESPACE_TAGS].ref,\n> >>> -\t\t\t      strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {\n> >>> +\t\t     starts_with(rs->items[i].src,\n> >>> +\t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n> >>\n> >> The original tries to check that \"namespace\" fully matches the\n> >> initial part of .src string, which is exactly what starts_with()\n> >> does.  Makes sense.\n> >\n> > There are two more such instances in the codebase, easily found with\n> >\n> >         $ cat >contrib/coccinelle/starts_with.cocci <<\\-EOF\n> >         @@\n> >         expression string, prefix;\n> >         @@\n> >         - !strncmp(string, prefix, strlen(prefix))\n> >         + starts_with(string, prefix)\n> >\n> >         @@\n> >         expression string, prefix;\n> >         @@\n> >         - strncmp(string, prefix, strlen(prefix))\n> >         + !starts_with(string, prefix)\n> >         EOF\n> >         $ make contrib/coccinelle/starts_with.cocci.patch\n> >\n> > which finds the one you just fixed (naturally, since the Cocci patch\n> > was written by taking inspiration from your fix).\n> >\n> > Here is one in the reftable code.  The other one is in xdiff/ I'd\n> > rather not touch (as starts_with() is probably not available there).\n> \n> Indeed, starts_with() is not available in xdiff/xpatience.c.  That whole\n> file is a Git-specific extension to libxdiff, though.  We could add an\n> include or copy the function definition.\n\nIt's kind of the same for the reftable library code. It's in this weird\nin-between state where it's supposed to be usable as a library, but it\nalready does use some of Git's infra. I would personally be happy to use\nmore bits of the Git library in the reftable library, but we do need to\nacknowledge that this makes it harder for other projects to use the\ncode.\n\nSo we should think about how we want to proceed here:\n\n  - Is the reftable library now a part of the Git codebase that can use\n    all of the features that we already have?\n\n  - Is the reftable library an independent part that has the goal of\n    being reusable for other projects?\n\nWith the ongoing libification project I certainly think that the first\noption becomes more viable, as we can essentially have the best of both\nworlds. The reftable library can be reused by other projects, and at the\nsame time we can use our own internals. But it's still going to take\nquite some time until that can be fully realized, I assume.\n\nPatrick\n\n> It might make sense for performance reasons alone if there are lots of\n> anchors or they are very long, as with starts_with() we no longer would\n> have to call strlen() on all of them for each line to diff.  Never used\n> the anchored diff algorithm, though, so I don't know whether that's a\n> problem in practice.\n> \n> >\n> > diff -u -p a/reftable/refname.c b/reftable/refname.c\n> > --- a/reftable/refname.c\n> > +++ b/reftable/refname.c\n> > @@ -103,7 +103,7 @@ static int modification_has_ref_with_pre\n> >  \t\t\t}\n> >  \t\t}\n> >\n> > -\t\tif (strncmp(ref.refname, prefix, strlen(prefix))) {\n> > +\t\tif (!starts_with(ref.refname, prefix)) {\n> >  \t\t\terr = 1;\n> >  \t\t\tgoto done;\n> >  \t\t}\n> \n> \n> That file has another one with reversed argument order:\n> \n> diff --git a/reftable/refname.c b/reftable/refname.c\n> index 7570e4acf9..10fc8b872d 100644\n> --- a/reftable/refname.c\n> +++ b/reftable/refname.c\n> @@ -78,8 +78,7 @@ static int modification_has_ref_with_prefix(struct modification *mod,\n>  \t\t\t.want = prefix,\n>  \t\t};\n>  \t\tint idx = binsearch(mod->add_len, find_name, &arg);\n> -\t\tif (idx < mod->add_len &&\n> -\t\t    !strncmp(prefix, mod->add[idx], strlen(prefix)))\n> +\t\tif (idx < mod->add_len && starts_with(mod->add[idx], prefix))\n>  \t\t\tgoto done;\n>  \t}\n>  \terr = reftable_table_seek_ref(&mod->tab, &it, prefix);\n> \n"}]}