| Message ID | adcfa7a6a9d8fb6f915faf77df52362544cd590e.1698152926.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | show-ref: introduce mode to check for ref existence | expand |
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote: > The synopsis treats the `--verify` and the implicit mode the same. They > are slightly different though: > > - They accept different sets of flags. > > - The implicit mode accepts patterns while the `--verify` mode > accepts references. > > Split up the synopsis for these two modes such that we can disambiguate > those differences. Good. When reading [2/12], my immediate thought was that such a documentation change was needed. > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt > @@ -8,9 +8,12 @@ git-show-ref - List references in a local repository > SYNOPSIS > -'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference] > +'git show-ref' [-q | --quiet] [--head] [-d | --dereference] > [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags] > [--heads] [--] [<pattern>...] > +'git show-ref' --verify [-q | --quiet] [-d | --dereference] > + [-s | --hash[=<n>]] [--abbrev[=<n>]] > + [--] [<ref>...] > 'git show-ref' --exclude-existing[=<pattern>] What does it mean to request "quiet" for the plain `git show-ref` mode? That seems pointless and counterintuitive. Even though this mode may accept --quiet as a quirk of implementation, we probably shouldn't be promoting its use in the documentation. Moreover, the blurb for --quiet later in the document: Do not print any results to stdout. When combined with --verify, this can be used to silently check if a reference exists. should probably be rephrased since it currently implies that it may be used with modes other than --verify, but that's not really the case (implementation quirks aside). This also raises the question as to whether an interlock should be added to disallow --quiet with plain `git show-ref`, much like the interlock preventing --exclude-existing and --verify from being used together. Ideally, such an interlock ought to be added, but I wouldn't be surprised to learn that doing so would break someone's existing tooling which insensibly uses --quiet with plain `git show-ref`.
On Tue, Oct 24, 2023 at 03:39:28PM -0400, Eric Sunshine wrote: > On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote: > > The synopsis treats the `--verify` and the implicit mode the same. They > > are slightly different though: > > > > - They accept different sets of flags. > > > > - The implicit mode accepts patterns while the `--verify` mode > > accepts references. > > > > Split up the synopsis for these two modes such that we can disambiguate > > those differences. > > Good. When reading [2/12], my immediate thought was that such a > documentation change was needed. > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt > > @@ -8,9 +8,12 @@ git-show-ref - List references in a local repository > > SYNOPSIS > > -'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference] > > +'git show-ref' [-q | --quiet] [--head] [-d | --dereference] > > [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags] > > [--heads] [--] [<pattern>...] > > +'git show-ref' --verify [-q | --quiet] [-d | --dereference] > > + [-s | --hash[=<n>]] [--abbrev[=<n>]] > > + [--] [<ref>...] > > 'git show-ref' --exclude-existing[=<pattern>] > > What does it mean to request "quiet" for the plain `git show-ref` > mode? That seems pointless and counterintuitive. Even though this mode > may accept --quiet as a quirk of implementation, we probably shouldn't > be promoting its use in the documentation. Moreover, the blurb for > --quiet later in the document: > > Do not print any results to stdout. When combined with --verify, > this can be used to silently check if a reference exists. > > should probably be rephrased since it currently implies that it may be > used with modes other than --verify, but that's not really the case > (implementation quirks aside). Good point indeed, will change. > This also raises the question as to whether an interlock should be > added to disallow --quiet with plain `git show-ref`, much like the > interlock preventing --exclude-existing and --verify from being used > together. Ideally, such an interlock ought to be added, but I wouldn't > be surprised to learn that doing so would break someone's existing > tooling which insensibly uses --quiet with plain `git show-ref`. Yeah, I also wouldn't go as far as this. The mutual exclusiveness for `--exclude-existing` and `--verify` makes sense in my opinion because the result is extremely misleading and may cause users to assume that the wrong thing has happened. I don't think that's necessarily true for `--quiet`. It may not make a lot of sense to specify `--quiet` here, but it also doesn't quietly do the wrong thing as in the other case. Furthermore, we also don't have any interlocks for incompatible other flags, either: git-show-ref(1) won't complain when passing any of the mode-specific flags to the other modes. If we want to fix that I'd rather defer it to a follow up patch series though. And as you said, I would almost certainly expect there to be some kind of fallout if we did this change. Patrick
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt index 2fe274b8faa..ab23e0b62e1 100644 --- a/Documentation/git-show-ref.txt +++ b/Documentation/git-show-ref.txt @@ -8,9 +8,12 @@ git-show-ref - List references in a local repository SYNOPSIS -------- [verse] -'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference] +'git show-ref' [-q | --quiet] [--head] [-d | --dereference] [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags] [--heads] [--] [<pattern>...] +'git show-ref' --verify [-q | --quiet] [-d | --dereference] + [-s | --hash[=<n>]] [--abbrev[=<n>]] + [--] [<ref>...] 'git show-ref' --exclude-existing[=<pattern>] DESCRIPTION diff --git a/builtin/show-ref.c b/builtin/show-ref.c index 10d0213e687..d0a32d07404 100644 --- a/builtin/show-ref.c +++ b/builtin/show-ref.c @@ -11,9 +11,12 @@ #include "parse-options.h" 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" " [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n" " [--heads] [--] [<pattern>...]"), + N_("git show-ref --verify [-q | --quiet] [-d | --dereference]\n" + " [-s | --hash[=<n>]] [--abbrev[=<n>]]\n" + " [--] [<ref>...]"), N_("git show-ref --exclude-existing[=<pattern>]"), NULL };
The synopsis treats the `--verify` and the implicit mode the same. They are slightly different though: - They accept different sets of flags. - The implicit mode accepts patterns while the `--verify` mode accepts references. Split up the synopsis for these two modes such that we can disambiguate those differences. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-show-ref.txt | 5 ++++- builtin/show-ref.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-)