git/ref-filter.c, branch v2.40.2

convert trivial uses of strncmp() to starts_with()

2023-01-08T01:34:35Z

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 Signed-off-by: Junio C Hamano

Merge branch 'jk/ref-filter-error-reporting-fix'

2022-12-26T02:42:06Z

Clean-ups in error messages produced by "git for-each-ref" and friends. * jk/ref-filter-error-reporting-fix: ref-filter: convert email atom parser to use err_bad_arg() ref-filter: truncate atom names in error messages ref-filter: factor out "unrecognized %(foo) arg" errors ref-filter: factor out "%(foo) does not take arguments" errors ref-filter: reject arguments to %(HEAD)

Merge branch 'jk/unused-post-2.39'

2022-12-26T02:42:05Z

Code clean-up around unused function parameters. * jk/unused-post-2.39: userdiff: mark unused parameter in internal callback list-objects-filter: mark unused parameters in virtual functions diff: mark unused parameters in callbacks xdiff: mark unused parameter in xdl_call_hunk_func() xdiff: drop unused parameter in def_ff() ws: drop unused parameter from ws_blank_line() list-objects: drop process_gitlink() function blob: drop unused parts of parse_blob_buffer() ls-refs: use repository parameter to iterate refs

ref-filter: convert email atom parser to use err_bad_arg()

2022-12-15T00:14:09Z

The error message for a bogus argument to %(authoremail), etc, is: $ git for-each-ref --format='%(authoremail:foo)' fatal: unrecognized email option: foo Saying just "email" is a little vague; most of the other atom parsers would use the full name "%(authoremail)", but we can't do that here because the same function also handles %(taggeremail), etc. Until recently, passing atom->name was a bad idea, because it erroneously included the arguments in the atom name. But since the previous commit taught err_bad_arg() to handle this, we can now do so and get: fatal: unrecognized %(authoremail) argument: foo which is consistent with other atoms. Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano

ref-filter: truncate atom names in error messages

2022-12-15T00:14:04Z

If you pass a bogus argument to %(refname), you may end up with a message like this: $ git for-each-ref --format='%(refname:foo)' fatal: unrecognized %(refname:foo) argument: foo which is confusing. It should just say: fatal: unrecognized %(refname) argument: foo which is clearer, and is consistent with most other atom parsers. Those other parsers do not have the same problem because they pass the atom name from a string literal in the parser function. But because the parser for %(refname) also handles %(upstream) and %(push), it instead uses atom->name, which includes the arguments. The oid atom parser which handles %(tree), %(parent), etc suffers from the same problem. It seems like the cleanest fix would be for atom->name to be _just_ the name, since there's already a separate "args" field. But since that field is also used for other things, we can't change it easily (e.g., it's how we find things in the used_atoms array, and clearly %(refname) and %(refname:short) are not the same thing). Instead, we'll teach our error_bad_arg() function to stop at the first ":". This is a little hacky, as we're effectively re-parsing the name, but the format is simple enough to do this as a one-liner, and this localizes the change to the error-reporting code. We'll give the same treatment to err_no_arg(). None of its callers use this atom->name trick, but it's worth future-proofing it while we're here. Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano

ref-filter: factor out "unrecognized %(foo) arg" errors

2022-12-15T00:14:00Z

Atom parsers that take arguments generally have a catch-all for "this arg is not recognized". Most of them use the same printf template, which is good, because it makes life easier for translators. Let's pull this template into a helper function, which makes the code in the parsers shorter and avoids any possibility of differences. As with the previous commit, we'll pick an arbitrary atom to make sure the test suite covers this code. Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano

ref-filter: factor out "%(foo) does not take arguments" errors

2022-12-15T00:13:56Z

Many atom parsers give the same error message, differing only in the name of the atom. If we use "%s does not take arguments", that should make life easier for translators, as they only need to translate one string. And in doing so, we can easily pull it into a helper function to make sure they are all using the exact same string. I've added a basic test here for %(HEAD), just to make sure this code is exercised at all in the test suite. We could cover each such atom, but the effort-to-reward ratio of trying to maintain an exhaustive list doesn't seem worth it. Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano

ref-filter: reject arguments to %(HEAD)

2022-12-15T00:13:35Z

The %(HEAD) atom doesn't take any arguments, but unlike other atoms in the same boat (objecttype, deltabase, etc), it does not detect this situation and complain. Let's make it consistent with the others. Signed-off-by: Jeff King Acked-by: Taylor Blau Signed-off-by: Junio C Hamano

Merge branch 'ab/various-leak-fixes'

2022-12-14T06:55:46Z

Various leak fixes. * ab/various-leak-fixes: built-ins: use free() not UNLEAK() if trivial, rm dead code revert: fix parse_options_concat() leak cherry-pick: free "struct replay_opts" members rebase: don't leak on "--abort" connected.c: free the "struct packed_git" sequencer.c: fix "opts->strategy" leak in read_strategy_opts() ls-files: fix a --with-tree memory leak revision API: call graph_clear() in release_revisions() unpack-file: fix ancient leak in create_temp_file() built-ins & libs & helpers: add/move destructors, fix leaks dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache" read-cache.c: clear and free "sparse_checkout_patterns" commit: discard partial cache before (re-)reading it {reset,merge}: call discard_index() before returning tests: mark tests as passing with SANITIZE=leak

ls-refs: use repository parameter to iterate refs

2022-12-13T13:16:22Z

The ls_refs() function (for the v2 protocol command of the same name) takes a repository parameter (like all v2 commands), but ignores it. It should use it to access the refs. This isn't a bug in practice, since we only call this function when serving upload-pack from the main repository. But it's an awkward gotcha, and it causes -Wunused-parameter to complain. The main reason we don't use the repository parameter is that the ref iteration interface we call doesn't have a "refs_" variant that takes a ref_store. However we can easily add one. In fact, since there is only one other caller (in ref-filter.c), there is no need to maintain the non-repository wrapper; that caller can just use the_repository. It's still a long way from consistently using a repository object, but it's one small step in the right direction. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano