patch, 2 partssome minor starts_with()/skip_prefix() cleanups

I happened to notice a recently-introduced strncmp() that could have been a starts_with(). So I grepped and found a few more, and this is the result.

  [1/2]: convert trivial uses of strncmp() to starts_with()
  [2/2]: convert trivial uses of strncmp() to skip_prefix()

 builtin/remote-ext.c | 6 ++++--
 builtin/remote-fd.c  | 2 +-
 bundle-uri.c         | 2 +-
 ref-filter.c         | 2 +-
 urlmatch.c           | 6 +++---
 ws.c                 | 7 ++++---
 6 files changed, 14 insertions(+), 11 deletions(-)

-Peff

It's more readable to use starts_with() instead of strncmp() to match a prefix, as the latter requires a manually-computed length, and has the funny "matching is zero" return value common to cmp functions. This patch converts several cases which were found with:

  git grep 'strncmp(.*, [0-9]*)'

But note that it doesn't convert all such cases. There are several where the magic length number is repeated elsewhere in the code, like:

  /* handle "buf" which isn't NUL-terminated and might be too small */
  if (len >= 3 && !strncmp(buf, "foo", 3))

or:

  /* exact match for "foo", but within a larger string */
  if (end - buf == 3 && !strncmp(buf, "foo", 3))

While it would not produce the wrong outcome to use starts_with() in these cases, we'd still be left with one instance of "3". We're better to leave them for now, as the repeated "3" makes it clear that the two are linked (there may be other refactorings that handle both, but they're out of scope for this patch).

A few things to note while reading the patch:

  - all cases but one are trying to match, and so lose the extra "!".
    The case in the first hunk of urlmatch.c is not-matching, and hence
    gains a "!".

  - the case in remote-fd.c is matching the beginning of "connect foo",
    but we never look at str+8 to parse the "foo" part (which would make
    this a candidate for skip_prefix(), not starts_with()). This seems
    at first glance like a bug, but is a limitation of how remote-fd
    works.

  - the second hunk in urlmatch.c shows some cases adjacent to other
    strncmp() calls that are left. These are of the "exact match within
    a larger string" type, as described above.

Signed-off-by: Jeff King <peff@peff.net>
---
No coccinelle rule for the reason given above that we don't want to just
convert all cases.

I do think a starts_with_mem() could help some of those, but I left that for another day.

 builtin/remote-fd.c | 2 +-
 bundle-uri.c        | 2 +-
 ref-filter.c        | 2 +-
 urlmatch.c          | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

As with the previous patch, using skip_prefix() is more readable and less error-prone than a raw strncmp(), because it avoids a manually-computed length. These cases differ from the previous patch that uses starts_with() because they care about the value after the matched prefix.

We can convert these to use skip_prefix() by introducing an extra variable to hold the out-pointer.

Note in the case in ws.c that to get rid of the magic number "9" completely, we also switch out "len" for recomputing the pointer difference. These are equivalent because "len" is always "ep - string".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/remote-ext.c | 6 ++++--
 ws.c                 | 7 ++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

On Sat, Jan 07, 2023 at 08:26:45AM -0500, Jeff King wrote:

By the way, one thing I noticed about this parse_whitespace_rule() function is that it's very loose about its matching. It does:

        for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++) {
                if (strncmp(whitespace_rule_names[i].rule_name,
                            string, len))
                        continue;
		...we matched...
		break;
	}

So it will prefix-match any of the options, even if there are ambiguities. E.g.:

  git -c core.whitespace=-t show

will turn off "trailing-space", even though it would also match "tab-in-indent". It would be easy enough to fix it to require the whole name, but I wasn't sure if this prefix-matching was supposed to be a feature (it doesn't seem to be documented anywhere, though).

-Peff

Am 07.01.23 um 14:33 schrieb Jeff King:

Abbreviations are being used:

   $ git grep whitespace= .gitattributes
   .gitattributes:* whitespace=!indent,trail,space
   .gitattributes:*.[ch] whitespace=indent,trail,space diff=cpp
   .gitattributes:*.sh whitespace=indent,trail,space eol=lf

(Full names: trailing-space, space-before-tab, indent-with-non-tab.)

a9cc857ada (War on whitespace: first, a bit of retreat., 2007-11-02) added this function. Its commit message says:

   "You can specify the desired types of errors to be detected by
    listing their names (unique abbreviations are accepted)
    separated by comma."

And the part about abbreviations needing to be unique was true at that time, because the names of the only two types back then started with different characters. aeb84b05ae (core.whitespace: split trailing-space into blank-at-{eol,eof}, 2009-09-05) broke that property first.

The current behavior of picking the first match from a list with a basically random order is not ideal, but at least stable. Can we still change it? The intent seems to have been to ignore ambiguous prefix matches.

René

On Sat, Jan 07, 2023 at 10:29:28PM +0100, René Scharfe wrote:

Ah, right, I should have checked to see if _we_ are using them before guessing whether anyone else might be.

Thanks, I dug around for something like that but somehow missed it.

So yeah, we definitely want to keep this abbreviation feature working. The only question is whether we ought to detect ambiguous ones. I think something like this would work, though I wonder if is even worth bothering about. I did not even see this in the wild, but it was just a curiosity while I was adjusting something else in the function: