| Message ID | cover.1698314128.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | show-ref: introduce mode to check for ref existence | expand |
On Thu, Oct 26, 2023 at 11:56:16AM +0200, Patrick Steinhardt wrote: > Hi, > > this is the second version of my patch series that introduces a new `git > show-ref --exists` mode to check for reference existence. All looks quite reasonable to me. I'm happy with this series as-is, though I left a few minor comments and suggestions throughout. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Thu, Oct 26, 2023 at 11:56:16AM +0200, Patrick Steinhardt wrote: >> Hi, >> >> this is the second version of my patch series that introduces a new `git >> show-ref --exists` mode to check for reference existence. > > All looks quite reasonable to me. I'm happy with this series as-is, > though I left a few minor comments and suggestions throughout. Thanks. I'll wait for hearing from Patrick and then decide ;-)
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 <ps@pks.im> @@ 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 <ps@pks.im> ## 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