Subject: [PATCH v2 00/12] show-ref: introduce mode to check for ref existence

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 26 Oct 2023 11:56:16 +0200

Hi,

this is the second version of my patch series that introduces a new `git
show-ref --exists` mode to check for reference existence.

Changes compared to v1:

    - Various typo fixes in commit messages.

    - Stopped using non-constant designated initializers.

    - Renamed a varible from `matchlen` to `patternlen` to match another
      renamed variable better.

    - Improved the error message for mutually exclusive modes to be
      easier to handle for translators.

    - "--quiet" is not advertised for the pattern-based mode of
      git-show-ref(1) anymore.

    - Rephrased "error code" to "exit code" in the documentation.

Thanks for the feedback so far!

Patrick

Patrick Steinhardt (12):
  builtin/show-ref: convert pattern to a local variable
  builtin/show-ref: split up different subcommands
  builtin/show-ref: fix leaking string buffer
  builtin/show-ref: fix dead code when passing patterns
  builtin/show-ref: refactor `--exclude-existing` options
  builtin/show-ref: stop using global variable to count matches
  builtin/show-ref: stop using global vars for `show_one()`
  builtin/show-ref: refactor options for patterns subcommand
  builtin/show-ref: ensure mutual exclusiveness of subcommands
  builtin/show-ref: explicitly spell out different modes in synopsis
  builtin/show-ref: add new mode to check for reference existence
  t: use git-show-ref(1) to check for ref existence

 Documentation/git-show-ref.txt |  20 ++-
 builtin/show-ref.c             | 280 ++++++++++++++++++++++-----------
 t/t1403-show-ref.sh            |  70 +++++++++
 t/t1430-bad-ref-name.sh        |  27 ++--
 t/t3200-branch.sh              |  33 ++--
 t/t5521-pull-options.sh        |   4 +-
 t/t5605-clone-local.sh         |   2 +-
 t/test-lib-functions.sh        |  55 +++++++
 8 files changed, 369 insertions(+), 122 deletions(-)

Range-diff against v1:
 1:  78163accbd2 =  1:  78163accbd2 builtin/show-ref: convert pattern to a local variable
 2:  7e6ab5dee23 !  2:  9a234622d99 builtin/show-ref: split up different subcommands
    @@ builtin/show-ref.c: static int exclude_existing(const char *match)
     +
     +static int cmd_show_ref__patterns(const char **patterns)
     +{
    -+	struct show_ref_data show_ref_data = {
    -+		.patterns = (patterns && *patterns) ? patterns : NULL,
    -+	};
    ++	struct show_ref_data show_ref_data = {0};
    ++
    ++	if (patterns && *patterns)
    ++		show_ref_data.patterns = patterns;
     +
     +	if (show_head)
     +		head_ref(show_ref, &show_ref_data);
 3:  ae2e401fbd8 =  3:  bb0d656a0b4 builtin/show-ref: fix leaking string buffer
 4:  29c0c0c6c97 !  4:  87afcee830c builtin/show-ref: fix dead code when passing patterns
    @@ Commit message
         builtin/show-ref: fix dead code when passing patterns
     
         When passing patterns to `git show-ref` we have some code that will
    -    cause us to die of `verify && !quiet` is true. But because `verify`
    +    cause us to die if `verify && !quiet` is true. But because `verify`
         indicates a different subcommand of git-show-ref(1) that causes us to
         execute `cmd_show_ref__verify()` and not `cmd_show_ref__patterns()`, the
         condition cannot ever be true.
 5:  8d0b0b5700c !  5:  bed2a8a0769 builtin/show-ref: refactor `--exclude-existing` options
    @@ Commit message
         builtin/show-ref: refactor `--exclude-existing` options
     
         It's not immediately obvious options which options are applicable to
    -    what subcommand ni git-show-ref(1) because all options exist as global
    +    what subcommand in git-show-ref(1) because all options exist as global
         state. This can easily cause confusion for the reader.
     
         Refactor options for the `--exclude-existing` subcommand to be contained
         in a separate structure. This structure is stored on the stack and
    -    passed down as required. Consequentially, it clearly delimits the scope
    -    of those options and requires the reader to worry less about global
    -    state.
    +    passed down as required. Consequently, it clearly delimits the scope of
    +    those options and requires the reader to worry less about global state.
     
         Signed-off-by: Patrick Steinhardt <[email-masked]>
     
    @@ builtin/show-ref.c: static int add_existing(const char *refname,
      	struct string_list existing_refs = STRING_LIST_INIT_DUP;
      	char buf[1024];
     -	int matchlen = match ? strlen(match) : 0;
    -+	int matchlen = opts->pattern ? strlen(opts->pattern) : 0;
    ++	int patternlen = opts->pattern ? strlen(opts->pattern) : 0;
      
      	for_each_ref(add_existing, &existing_refs);
      	while (fgets(buf, sizeof(buf), stdin)) {
    @@ builtin/show-ref.c: static int cmd_show_ref__exclude_existing(const char *match)
     -		if (match) {
     +		if (opts->pattern) {
      			int reflen = buf + len - ref;
    - 			if (reflen < matchlen)
    +-			if (reflen < matchlen)
    ++			if (reflen < patternlen)
      				continue;
     -			if (strncmp(ref, match, matchlen))
    -+			if (strncmp(ref, opts->pattern, matchlen))
    ++			if (strncmp(ref, opts->pattern, patternlen))
      				continue;
      		}
      		if (check_refname_format(ref, 0)) {
 6:  6e0f3d4e104 =  6:  d52a5e8ced2 builtin/show-ref: stop using global variable to count matches
 7:  2da1e65dd8f !  7:  63f1dadf4c2 builtin/show-ref: stop using global vars for `show_one()`
    @@ builtin/show-ref.c: static int cmd_show_ref__verify(const char **refs)
     +static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
     +				  const char **patterns)
      {
    - 	struct show_ref_data show_ref_data = {
    +-	struct show_ref_data show_ref_data = {0};
    ++	struct show_ref_data show_ref_data = {
     +		.show_one_opts = show_one_opts,
    - 		.patterns = (patterns && *patterns) ? patterns : NULL,
    - 	};
    ++	};
      
    + 	if (patterns && *patterns)
    + 		show_ref_data.patterns = patterns;
     @@ builtin/show-ref.c: static int cmd_show_ref__patterns(const char **patterns)
      
      static int hash_callback(const struct option *opt, const char *arg, int unset)
 8:  805889eda4c !  8:  88dfeaa4871 builtin/show-ref: refactor options for patterns subcommand
    @@ builtin/show-ref.c: static int cmd_show_ref__verify(const struct show_one_option
      	struct show_ref_data show_ref_data = {
      		.show_one_opts = show_one_opts,
     +		.show_head = opts->show_head,
    - 		.patterns = (patterns && *patterns) ? patterns : NULL,
      	};
      
    + 	if (patterns && *patterns)
    + 		show_ref_data.patterns = patterns;
    + 
     -	if (show_head)
     +	if (opts->show_head)
      		head_ref(show_ref, &show_ref_data);
 9:  d0a991cf4f8 !  9:  5ba566723e8 builtin/show-ref: ensure mutual exclusiveness of subcommands
    @@ builtin/show-ref.c: int cmd_show_ref(int argc, const char **argv, const char *pr
      			     show_ref_usage, 0);
      
     +	if ((!!exclude_existing_opts.enabled + !!verify) > 1)
    -+		die(_("only one of --exclude-existing or --verify can be given"));
    ++		die(_("only one of '%s' or '%s' can be given"),
    ++		    "--exclude-existing", "--verify");
     +
      	if (exclude_existing_opts.enabled)
      		return cmd_show_ref__exclude_existing(&exclude_existing_opts);
    @@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '
      
     +test_expect_success 'show-ref sub-modes are mutually exclusive' '
     +	test_must_fail git show-ref --verify --exclude-existing 2>err &&
    -+	grep "only one of --exclude-existing or --verify can be given" err
    ++	grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
     +'
     +
      test_done
10:  adcfa7a6a9d ! 10:  b78ccc5f692 builtin/show-ref: explicitly spell out different modes in synopsis
    @@ Commit message
         Split up the synopsis for these two modes such that we can disambiguate
         those differences.
     
    +    While at it, drop "--quiet" from the pattern mode's synopsis. It does
    +    not make a lot of sense to list patterns, but squelch the listing output
    +    itself. The description for "--quiet" is adapted accordingly.
    +
         Signed-off-by: Patrick Steinhardt <[email-masked]>
     
      ## Documentation/git-show-ref.txt ##
    @@ Documentation/git-show-ref.txt: git-show-ref - List references in a local reposi
      --------
      [verse]
     -'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
    -+'git show-ref' [-q | --quiet] [--head] [-d | --dereference]
    ++'git show-ref' [--head] [-d | --dereference]
      	     [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
      	     [--heads] [--] [<pattern>...]
     +'git show-ref' --verify [-q | --quiet] [-d | --dereference]
    @@ Documentation/git-show-ref.txt: git-show-ref - List references in a local reposi
      'git show-ref' --exclude-existing[=<pattern>]
      
      DESCRIPTION
    +@@ Documentation/git-show-ref.txt: OPTIONS
    + -q::
    + --quiet::
    + 
    +-	Do not print any results to stdout. When combined with `--verify`, this
    +-	can be used to silently check if a reference exists.
    ++	Do not print any results to stdout. Can be used with `--verify` to
    ++	silently check if a reference exists.
    + 
    + --exclude-existing[=<pattern>]::
    + 
     
      ## builtin/show-ref.c ##
     @@
    @@ builtin/show-ref.c
      
      static const char * const show_ref_usage[] = {
     -	N_("git show-ref [-q | --quiet] [--verify] [--head] [-d | --dereference]\n"
    -+	N_("git show-ref [-q | --quiet] [--head] [-d | --dereference]\n"
    ++	N_("git show-ref [--head] [-d | --dereference]\n"
      	   "             [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n"
      	   "             [--heads] [--] [<pattern>...]"),
     +	N_("git show-ref --verify [-q | --quiet] [-d | --dereference]\n"
11:  2f876e61dd3 ! 11:  327942b1162 builtin/show-ref: add new mode to check for reference existence
    @@ Commit message
     
             - Dangling symrefs can be resolved via git-symbolic-ref(1), but this
               requires the caller to special case existence checks depending on
    -          whteher or not a reference is symbolic or direct.
    +          whether or not a reference is symbolic or direct.
     
         Furthermore, git-rev-list(1) and other commands do not let the caller
         distinguish easily between an actually missing reference and a generic
         error.
     
    -    Taken together, this gseems like sufficient motivation to introduce a
    +    Taken together, this seems like sufficient motivation to introduce a
         separate plumbing command to explicitly check for the existence of a
         reference without trying to resolve its contents.
     
         This new command comes in the form of `git show-ref --exists`. This
         new mode will exit successfully when the reference exists, with a
    -    specific error code of 2 when it does not exist, or with 1 when there
    +    specific exit code of 2 when it does not exist, or with 1 when there
         has been a generic error.
     
         Note that the only way to properly implement this command is by using
    @@ Documentation/git-show-ref.txt: OPTIONS
      
     +--exists::
     +
    -+	Check whether the given reference exists. Returns an error code of 0 if
    -+	it does, 2 if it is missing, and 128 in case looking up the reference
    ++	Check whether the given reference exists. Returns an exit code of 0 if
    ++	it does, 2 if it is missing, and 1 in case looking up the reference
     +	failed with an error other than the reference being missing.
     +
      --abbrev[=<n>]::
    @@ builtin/show-ref.c: static int cmd_show_ref__patterns(const struct patterns_opti
     +	unsigned int unused_type;
     +	int failure_errno = 0;
     +	const char *ref;
    -+	int ret = 1;
    ++	int ret = 0;
     +
     +	if (!refs || !*refs)
     +		die("--exists requires a reference");
    @@ builtin/show-ref.c: static int cmd_show_ref__patterns(const struct patterns_opti
     +			error(_("reference does not exist"));
     +			ret = 2;
     +		} else {
    -+			error(_("failed to look up reference: %s"), strerror(failure_errno));
    ++			errno = failure_errno;
    ++			error_errno(_("failed to look up reference"));
    ++			ret = 1;
     +		}
     +
     +		goto out;
     +	}
     +
    -+	ret = 0;
    -+
     +out:
     +	strbuf_release(&unused_referent);
     +	return ret;
    @@ builtin/show-ref.c: int cmd_show_ref(int argc, const char **argv, const char *pr
      			     show_ref_usage, 0);
      
     -	if ((!!exclude_existing_opts.enabled + !!verify) > 1)
    --		die(_("only one of --exclude-existing or --verify can be given"));
    +-		die(_("only one of '%s' or '%s' can be given"),
    +-		    "--exclude-existing", "--verify");
     +	if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
    -+		die(_("only one of --exclude-existing, --exists or --verify can be given"));
    ++		die(_("only one of '%s', '%s' or '%s' can be given"),
    ++		    "--exclude-existing", "--verify", "--exists");
      
      	if (exclude_existing_opts.enabled)
      		return cmd_show_ref__exclude_existing(&exclude_existing_opts);
    @@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '
      
      test_expect_success 'show-ref sub-modes are mutually exclusive' '
     +	cat >expect <<-EOF &&
    -+	fatal: only one of --exclude-existing, --exists or --verify can be given
    ++	fatal: only one of ${SQ}--exclude-existing${SQ}, ${SQ}--verify${SQ} or ${SQ}--exists${SQ} can be given
     +	EOF
     +
      	test_must_fail git show-ref --verify --exclude-existing 2>err &&
    --	grep "only one of --exclude-existing or --verify can be given" err
    +-	grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
     +	test_cmp expect err &&
     +
     +	test_must_fail git show-ref --verify --exists 2>err &&
12:  a3a43d82e1f = 12:  226731c5f18 t: use git-show-ref(1) to check for ref existence

base-commit: a9ecda2788e229afc9b611acaa26d0d9d4da53ed
-- 
2.42.0