| Message ID | pull.1684.git.git.1709592718743.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | show-ref: add --unresolved option | expand |
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: John Cai <johncai86@gmail.com> > > For reftable development, it would be handy to have a tool to provide > the direct value of any ref whether it be a symbolic ref or not. > Currently there is git-symbolic-ref, which only works for symbolic refs, > and git-rev-parse, which will resolve the ref. Let's add a --unresolved > option that will only take one ref and return whatever it points to > without dereferencing it. The approach may be reasonble, but the above description can use some improvements. * Even though the title of the patch says show-ref, the last sentence is a bit too far from there and it was unclear to what you are adding a new feature at least to me during my first read. Let's teach show-ref a `--unresolved` optionthat will ... may make it easier to follow. * "Whatever it points to without dereferencing it" implied that it assumes what it is asked to show can be dereferenced, which invites a natural question: what happens to a thing that is not dereferenceable in the first place? The implementation seems to show either symbolic-ref target (for symbolic refs) or the object name (for others), but let's make it easier for readers. > Documentation/git-show-ref.txt | 8 ++++++ > builtin/show-ref.c | 33 ++++++++++++++++-------- > t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++ > 3 files changed, 77 insertions(+), 11 deletions(-) > > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt > index ba757470059..2f9b4de1346 100644 > --- a/Documentation/git-show-ref.txt > +++ b/Documentation/git-show-ref.txt > @@ -16,6 +16,7 @@ SYNOPSIS > [--] [<ref>...] > 'git show-ref' --exclude-existing[=<pattern>] > 'git show-ref' --exists <ref> > +'git show-ref' --unresolved <ref> > > DESCRIPTION > ----------- > @@ -76,6 +77,13 @@ OPTIONS > 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. > > +--unresolved:: > + > + Prints out what the reference points to without resolving it. 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. Exactly the same issue as in the proposed log message, i.e. what is printed for what kind of ref is not really clear. > -static int cmd_show_ref__exists(const char **refs) > +static int cmd_show_ref__raw(const char **refs, int show) > { > - struct strbuf unused_referent = STRBUF_INIT; > - struct object_id unused_oid; > - unsigned int unused_type; > + struct strbuf referent = STRBUF_INIT; > + struct object_id oid; > + unsigned int type; > int failure_errno = 0; > const char *ref; > int ret = 0; > @@ -236,7 +237,7 @@ static int cmd_show_ref__exists(const char **refs) > die("--exists requires exactly one reference"); > > if (refs_read_raw_ref(get_main_ref_store(the_repository), ref, > - &unused_oid, &unused_referent, &unused_type, > + &oid, &referent, &type, > &failure_errno)) { > if (failure_errno == ENOENT || failure_errno == EISDIR) { > error(_("reference does not exist")); > @@ -250,8 +251,16 @@ static int cmd_show_ref__exists(const char **refs) > goto out; > } > > + if (!show) > + goto out; > + > + if (type & REF_ISSYMREF) > + printf("ref: %s\n", referent.buf); > + else > + printf("ref: %s\n", oid_to_hex(&oid)); If I create a symbolic ref whose value is deadbeef....deadbeef 40-hex, I cannot tell from this output if it is a symbolic ref of a ref that stores an object whose name is that hash. Reserve the use of "ref: %s" to the symbolic refs (so that it will also match how the files backend stores them in modern Git), and use some other prefix (or no perfix). Actually, I am not sure if what is proposed is even a good interface. Given a repository with these few refs: $ git show-ref refs/heads/master b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master $ git show-ref refs/remotes/repo/HEAD b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD $ git symbolic-ref refs/remotes/repo/HEAD refs/remotes/repo/master I would think that the second command above shows the gap in feature set our current "show-ref" has. If we could do $ git show-ref --<option> refs/heads/master refs/remotes/repo/HEAD b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master ref:refs/remotes/repo/master refs/remotes/repo/HEAD or alternatively $ git show-ref --<option> refs/heads/master refs/remotes/repo/HEAD b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master ref:refs/remotes/repo/master b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD wouldn't it match the existing feature set better? You also do not have to limit yourself to single ref query per process invocation. I am not sure if you need to worry about quoting of the values of symbolic-ref, though. You _might_ need to move the (optional) symref information to the end, i.e. something like this you might prefer. I dunno. $ git show-ref --<option> refs/remotes/repo/HEAD b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD refs/remotes/repo/master I do not know what the <option> should be called, either. From an end-user's point of view, the option tells the command to also report which ref the ref points at, if it were a symbolic one. "unresolved" may be technically acceptable name to those who know the underlying implementation (i.e. we tell read_raw_ref not to resolve when it does its thing), but I am afraid that is a bit too opaque implementation detail for end-users who are expected to learn this option.
Hi John On 04/03/2024 22:51, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > For reftable development, it would be handy to have a tool to provide > the direct value of any ref whether it be a symbolic ref or not. > Currently there is git-symbolic-ref, which only works for symbolic refs, > and git-rev-parse, which will resolve the ref. Let's add a --unresolved > option that will only take one ref and return whatever it points to > without dereferencing it. "--unresolved" makes me think of merge conflicts. I wonder if "--no-dereference" would be clearer. Best Wishes Phillip > Signed-off-by: John Cai <johncai86@gmail.com> > --- > show-ref: add --unresolved option > > For reftable development, it would be handy to have a tool to provide > the direct value of any ref whether it be a symbolic ref or not. > Currently there is git-symbolic-ref, which only works for symbolic refs, > and git-rev-parse, which will resolve the ref. Let's add a --unresolved > option that will only take one ref and return whatever it points to > without dereferencing it. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1684%2Fjohn-cai%2Fjc%2Fshow-ref-direct-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1684/john-cai/jc/show-ref-direct-v1 > Pull-Request: https://github.com/git/git/pull/1684 > > Documentation/git-show-ref.txt | 8 ++++++ > builtin/show-ref.c | 33 ++++++++++++++++-------- > t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++ > 3 files changed, 77 insertions(+), 11 deletions(-) > > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt > index ba757470059..2f9b4de1346 100644 > --- a/Documentation/git-show-ref.txt > +++ b/Documentation/git-show-ref.txt > @@ -16,6 +16,7 @@ SYNOPSIS > [--] [<ref>...] > 'git show-ref' --exclude-existing[=<pattern>] > 'git show-ref' --exists <ref> > +'git show-ref' --unresolved <ref> > > DESCRIPTION > ----------- > @@ -76,6 +77,13 @@ OPTIONS > 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. > > +--unresolved:: > + > + Prints out what the reference points to without resolving it. 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>]:: > > Abbreviate the object name. When using `--hash`, you do > diff --git a/builtin/show-ref.c b/builtin/show-ref.c > index 1c15421e600..58efa078399 100644 > --- a/builtin/show-ref.c > +++ b/builtin/show-ref.c > @@ -18,6 +18,7 @@ static const char * const show_ref_usage[] = { > " [--] [<ref>...]"), > N_("git show-ref --exclude-existing[=<pattern>]"), > N_("git show-ref --exists <ref>"), > + N_("git show-ref --unresolved <ref>"), > NULL > }; > > @@ -220,11 +221,11 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts, > return 0; > } > > -static int cmd_show_ref__exists(const char **refs) > +static int cmd_show_ref__raw(const char **refs, int show) > { > - struct strbuf unused_referent = STRBUF_INIT; > - struct object_id unused_oid; > - unsigned int unused_type; > + struct strbuf referent = STRBUF_INIT; > + struct object_id oid; > + unsigned int type; > int failure_errno = 0; > const char *ref; > int ret = 0; > @@ -236,7 +237,7 @@ static int cmd_show_ref__exists(const char **refs) > die("--exists requires exactly one reference"); > > if (refs_read_raw_ref(get_main_ref_store(the_repository), ref, > - &unused_oid, &unused_referent, &unused_type, > + &oid, &referent, &type, > &failure_errno)) { > if (failure_errno == ENOENT || failure_errno == EISDIR) { > error(_("reference does not exist")); > @@ -250,8 +251,16 @@ static int cmd_show_ref__exists(const char **refs) > goto out; > } > > + if (!show) > + goto out; > + > + if (type & REF_ISSYMREF) > + printf("ref: %s\n", referent.buf); > + else > + printf("ref: %s\n", oid_to_hex(&oid)); > + > out: > - strbuf_release(&unused_referent); > + strbuf_release(&referent); > return ret; > } > > @@ -284,11 +293,12 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix) > struct exclude_existing_options exclude_existing_opts = {0}; > struct patterns_options patterns_opts = {0}; > struct show_one_options show_one_opts = {0}; > - int verify = 0, exists = 0; > + int verify = 0, exists = 0, unresolved = 0; > const struct option show_ref_options[] = { > OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")), > OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")), > OPT_BOOL(0, "exists", &exists, N_("check for reference existence without resolving")), > + OPT_BOOL(0, "unresolved", &unresolved, N_("print out unresolved value of reference")), > OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, " > "requires exact ref path")), > OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head, > @@ -314,16 +324,17 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix) > argc = parse_options(argc, argv, prefix, show_ref_options, > show_ref_usage, 0); > > - die_for_incompatible_opt3(exclude_existing_opts.enabled, "--exclude-existing", > + die_for_incompatible_opt4(exclude_existing_opts.enabled, "--exclude-existing", > verify, "--verify", > - exists, "--exists"); > + exists, "--exists", > + unresolved, "--unresolved"); > > if (exclude_existing_opts.enabled) > return cmd_show_ref__exclude_existing(&exclude_existing_opts); > else if (verify) > return cmd_show_ref__verify(&show_one_opts, argv); > - else if (exists) > - return cmd_show_ref__exists(argv); > + else if (exists || unresolved) > + return cmd_show_ref__raw(argv, unresolved); > else > return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv); > } > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh > index 33fb7a38fff..11811201738 100755 > --- a/t/t1403-show-ref.sh > +++ b/t/t1403-show-ref.sh > @@ -218,6 +218,16 @@ test_expect_success 'show-ref sub-modes are mutually exclusive' ' > test_must_fail git show-ref --exclude-existing --exists 2>err && > grep "exclude-existing" err && > grep "exists" err && > + grep "cannot be used together" err && > + > + test_must_fail git show-ref --exclude-existing --unresolved 2>err && > + grep "exclude-existing" err && > + grep "unresolved" err && > + grep "cannot be used together" err && > + > + test_must_fail git show-ref --verify --unresolved 2>err && > + grep "verify" err && > + grep "unresolved" err && > grep "cannot be used together" err > ' > > @@ -286,4 +296,41 @@ test_expect_success '--exists with existing special ref' ' > git show-ref --exists FETCH_HEAD > ' > > +test_expect_success '--unresolved with existing reference' ' > + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && > + cat >expect <<-EOF && > + ref: $commit_oid > + EOF > + git show-ref --unresolved refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--unresolved with symbolic ref' ' > + test_when_finished "git symbolic-ref -d SYMBOLIC_REF_A" && > + cat >expect <<-EOF && > + ref: refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > + EOF > + git symbolic-ref SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && > + git show-ref --unresolved SYMBOLIC_REF_A >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--unresolved with nonexistent object ID' ' > + oid=$(test_oid 002) && > + test-tool ref-store main update-ref msg refs/heads/missing-oid-2 $oid $ZERO_OID REF_SKIP_OID_VERIFICATION && > + cat >expect <<-EOF && > + ref: $oid > + EOF > + git show-ref --unresolved refs/heads/missing-oid-2 >actual && > + test_cmp expect actual > +' > + > +test_expect_success '--unresolved with nonexistent reference' ' > + cat >expect <<-EOF && > + error: reference does not exist > + EOF > + test_expect_code 2 git show-ref --unresolved refs/heads/not-exist 2>err && > + test_cmp expect err > +' > + > test_done > > base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
On Tue, Mar 5, 2024, at 16:30, Phillip Wood wrote: > Hi John > > On 04/03/2024 22:51, John Cai via GitGitGadget wrote: >> From: John Cai <johncai86@gmail.com> >> >> For reftable development, it would be handy to have a tool to provide >> the direct value of any ref whether it be a symbolic ref or not. >> Currently there is git-symbolic-ref, which only works for symbolic refs, >> and git-rev-parse, which will resolve the ref. Let's add a --unresolved >> option that will only take one ref and return whatever it points to >> without dereferencing it. > > "--unresolved" makes me think of merge conflicts. I wonder if > "--no-dereference" would be clearer. Yeah, a `--no`-style option looks more consistent.
Hi Junio, On 4 Mar 2024, at 18:23, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: John Cai <johncai86@gmail.com> >> >> For reftable development, it would be handy to have a tool to provide >> the direct value of any ref whether it be a symbolic ref or not. >> Currently there is git-symbolic-ref, which only works for symbolic refs, >> and git-rev-parse, which will resolve the ref. Let's add a --unresolved >> option that will only take one ref and return whatever it points to >> without dereferencing it. > > The approach may be reasonble, but the above description can use > some improvements. > > * Even though the title of the patch says show-ref, the last > sentence is a bit too far from there and it was unclear to what > you are adding a new feature at least to me during my first read. > > Let's teach show-ref a `--unresolved` optionthat will ... > > may make it easier to follow. > > * "Whatever it points to without dereferencing it" implied that it > assumes what it is asked to show can be dereferenced, which > invites a natural question: what happens to a thing that is not > dereferenceable in the first place? The implementation seems to > show either symbolic-ref target (for symbolic refs) or the object > name (for others), but let's make it easier for readers. Yeah good point. The language could be made more precise. > >> Documentation/git-show-ref.txt | 8 ++++++ >> builtin/show-ref.c | 33 ++++++++++++++++-------- >> t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 77 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt >> index ba757470059..2f9b4de1346 100644 >> --- a/Documentation/git-show-ref.txt >> +++ b/Documentation/git-show-ref.txt >> @@ -16,6 +16,7 @@ SYNOPSIS >> [--] [<ref>...] >> 'git show-ref' --exclude-existing[=<pattern>] >> 'git show-ref' --exists <ref> >> +'git show-ref' --unresolved <ref> >> >> DESCRIPTION >> ----------- >> @@ -76,6 +77,13 @@ OPTIONS >> 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. >> >> +--unresolved:: >> + >> + Prints out what the reference points to without resolving it. 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. > > Exactly the same issue as in the proposed log message, i.e. what is > printed for what kind of ref is not really clear. > >> -static int cmd_show_ref__exists(const char **refs) >> +static int cmd_show_ref__raw(const char **refs, int show) >> { >> - struct strbuf unused_referent = STRBUF_INIT; >> - struct object_id unused_oid; >> - unsigned int unused_type; >> + struct strbuf referent = STRBUF_INIT; >> + struct object_id oid; >> + unsigned int type; >> int failure_errno = 0; >> const char *ref; >> int ret = 0; >> @@ -236,7 +237,7 @@ static int cmd_show_ref__exists(const char **refs) >> die("--exists requires exactly one reference"); >> >> if (refs_read_raw_ref(get_main_ref_store(the_repository), ref, >> - &unused_oid, &unused_referent, &unused_type, >> + &oid, &referent, &type, >> &failure_errno)) { >> if (failure_errno == ENOENT || failure_errno == EISDIR) { >> error(_("reference does not exist")); >> @@ -250,8 +251,16 @@ static int cmd_show_ref__exists(const char **refs) >> goto out; >> } >> >> + if (!show) >> + goto out; >> + >> + if (type & REF_ISSYMREF) >> + printf("ref: %s\n", referent.buf); >> + else >> + printf("ref: %s\n", oid_to_hex(&oid)); > > If I create a symbolic ref whose value is deadbeef....deadbeef 40-hex, > I cannot tell from this output if it is a symbolic ref of a ref that > stores an object whose name is that hash. Reserve the use of "ref: %s" > to the symbolic refs (so that it will also match how the files backend > stores them in modern Git), and use some other prefix (or no > perfix). > > Actually, I am not sure if what is proposed is even a good > interface. Given a repository with these few refs: > > $ git show-ref refs/heads/master > b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master > $ git show-ref refs/remotes/repo/HEAD > b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD > $ git symbolic-ref refs/remotes/repo/HEAD > refs/remotes/repo/master > > I would think that the second command above shows the gap in feature > set our current "show-ref" has. If we could do > > $ git show-ref --<option> refs/heads/master refs/remotes/repo/HEAD > b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master > ref:refs/remotes/repo/master refs/remotes/repo/HEAD I like this option. It makes it clear that it's a symbolic ref without adding additional output to the command. cc'ing Patrick here for his thoughts as well since he has interest in this topic. > > or alternatively > > $ git show-ref --<option> refs/heads/master refs/remotes/repo/HEAD > b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master > ref:refs/remotes/repo/master b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD > > wouldn't it match the existing feature set better? You also do not > have to limit yourself to single ref query per process invocation. > > I am not sure if you need to worry about quoting of the values of > symbolic-ref, though. You _might_ need to move the (optional) > symref information to the end, i.e. something like this you might > prefer. I dunno. > > $ git show-ref --<option> refs/remotes/repo/HEAD > b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD refs/remotes/repo/master > > I do not know what the <option> should be called, either. From an > end-user's point of view, the option tells the command to also > report which ref the ref points at, if it were a symbolic one. > "unresolved" may be technically acceptable name to those who know > the underlying implementation (i.e. we tell read_raw_ref not to > resolve when it does its thing), but I am afraid that is a bit too > opaque implementation detail for end-users who are expected to learn > this option. I think something like --no-dereference that was suggested in [1] could work since the concept of dereferencing should be familiar to the user. However, this maybe confusing because of the existing --dereference flag that is specific to tags... 1. https://lore.kernel.org/git/a3de2b7b-4603-4604-a4d2-938a598e312e@gmail.com/ thanks John
John Cai <johncai86@gmail.com> writes: > I think something like --no-dereference that was suggested in [1] could work > since the concept of dereferencing should be familiar to the user. However, this > maybe confusing because of the existing --dereference flag that is specific to > tags... True. As a symbolic reference is like a symbolic link [*], another possibility is to use "follow", which often is used to refer to the action of reading the contents of a symbolic link and using it as the target pathname (e.g., O_NOFOLLOW flag forbids open(2) from following symbolic links). Unfortunately the checkbox feature "--follow" the "git log" has refers to an entirely different kind of following, so it is just as confusing as --dereference. Perhaps "--readlink", if you are showing only "ref: refs/heads/main" instead of the usual object name in the output? If we are showing both, we may want to use a name that signals the optional nature of the symref part of the output, e.g., "--with-symref-target" [*]. [Footnote] * In fact that is how the symbolic reference started out as. We literally used a symbolic link .git/HEAD that points at the current branch, which can be seen in v0.99~418 (git-init-db: set up the full default environment, 2005-05-30). * Yes, I know, I am terrible at naming things---my names may be descriptive but end up being unusably long and verbose.
On Tue, Mar 05, 2024 at 03:30:35PM +0000, Phillip Wood wrote: > Hi John > > On 04/03/2024 22:51, John Cai via GitGitGadget wrote: > > From: John Cai <johncai86@gmail.com> > > > > For reftable development, it would be handy to have a tool to provide > > the direct value of any ref whether it be a symbolic ref or not. > > Currently there is git-symbolic-ref, which only works for symbolic refs, > > and git-rev-parse, which will resolve the ref. Let's add a --unresolved > > option that will only take one ref and return whatever it points to > > without dereferencing it. > > "--unresolved" makes me think of merge conflicts. I wonder if > "--no-dereference" would be clearer. We have "--no-deref" in "git update-ref" already. It is probably better to stay consistent. -Peff
On Mon, Mar 04, 2024 at 10:51:58PM +0000, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > For reftable development, it would be handy to have a tool to provide > the direct value of any ref whether it be a symbolic ref or not. > Currently there is git-symbolic-ref, which only works for symbolic refs, > and git-rev-parse, which will resolve the ref. Let's add a --unresolved > option that will only take one ref and return whatever it points to > without dereferencing it. What about "git rev-parse --symbolic-full-name"? I don't think that behaves quite the same as your patch here: - it is actually not a true no-deref; it resolves to the final name and then prints it (so the behavior is the same for a single-level symref, but I believe a multi-level symref chain like one->two->three will print "three" when resolving "one"). - it always prints the resolved name, whereas your patch prints an oid for non-symrefs I'm not sure if those are important or not, as I don't quite understand what you're trying to accomplish. I'd probably have just run: git symbolic-ref -q $name || git rev-parse --verify $name I'm not opposed to making that more ergonomic, but I think we should avoid redundant plumbing options if we can (I'm not sure yet if this is redundant or not, but in general I find "show-ref" to be a weird mix of "rev-parse" and "for-each-ref" that I'd be just as happy if it did not exist). -Peff
Jeff King <peff@peff.net> writes: > On Tue, Mar 05, 2024 at 03:30:35PM +0000, Phillip Wood wrote: > >> Hi John >> >> On 04/03/2024 22:51, John Cai via GitGitGadget wrote: >> > From: John Cai <johncai86@gmail.com> >> > >> > For reftable development, it would be handy to have a tool to provide >> > the direct value of any ref whether it be a symbolic ref or not. >> > Currently there is git-symbolic-ref, which only works for symbolic refs, >> > and git-rev-parse, which will resolve the ref. Let's add a --unresolved >> > option that will only take one ref and return whatever it points to >> > without dereferencing it. >> >> "--unresolved" makes me think of merge conflicts. I wonder if >> "--no-dereference" would be clearer. > > We have "--no-deref" in "git update-ref" already. It is probably better > to stay consistent. That's an excellent precedent. Thanks.
On Tue, Mar 05, 2024 at 07:41:39PM -0500, Jeff King wrote: > On Mon, Mar 04, 2024 at 10:51:58PM +0000, John Cai via GitGitGadget wrote: > > > From: John Cai <johncai86@gmail.com> > > > > For reftable development, it would be handy to have a tool to provide > > the direct value of any ref whether it be a symbolic ref or not. > > Currently there is git-symbolic-ref, which only works for symbolic refs, > > and git-rev-parse, which will resolve the ref. Let's add a --unresolved > > option that will only take one ref and return whatever it points to > > without dereferencing it. > > What about "git rev-parse --symbolic-full-name"? I don't think that > behaves quite the same as your patch here: > > - it is actually not a true no-deref; it resolves to the final name > and then prints it (so the behavior is the same for a single-level > symref, but I believe a multi-level symref chain like > one->two->three will print "three" when resolving "one"). > > - it always prints the resolved name, whereas your patch prints an oid > for non-symrefs > > I'm not sure if those are important or not, as I don't quite understand > what you're trying to accomplish. I'd probably have just run: > > git symbolic-ref -q $name || git rev-parse --verify $name > > I'm not opposed to making that more ergonomic, but I think we should > avoid redundant plumbing options if we can (I'm not sure yet if this is > redundant or not, but in general I find "show-ref" to be a weird mix of > "rev-parse" and "for-each-ref" that I'd be just as happy if it did not > exist). Yeah, the proposed patch basically aims to do the above chained command in an easier way. I think that this would be a nice addition to make this easier to use and better discoverable. But in the long run what I think would be really useful is to extend git-for-each-ref(1) and/or git-show-ref(1) so that they can print all existing refs with their direct values. Right now, it's impossible to get a globally consistent view of all refs in the refdb with their unresolved values. That will end up a bit more involved though. The ref iterators we have right now do not provide any way to return symref targets to the caller, so we would have to first extend the interfaces and adapt both backends to support this. But this is kind of where I want to end up. Patrick
On Wed, Mar 06, 2024 at 08:31:15AM +0100, Patrick Steinhardt wrote: > Yeah, the proposed patch basically aims to do the above chained command > in an easier way. I think that this would be a nice addition to make > this easier to use and better discoverable. But in the long run what I > think would be really useful is to extend git-for-each-ref(1) and/or > git-show-ref(1) so that they can print all existing refs with their > direct values. Right now, it's impossible to get a globally consistent > view of all refs in the refdb with their unresolved values. Yeah, it seems like if this were a format-specifier for for-each-ref it would be a lot more flexible. You can do: git for-each-ref --format='%(refname) %(objectname) %(symref)' to get the resolved values next to the symrefs (if any). I think that does a full resolution, though (so again, if you had one->two->three, you can never learn about the intermediate "two"). > That will end up a bit more involved though. The ref iterators we have > right now do not provide any way to return symref targets to the caller, > so we would have to first extend the interfaces and adapt both backends > to support this. But this is kind of where I want to end up. I think for-each-ref in the above command works by calling resolve_refdup() itself, and then recording the result. It would be nice to get it from the iterator, though (more efficient, and avoids any races). -Peff
Le 04/03/2024 à 23:51, John Cai via GitGitGadget a écrit : > From: John Cai <johncai86@gmail.com> > > @@ -76,6 +77,13 @@ OPTIONS > 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. > > +--unresolved:: > + > + Prints out what the reference points to without resolving it. Returns Style nitpicking: we use imperative form in the descriptions of options → Print out what... Return... > + 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>]:: > > Abbreviate the object name. When using `--hash`, you do
Jeff King <peff@peff.net> writes: > You can do: > > git for-each-ref --format='%(refname) %(objectname) %(symref)' We can even do that "ref target || object name" thing with --format='%(refname) %(if)%(symref)%(then)%(symref)%(else)%(objectname)' if we wanted to. But if we have both available, I think the output that adds the symref target, if available, after the object name, is better than the output that switches between the two. > to get the resolved values next to the symrefs (if any). I think that > does a full resolution, though (so again, if you had one->two->three, > you can never learn about the intermediate "two"). Yeah, I know we discussed the usefulness of tag-of-tag-of-something, but this is a similar one. > I think for-each-ref in the above command works by calling > resolve_refdup() itself, and then recording the result. It would be nice > to get it from the iterator, though (more efficient, and avoids any > races). Indeed. Thanks for an interesting thought.
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt index ba757470059..2f9b4de1346 100644 --- a/Documentation/git-show-ref.txt +++ b/Documentation/git-show-ref.txt @@ -16,6 +16,7 @@ SYNOPSIS [--] [<ref>...] 'git show-ref' --exclude-existing[=<pattern>] 'git show-ref' --exists <ref> +'git show-ref' --unresolved <ref> DESCRIPTION ----------- @@ -76,6 +77,13 @@ OPTIONS 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. +--unresolved:: + + Prints out what the reference points to without resolving it. 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>]:: Abbreviate the object name. When using `--hash`, you do diff --git a/builtin/show-ref.c b/builtin/show-ref.c index 1c15421e600..58efa078399 100644 --- a/builtin/show-ref.c +++ b/builtin/show-ref.c @@ -18,6 +18,7 @@ static const char * const show_ref_usage[] = { " [--] [<ref>...]"), N_("git show-ref --exclude-existing[=<pattern>]"), N_("git show-ref --exists <ref>"), + N_("git show-ref --unresolved <ref>"), NULL }; @@ -220,11 +221,11 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts, return 0; } -static int cmd_show_ref__exists(const char **refs) +static int cmd_show_ref__raw(const char **refs, int show) { - struct strbuf unused_referent = STRBUF_INIT; - struct object_id unused_oid; - unsigned int unused_type; + struct strbuf referent = STRBUF_INIT; + struct object_id oid; + unsigned int type; int failure_errno = 0; const char *ref; int ret = 0; @@ -236,7 +237,7 @@ static int cmd_show_ref__exists(const char **refs) die("--exists requires exactly one reference"); if (refs_read_raw_ref(get_main_ref_store(the_repository), ref, - &unused_oid, &unused_referent, &unused_type, + &oid, &referent, &type, &failure_errno)) { if (failure_errno == ENOENT || failure_errno == EISDIR) { error(_("reference does not exist")); @@ -250,8 +251,16 @@ static int cmd_show_ref__exists(const char **refs) goto out; } + if (!show) + goto out; + + if (type & REF_ISSYMREF) + printf("ref: %s\n", referent.buf); + else + printf("ref: %s\n", oid_to_hex(&oid)); + out: - strbuf_release(&unused_referent); + strbuf_release(&referent); return ret; } @@ -284,11 +293,12 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix) struct exclude_existing_options exclude_existing_opts = {0}; struct patterns_options patterns_opts = {0}; struct show_one_options show_one_opts = {0}; - int verify = 0, exists = 0; + int verify = 0, exists = 0, unresolved = 0; const struct option show_ref_options[] = { OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")), OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")), OPT_BOOL(0, "exists", &exists, N_("check for reference existence without resolving")), + OPT_BOOL(0, "unresolved", &unresolved, N_("print out unresolved value of reference")), OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, " "requires exact ref path")), OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head, @@ -314,16 +324,17 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, show_ref_options, show_ref_usage, 0); - die_for_incompatible_opt3(exclude_existing_opts.enabled, "--exclude-existing", + die_for_incompatible_opt4(exclude_existing_opts.enabled, "--exclude-existing", verify, "--verify", - exists, "--exists"); + exists, "--exists", + unresolved, "--unresolved"); if (exclude_existing_opts.enabled) return cmd_show_ref__exclude_existing(&exclude_existing_opts); else if (verify) return cmd_show_ref__verify(&show_one_opts, argv); - else if (exists) - return cmd_show_ref__exists(argv); + else if (exists || unresolved) + return cmd_show_ref__raw(argv, unresolved); else return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv); } diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh index 33fb7a38fff..11811201738 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -218,6 +218,16 @@ test_expect_success 'show-ref sub-modes are mutually exclusive' ' test_must_fail git show-ref --exclude-existing --exists 2>err && grep "exclude-existing" err && grep "exists" err && + grep "cannot be used together" err && + + test_must_fail git show-ref --exclude-existing --unresolved 2>err && + grep "exclude-existing" err && + grep "unresolved" err && + grep "cannot be used together" err && + + test_must_fail git show-ref --verify --unresolved 2>err && + grep "verify" err && + grep "unresolved" err && grep "cannot be used together" err ' @@ -286,4 +296,41 @@ test_expect_success '--exists with existing special ref' ' git show-ref --exists FETCH_HEAD ' +test_expect_success '--unresolved with existing reference' ' + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + ref: $commit_oid + EOF + git show-ref --unresolved refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >actual && + test_cmp expect actual +' + +test_expect_success '--unresolved with symbolic ref' ' + test_when_finished "git symbolic-ref -d SYMBOLIC_REF_A" && + cat >expect <<-EOF && + ref: refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git symbolic-ref SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && + git show-ref --unresolved SYMBOLIC_REF_A >actual && + test_cmp expect actual +' + +test_expect_success '--unresolved with nonexistent object ID' ' + oid=$(test_oid 002) && + test-tool ref-store main update-ref msg refs/heads/missing-oid-2 $oid $ZERO_OID REF_SKIP_OID_VERIFICATION && + cat >expect <<-EOF && + ref: $oid + EOF + git show-ref --unresolved refs/heads/missing-oid-2 >actual && + test_cmp expect actual +' + +test_expect_success '--unresolved with nonexistent reference' ' + cat >expect <<-EOF && + error: reference does not exist + EOF + test_expect_code 2 git show-ref --unresolved refs/heads/not-exist 2>err && + test_cmp expect err +' + test_done