From patchwork Thu Oct 26 09:56:16 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13437414 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B58B72AB25 for ; Thu, 26 Oct 2023 09:56:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="qKTfoTiJ"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="EVcqXWQ/" Received: from wout2-smtp.messagingengine.com (wout2-smtp.messagingengine.com [64.147.123.25]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 733CA198 for ; Thu, 26 Oct 2023 02:56:22 -0700 (PDT) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id A777A3200928; Thu, 26 Oct 2023 05:56:21 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Thu, 26 Oct 2023 05:56:22 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:sender :subject:subject:to:to; s=fm3; t=1698314181; x=1698400581; bh=9c 4c/7icKmNlxtu0X5KNzkJUCBDDS9uS2Ebz6WAPPII=; b=qKTfoTiJLJGWJpNQvp pbbPhzvR4iw9mKbsvRcZ66FT0kdCa15McDaflV5rUAo6p8hihoWphK7vvslE24Me 53RhpMyg6vxSNIfahvTWMq14AGUxZ3SMdf4VhCUUdTjhAKNisH87iw6PYe2Swd6b KuG1jDqf8huMroWFKauvwggI5z7tYj0mZqwWIQ4/MYd1cXdAMJZXBs23pbATjfnl Km2IkPAPqhmcSBrmGg2VgZzEl03zsjV+GYJHQ70xBCKf2qt5xroI+CQiEJd0bbW+ i1RZtntvoGMGctert+ceWIU5JPH5FyFhGc4mbOjVD2Brl0YTyF4+Wuz+wFo2gQWu pCwQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; t=1698314181; x=1698400581; bh=9c4c/7icKmNlx tu0X5KNzkJUCBDDS9uS2Ebz6WAPPII=; b=EVcqXWQ/yWQeVyzV1vx2mVvB+gysb KNxJLHhdaTfk0gd5UiUdXEjtor7Yt/EzjYhkyo1gv54qJVb4MksbpHtMhK47pBjB DLEMvpM8VqS0Xvf43O1WPWOjcwreCo6+F13TfWg1YjHRdUJUBYLsX9B4gy7qiBBp QFZQxy8fPOsmO89Q8BHaq/OM1j5Iw80+wHVVpSmPlEy60LxGmHl35IBFPIHIT3ty USNO5wIdhZaO0gZJu5IJFVQNR+U+dxibGxO+LVd0VtLuepdJ9akKQ1FGEW1YDHgF Cc8lY5NN23hdv1Xk9T3doQpYHd+4J5g0YLD3b2u7wdK/bS5OwwR2VIuQQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrledvgddvudcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeeukedtvedtffevleejtefgheehieegkeeluddvfeefgeehgfeltddtheejleffteen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhm X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 26 Oct 2023 05:56:20 -0400 (EDT) Received: by vm-mail (OpenSMTPD) with ESMTPSA id bbb56984 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Thu, 26 Oct 2023 09:56:10 +0000 (UTC) Date: Thu, 26 Oct 2023 11:56:16 +0200 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Junio C Hamano , Eric Sunshine , Han-Wen Nienhuys Subject: [PATCH v2 00/12] show-ref: introduce mode to check for ref existence Message-ID: References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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 @@ 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 ## 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[=]] [--abbrev[=]] [--tags] [--heads] [--] [...] +'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[=] 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[=]:: + ## 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[=]] [--abbrev[=]] [--tags]\n" " [--heads] [--] [...]"), + 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[=]:: @@ 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