show-ref: add --unresolved option
PATCH 1/1 show-ref: add --unresolved option
From: John Cai <[email protected]>
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.
---
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(-)
Documentation/git-show-ref.txt +8 -0
@@ -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
builtin/show-ref.c +22 -11
@@ -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); }
t/t1403-show-ref.sh +47 -0
57 lines not on this page · raw message
base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
Signature · 1 line
gitgitgadget
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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.
Quoted 18 lines
Documentation/git-show-ref.txt | 8 ++++++ builtin/show-ref.c | 33 ++++++++++++++++-------- t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 11 deletions(-)Documentation/git-show-ref.txt +1
-0@@ -16,6 +16,7 @@ SYNOPSIS [--] [<ref>...] 'git show-ref' --exclude-existing[=<pattern>] 'git show-ref' --exists <ref> +'git show-ref' --unresolved <ref> DESCRIPTION -----------
Documentation/git-show-ref.txt OPTIONS · line 85
Quoted 9 lines
Documentation/git-show-ref.txt +6
-0@@ -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.
Quoted 22 lines
Documentation/git-show-ref.txt +5
-5-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"));
Documentation/git-show-ref.txt static int cmd_show_ref__exists(const char **refs) · line 260
Quoted 10 lines
Documentation/git-show-ref.txt +7
-0@@ -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/masterI 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/HEADor 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/HEADwouldn'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 Junio,
Quoted 20 lines · Junio C Hamano
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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
--unresolvedoptionthat will ...
Quoted 8 lines
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.
Quoted 82 lines
Quoted 19 lines
Documentation/git-show-ref.txt | 8 ++++++ builtin/show-ref.c | 33 ++++++++++++++++-------- t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 11 deletions(-)Documentation/git-show-ref.txt +1
-0@@ -16,6 +16,7 @@ SYNOPSIS [--] [<ref>...] 'git show-ref' --exclude-existing[=<pattern>] 'git show-ref' --exists <ref> +'git show-ref' --unresolved <ref> DESCRIPTION -----------Documentation/git-show-ref.txt OPTIONS · line 85
Quoted 9 lines
Documentation/git-show-ref.txt +6
-0@@ -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.
Quoted 22 lines
Documentation/git-show-ref.txt +5
-5-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"));Documentation/git-show-ref.txt static int cmd_show_ref__exists(const char **refs) · line 260
Quoted 10 lines
Documentation/git-show-ref.txt +7
-0@@ -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
Quoted 6 lines
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.
Quoted 18 lines
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/HEADwouldn'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
Quoted 8 lines
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_refnot 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/[email protected]/
thanks
John
Quoted 4 lines · John Cai
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.
Hi John
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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
Quoted 202 lines
Signed-off-by John Cai <[email protected]>---
show-ref: add --unresolved optionFor 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/1684Documentation/git-show-ref.txt | 8 ++++++ builtin/show-ref.c | 33 ++++++++++++++++-------- t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 11 deletions(-)Documentation/git-show-ref.txt +8
-0@@ -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 dobuiltin/show-ref.c +22
-11@@ -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); }t/t1403-show-ref.sh +47
-0@@ -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_donebase-commit:
b387623c12f3f4a376e4d35a610fd3e55d7ea907
Quoted 12 lines · Phillip Wood
Hi John
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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.
Quoted 2 lines
"--unresolved" makes me think of merge conflicts. I wonder if "--no-dereference" would be clearer.
Yeah, a --no-style option looks more consistent.
Quoted 12 lines · Phillip Wood
Hi John
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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.
Quoted 2 lines
"--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
Quoted 14 lines · Jeff King
Quoted 12 lines · Phillip Wood
Hi John
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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.
Quoted 5 lines
Quoted 2 lines
"--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.
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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
Quoted 25 lines · Jeff King
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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->threewill 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:
Quoted 7 lines
git symbolic-ref -q $name || git rev-parse --verify $nameI'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
Quoted 7 lines · Patrick Steinhardt
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").
Quoted 4 lines
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
Quoted 3 lines · Jeff King
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.
Quoted 3 lines
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.
Quoted 4 lines
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.
Le 04/03/2024 à 23:51, John Cai via GitGitGadget a écrit :
Documentation/git-show-ref.txt OPTIONS · line 82
Quoted 8 lines
From: John Cai <[email protected]>
Quoted 1 line
(quoted diff) +0
-0@@ -76,6 +77,13 @@ OPTIONSDocumentation/git-show-ref.txt +3
-0it 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...
Quoted 7 lines
Documentation/git-show-ref.txt +4
-0+ 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
v2 cover show-ref: add --symbolic-name option
In reply to John Cai via GitGitGadgetFor 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 --symbolic-name option that will print out the value the ref directly points to without dereferencing it.
Changes since V1:
* changed output format to print out values as a third column
* made plumbing changes to enable the value of a symbolic ref to be read from the iterator
* changed the name of the flag
John Cai (3):
refs: keep track of unresolved reference value in iterator
refs: add referent to each_repo_ref_fn
show-ref: add --symbolic-name option
Documentation/git-show-ref.txt | 21 ++++++++++++++++++- builtin/replace.c | 1 + builtin/show-ref.c | 38 ++++++++++++++++++++++++---------- builtin/submodule--helper.c | 2 +- refs.c | 31 ++++++++++++++++++--------- refs.h | 6 ++++-- refs/files-backend.c | 20 ++++++++++-------- refs/iterator.c | 3 ++- refs/ref-cache.c | 3 +++ refs/ref-cache.h | 2 ++ refs/refs-internal.h | 1 + refs/reftable-backend.c | 13 ++++++++---- remote.c | 2 +- replace-object.c | 1 + sequencer.c | 4 ++-- t/helper/test-ref-store.c | 2 +- t/t1403-show-ref.sh | 20 ++++++++++++++++++ worktree.c | 4 +++- 18 files changed, 130 insertions(+), 44 deletions(-)
base-commit: 7774cfed6261ce2900c84e55906da708c711d601
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1684%2Fjohn-cai%2Fjc%2Fshow-ref-direct-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1684/john-cai/jc/show-ref-direct-v2
Pull-Request: https://github.com/git/git/pull/1684
Range-diff vs v1:
-: ----------- > 1: 6adc9dd26da refs: keep track of unresolved reference value in iterator -: ----------- > 2: b60e78560e0 refs: add referent to each_repo_ref_fn 1: c32572a8d0b ! 3: a9e6644327a show-ref: add --unresolved option
@@ Metadata
Author: John Cai <[email protected]>
## Commit message ##
- show-ref: add --unresolved option
+ show-ref: add --symbolic-name 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.
+ and git-rev-parse, which will resolve the ref. Let's teach show-ref a
+ --symbolic-name option that will cause git-show-ref(1) to print out the
+ value symbolic references points to.
## Documentation/git-show-ref.txt ##
@@ Documentation/git-show-ref.txt: SYNOPSIS
+ [verse]
+ 'git show-ref' [--head] [-d | --dereference]
+ [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
+- [--heads] [--] [<pattern>...]
++ [--heads] [--symbolic-name] [--] [<pattern>...]
+ 'git show-ref' --verify [-q | --quiet] [-d | --dereference]
+ [-s | --hash[=<n>]] [--abbrev[=<n>]]
[--] [<ref>...]
- 'git show-ref' --exclude-existing[=<pattern>]
- 'git show-ref' --exists <ref>
-+'git show-ref' --unresolved <ref>
-
- DESCRIPTION
- -----------
@@ Documentation/git-show-ref.txt: 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.
+ Dereference tags into object IDs as well. They will be shown with ^{}
+ appended.
+
++--symbolic-name::
++
++ Print out the value the reference points to without dereferencing. This
++ is useful to know the reference that a symbolic ref is pointing to.
++
+ -s::
+ --hash[=<n>]::
+
+@@ Documentation/git-show-ref.txt: $ git show-ref --heads --hash
+ ...
+ -----------------------------------------------------------------------------
-+--unresolved::
++When using --symbolic-name, the output is in the format:
++
++-----------
++<oid> SP <ref> SP <symbolic-name>
++-----------
+
-+ 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.
++For example,
+
- --abbrev[=<n>]::
++-----------------------------------------------------------------------------
++$ git show-ref --symbolic-name ++b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/main
++...
++-----------------------------------------------------------------------------
++
+ EXAMPLES
+ --------
- Abbreviate the object name. When using --hash, you do
## builtin/show-ref.c ##
-@@ builtin/show-ref.c: static const char * const show_ref_usage[] = { +@@
+ static const char * const show_ref_usage[] = {
+ N_("git show-ref [--head] [-d | --dereference]\n"
+ " [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n"
+- " [--heads] [--] [<pattern>...]"),
++ " [--heads] [--symbolic-name] [--] [<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>]"),
- N_("git show-ref --exists <ref>"),
-+ N_("git show-ref --unresolved <ref>"),
- NULL
+@@ builtin/show-ref.c: struct show_one_options {
+ int hash_only;
+ int abbrev;
+ int deref_tags;
++ int symbolic_name;
};
-@@ builtin/show-ref.c: static int cmd_show_ref__patterns(const struct patterns_options *opts,
- return 0;
- }
+ static void show_one(const struct show_one_options *opts,
+- const char *refname, const struct object_id *oid)
++ const char *refname,
++ const char *referent,
++ const struct object_id *oid, const int is_symref)
+ {
+ const char *hex;
+ struct object_id peeled;
+@@ builtin/show-ref.c: static void show_one(const struct show_one_options *opts,
+ hex = repo_find_unique_abbrev(the_repository, oid, opts->abbrev);
+ if (opts->hash_only)
+ printf("%s\n", hex);
+- else
++ else if (opts->symbolic_name & is_symref) {
++ printf("%s %s ref:%s\n", hex, refname, referent);
++ } else
+ printf("%s %s\n", hex, refname);
--static int cmd_show_ref__exists(const char **refs)
-+static int cmd_show_ref__raw(const char **refs, int show)
+ if (!opts->deref_tags)
+@@ builtin/show-ref.c: struct show_ref_data {
+ int show_head;
+ };
+
+-static int show_ref(const char *refname, const struct object_id *oid,
+- int flag UNUSED, void *cbdata)
++static int show_ref_referent(struct repository *repo UNUSED,
++ const char *refname,
++ const char *referent,
++ const struct object_id *oid,
++ int flag, void *cbdata)
{
-- 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;
-@@ builtin/show-ref.c: 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"));
-@@ builtin/show-ref.c: static int cmd_show_ref__exists(const char **refs)
- goto out;
- }
+ struct show_ref_data *data = cbdata;
-+ 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;
+@@ builtin/show-ref.c: static int show_ref(const char *refname, const struct object_id *oid,
+ match:
+ data->found_match++;
+
+- show_one(data->show_one_opts, refname, oid);
++ show_one(data->show_one_opts, refname, referent, oid, flag & REF_ISSYMREF);
+
+ return 0;
}
++static int show_ref(const char *refname, const struct object_id *oid,
++ int flag, void *cbdata)
++{
++ return show_ref_referent(NULL, refname, NULL, oid, flag, cbdata);
++}
++
+ static int add_existing(const char *refname,
+ const struct object_id *oid UNUSED,
+ int flag UNUSED, void *cbdata)
+@@ builtin/show-ref.c: static int cmd_show_ref__verify(const struct show_one_options *show_one_opts, +
+ while (*refs) {
+ struct object_id oid;
++ int flags = 0;
+
+ if ((starts_with(*refs, "refs/") || refname_is_safe(*refs)) &&
+- !read_ref(*refs, &oid)) {
+- show_one(show_one_opts, *refs, &oid);
++ !read_ref_full(*refs, 0, &oid, &flags)) {
++ show_one(show_one_opts, *refs, NULL, &oid, flags & REF_ISSYMREF);
+ }
+ else if (!show_one_opts->quiet)
+ die("'%s' - not a valid ref", *refs);
+@@ builtin/show-ref.c: static int cmd_show_ref__patterns(const struct patterns_options *opts,
+ head_ref(show_ref, &show_ref_data);
+ if (opts->heads_only || opts->tags_only) {
+ if (opts->heads_only)
+- for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
++ for_each_ref_all("refs/heads/", show_ref_referent, &show_ref_data);
+ if (opts->tags_only)
+- for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
++ for_each_ref_all("refs/tags/", show_ref_referent, &show_ref_data);
+ } else {
+- for_each_ref(show_ref, &show_ref_data);
++ for_each_ref_all("", show_ref_referent, &show_ref_data);
+ }
+ if (!show_ref_data.found_match)
+ return 1;
@@ builtin/show-ref.c: 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, "symbolic-name", &show_one_opts.symbolic_name, N_("print out symbolic reference values")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
-@@ builtin/show-ref.c: 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);
- }
- ## t/t1403-show-ref.sh ##
-@@ t/t1403-show-ref.sh: 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 &&
+ ## refs.c ##
+@@ refs.c: int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_dat
+ DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
+ }
+
++int for_each_ref_all(const char *prefix, each_repo_ref_fn fn, void *cb_data) ++{
++ return do_for_each_repo_ref(the_repository, prefix, fn, 0,
++ 0, cb_data);
++}
+
-+ test_must_fail git show-ref --verify --unresolved 2>err &&
-+ grep "verify" err &&
-+ grep "unresolved" err &&
- grep "cannot be used together" err
- '
+ int for_each_namespaced_ref(const char **exclude_patterns,
+ each_ref_fn fn, void *cb_data)
+ {
+
+ ## refs.h ##
+@@ refs.h: int refs_for_each_branch_ref(struct ref_store *refs,
+ each_ref_fn fn, void *cb_data);
+ int refs_for_each_remote_ref(struct ref_store *refs,
+ each_ref_fn fn, void *cb_data);
+-
+ /* just iterates the head ref. */
+ int head_ref(each_ref_fn fn, void *cb_data);
+
+@@ refs.h: int for_each_tag_ref(each_ref_fn fn, void *cb_data);
+ int for_each_branch_ref(each_ref_fn fn, void *cb_data);
+ int for_each_remote_ref(each_ref_fn fn, void *cb_data);
+ int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data); ++int for_each_ref_all(const char *prefix, each_repo_ref_fn fn, void *cb_data);
+ /* iterates all refs that match the specified glob pattern. */
+ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data); +
+ ## t/t1403-show-ref.sh ##
@@ t/t1403-show-ref.sh: test_expect_success '--exists with existing special ref' '
git show-ref --exists FETCH_HEAD
'
-+test_expect_success '--unresolved with existing reference' ' ++test_expect_success '--symbolic-name with a non symbolic ref' '
+ 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
++ $commit_oid 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 &&
++ git show-ref --symbolic-name refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >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 &&
++test_expect_success '--symbolic-name with symbolic ref' '
++ test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" &&
++ commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) &&
+ cat >expect <<-EOF &&
-+ ref: $oid
++ $commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ EOF
-+ git show-ref --unresolved refs/heads/missing-oid-2 >actual &&
++ git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
++ git show-ref --symbolic-name SYMBOLIC_REF_A >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
Signature · 1 line
gitgitgadget
PATCH v2 1/3 refs: keep track of unresolved reference value in iterator
From: John Cai <[email protected]>
Since ref iterators do not hold onto the direct value of a reference without resolving it, the only way to get ahold of a direct value of a symbolic ref is to make a separate call to refs_read_symbolic_ref.
To make accessing the direct value of a symbolic ref more efficient, let's save the direct value of the ref in the iterator for both the files backend and reftable backend.
To do so, we also need to add an argument to refs_resolve_ref_unsafe to save the direct value of the reference somewhere.
--- builtin/submodule--helper.c | 2 +- refs.c | 23 +++++++++++++---------- refs.h | 3 ++- refs/files-backend.c | 20 +++++++++++--------- refs/iterator.c | 1 + refs/ref-cache.c | 3 +++ refs/ref-cache.h | 2 ++ refs/refs-internal.h | 1 + refs/reftable-backend.c | 13 +++++++++---- remote.c | 2 +- sequencer.c | 4 ++-- t/helper/test-ref-store.c | 2 +- worktree.c | 4 +++- 13 files changed, 50 insertions(+), 30 deletions(-)
builtin/submodule--helper.c +1 -1
@@ -45,7 +45,7 @@ static int repo_get_default_remote(struct repository *repo, char **default_remotchar *dest =NULL ;struct strbuf sb =STRBUF_INIT ;struct ref_store *store =get_main_ref_store (repo); -const char *refname =refs_resolve_ref_unsafe (store,"HEAD" ,0 ,NULL , +const char *refname =refs_resolve_ref_unsafe (store,"HEAD" ,NULL ,0 ,NULL ,NULL );if (!refname)
refs.c +13 -10
@@ -379,7 +379,7 @@ char *refs_resolve_refdup(struct ref_store *refs, {const char *result; - result =refs_resolve_ref_unsafe (refs, refname, resolve_flags, + result =refs_resolve_ref_unsafe (refs, refname,NULL , resolve_flags, oid, flags);return xstrdup_or_null (result); }
@@ -404,7 +404,7 @@ int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, {struct ref_store *refs =get_main_ref_store (the_repository); -if (refs_resolve_ref_unsafe (refs, refname, resolve_flags, +if (refs_resolve_ref_unsafe (refs, refname,NULL , resolve_flags, oid, flags))return 0 ;return -1 ;
@@ -417,7 +417,7 @@ int read_ref(const char *refname, struct object_id *oid)int refs_ref_exists (struct ref_store *refs,const char *refname) { -return !!refs_resolve_ref_unsafe (refs, refname,RESOLVE_REF_READING , +return !!refs_resolve_ref_unsafe (refs, refname,NULL ,RESOLVE_REF_READING ,NULL ,NULL ); }
@@ -773,7 +773,7 @@ int expand_ref(struct repository *repo, const char *str, int len, this_result = refs_found ? &oid_from_ref : oid;strbuf_reset (&fullref);strbuf_addf (&fullref, *p, len, str); - r =refs_resolve_ref_unsafe (refs, fullref.buf, + r =refs_resolve_ref_unsafe (refs, fullref.buf,NULL ,RESOLVE_REF_READING , this_result, &flag);if (r) {
@@ -807,7 +807,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,strbuf_reset (&path);strbuf_addf (&path, *p, len, str); - ref =refs_resolve_ref_unsafe (refs, path.buf, + ref =refs_resolve_ref_unsafe (refs, path.buf,NULL ,RESOLVE_REF_READING , oid ? &hash :NULL ,NULL );if (!ref)
@@ -876,7 +876,7 @@ int is_pseudoref(struct ref_store *refs, const char *refname)return 0 ;if (ends_with (refname,"_HEAD" )) { -refs_resolve_ref_unsafe (refs, refname, +refs_resolve_ref_unsafe (refs, refname,NULL ,RESOLVE_REF_READING |RESOLVE_REF_NO_RECURSE , &oid,NULL );return !is_null_oid (&oid);
@@ -884,7 +884,7 @@ int is_pseudoref(struct ref_store *refs, const char *refname)for (i =0 ; i <ARRAY_SIZE (irregular_pseudorefs); i++)if (!strcmp (refname, irregular_pseudorefs[i])) { -refs_resolve_ref_unsafe (refs, refname, +refs_resolve_ref_unsafe (refs, refname,NULL ,RESOLVE_REF_READING |RESOLVE_REF_NO_RECURSE , &oid,NULL );return !is_null_oid (&oid);
@@ -1590,7 +1590,7 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)struct object_id oid;int flag; -if (refs_resolve_ref_unsafe (refs,"HEAD" ,RESOLVE_REF_READING , +if (refs_resolve_ref_unsafe (refs,"HEAD" ,NULL ,RESOLVE_REF_READING , &oid, &flag))return fn ("HEAD" , &oid, flag, cb_data);
@@ -1928,6 +1928,7 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,const char *refs_resolve_ref_unsafe (struct ref_store *refs,const char *refname, +char **referent,int resolve_flags,struct object_id *oid,int *flags)
@@ -1989,6 +1990,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, } *flags |= read_flags; +if (referent && read_flags &REF_ISSYMREF && sb_refname.len >0 ) + *referent = sb_refname.buf;if (!(read_flags &REF_ISSYMREF )) {if (*flags &REF_BAD_NAME ) {
@@ -2024,7 +2027,7 @@ int refs_init_db(struct ref_store *refs, int flags, struct strbuf *err)const char *resolve_ref_unsafe (const char *refname,int resolve_flags,struct object_id *oid,int *flags) { -return refs_resolve_ref_unsafe (get_main_ref_store (the_repository), refname, +return refs_resolve_ref_unsafe (get_main_ref_store (the_repository), refname,NULL , resolve_flags, oid, flags); }
@@ -2039,7 +2042,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,if (!refs)return -1 ; -if (!refs_resolve_ref_unsafe (refs, refname,0 , oid, &flags) || +if (!refs_resolve_ref_unsafe (refs, refname,NULL ,0 , oid, &flags) ||is_null_oid (oid))return -1 ;return 0 ;
refs.h +2 -1
@@ -71,9 +71,10 @@ struct pack_refs_opts {struct ref_exclusions *exclusions;struct string_list *includes; }; -const char *refs_resolve_ref_unsafe (struct ref_store *refs, +const char *refname, +char **referent,int resolve_flags,struct object_id *oid,int *flags);
refs/files-backend.c +11 -9
@@ -235,8 +235,9 @@ static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs, {struct object_id oid;int flag; +char *referent; -if (!refs_resolve_ref_unsafe (&refs->base, refname,RESOLVE_REF_READING , +if (!refs_resolve_ref_unsafe (&refs->base, refname, &referent,RESOLVE_REF_READING , &oid, &flag)) {oidclr (&oid); flag |=REF_ISBROKEN ;
@@ -258,7 +259,7 @@ static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,oidclr (&oid); flag |=REF_BAD_NAME |REF_ISBROKEN ; } -add_entry_to_dir (dir,create_ref_entry (refname, &oid, flag)); +add_entry_to_dir (dir,create_ref_entry (refname, referent, &oid, flag)); }/*
@@ -843,6 +844,7 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)continue ; iter->base.refname = iter->iter0->refname; + iter->base.referent = iter->iter0->referent; iter->base.oid = iter->iter0->oid; iter->base.flags = iter->iter0->flags;return ITER_OK ;
@@ -1109,7 +1111,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, goto error_return; } -if (!refs_resolve_ref_unsafe (&refs->base, lock->ref_name,0 , +if (!refs_resolve_ref_unsafe (&refs->base, lock->ref_name,NULL ,0 , &lock->old_oid,NULL ))oidclr (&lock->old_oid); goto out;
@@ -1444,7 +1446,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, goto out; } -if (!refs_resolve_ref_unsafe (&refs->base, oldrefname, +if (!refs_resolve_ref_unsafe (&refs->base, oldrefname,NULL ,RESOLVE_REF_READING |RESOLVE_REF_NO_RECURSE , &orig_oid, &flag)) { ret =error ("refname %s not found" , oldrefname);
@@ -1490,7 +1492,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,* the safety anyway; we want to delete the reference whatever * its current value. */ -if (!copy &&refs_resolve_ref_unsafe (&refs->base, newrefname, +if (!copy &&refs_resolve_ref_unsafe (&refs->base, newrefname,NULL ,RESOLVE_REF_READING |RESOLVE_REF_NO_RECURSE ,NULL ,NULL ) && refs_delete_ref(&refs->base, NULL, newrefname,
@@ -1863,7 +1865,7 @@ static int commit_ref_update(struct files_ref_store *refs,int head_flag;const char *head_ref; - head_ref =refs_resolve_ref_unsafe (&refs->base,"HEAD" , + head_ref =refs_resolve_ref_unsafe (&refs->base,"HEAD" ,NULL ,RESOLVE_REF_READING ,NULL , &head_flag); if (head_ref && (head_flag & REF_ISSYMREF) &&
@@ -1911,7 +1913,7 @@ static void update_symref_reflog(struct files_ref_store *refs,struct object_id new_oid;if (logmsg && -refs_resolve_ref_unsafe (&refs->base, target, +refs_resolve_ref_unsafe (&refs->base, target,NULL ,RESOLVE_REF_READING , &new_oid,NULL ) &&files_log_ref_write (refs, refname, &lock->old_oid, &new_oid, logmsg,0 , &err)) {
@@ -2505,7 +2507,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,* to record and possibly check old_oid: */ if (!refs_resolve_ref_unsafe (&refs->base, - referent.buf,0 , + referent.buf,NULL ,0 , &lock->old_oid,NULL )) {if (update->flags &REF_HAVE_OLD ) { strbuf_addf(err, "cannot lock ref '%s': "
@@ -3200,7 +3202,7 @@ static int files_reflog_expire(struct ref_store *ref_store,int type;const char *ref; - ref =refs_resolve_ref_unsafe (&refs->base, refname, + ref =refs_resolve_ref_unsafe (&refs->base, refname,NULL ,RESOLVE_REF_NO_RECURSE ,NULL , &type); update = !!(ref && !(type &REF_ISSYMREF ));
refs/iterator.c +1 -0
@@ -199,6 +199,7 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator) }if (selection &ITER_YIELD_CURRENT ) { + iter->base.referent = (*iter->current)->referent; iter->base.refname = (*iter->current)->refname; iter->base.oid = (*iter->current)->oid; iter->base.flags = (*iter->current)->flags;
refs/ref-cache.c +3 -0
@@ -34,12 +34,14 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry) }struct ref_entry *create_ref_entry (const char *refname, +const char *referent,const struct object_id *oid,int flag) {struct ref_entry *ref;FLEX_ALLOC_STR (ref, name, refname);oidcpy (&ref->u.value.oid, oid); + ref->u.value.referent = referent; ref->flag = flag;return ref; }
@@ -428,6 +430,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) level->prefix_state = entry_prefix_state; level->index =-1 ; }else { + iter->base.referent = entry->u.value.referent; iter->base.refname = entry->name; iter->base.oid = &entry->u.value.oid; iter->base.flags = entry->flag;
refs/ref-cache.h +2 -0
@@ -42,6 +42,7 @@ struct ref_value {* referred to by the last reference in the symlink chain. */ struct object_id oid; +const char *referent; };/*
@@ -173,6 +174,7 @@ struct ref_entry *create_dir_entry(struct ref_cache *cache, const char *dirname, size_t len);struct ref_entry *create_ref_entry (const char *refname, +const char *referent,const struct object_id *oid,int flag);/*
refs/refs-internal.h +1 -0
@@ -319,6 +319,7 @@ enum do_for_each_ref_flags {struct ref_iterator {struct ref_iterator_vtable *vtable;const char *refname; +const char *referent;const struct object_id *oid;unsigned int flags; };
refs/reftable-backend.c +9 -4
@@ -342,6 +342,7 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)while (!iter->err) {int flags =0 ; +char **symref =NULL ; iter->err =reftable_iterator_next_ref (&iter->iter, &iter->ref);if (iter->err)
@@ -377,7 +378,10 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)oidread (&iter->oid, iter->ref.value.val2.value);break ;case REFTABLE_REF_SYMREF : -if (!refs_resolve_ref_unsafe (&iter->refs->base, iter->ref.refname, +if (!iter->ref.value.symref) + symref = &iter->ref.value.symref; + +if (!refs_resolve_ref_unsafe (&iter->refs->base, iter->ref.refname, symref,RESOLVE_REF_READING , &iter->oid, &flags))oidclr (&iter->oid);break ;
@@ -406,6 +410,7 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)continue ; iter->base.refname = iter->ref.refname; + iter->base.referent = iter->ref.value.symref; iter->base.oid = &iter->oid; iter->base.flags = flags;
@@ -877,7 +882,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,* so it is safe to call `refs_resolve_ref_unsafe()` * here without causing races. */ -const char *resolved =refs_resolve_ref_unsafe (&refs->base, u->refname,0 , +const char *resolved =refs_resolve_ref_unsafe (&refs->base, u->refname,NULL ,0 , ¤t_oid,NULL );if (u->flags &REF_NO_DEREF ) {
@@ -1252,7 +1257,7 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da* never happens. */ if (!create->logmsg || - !refs_resolve_ref_unsafe (&create->refs->base, create->target, + !refs_resolve_ref_unsafe (&create->refs->base, create->target,NULL ,RESOLVE_REF_READING , &new_oid,NULL ) || !should_write_log (&create->refs->base, create->refname))return 0 ;
@@ -1263,7 +1268,7 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da log.value.update.message =xstrndup (create->logmsg, create->refs->write_options.block_size /2 );memcpy (log.value.update.new_hash, new_oid.hash,GIT_MAX_RAWSZ ); -if (refs_resolve_ref_unsafe (&create->refs->base, create->refname, +if (refs_resolve_ref_unsafe (&create->refs->base, create->refname,NULL ,RESOLVE_REF_READING , &old_oid,NULL ))memcpy (log.value.update.old_hash, old_oid.hash,GIT_MAX_RAWSZ );
remote.c +1 -1
@@ -518,7 +518,7 @@ static void read_config(struct repository *repo, int early) repo->remote_state->current_branch =NULL ;if (startup_info->have_repository && !early) {const char *head_ref =refs_resolve_ref_unsafe ( -get_main_ref_store (repo),"HEAD" ,0 ,NULL , &flag); +get_main_ref_store (repo),"HEAD" ,NULL ,0 ,NULL , &flag);if (head_ref && (flag &REF_ISSYMREF ) &&skip_prefix (head_ref,"refs/heads/" , &head_ref)) { repo->remote_state->current_branch = make_branch(
sequencer.c +2 -2
@@ -1430,7 +1430,7 @@ void print_commit_summary(struct repository *r,diff_setup_done (&rev.diffopt); refs =get_main_ref_store (r); - head =refs_resolve_ref_unsafe (refs,"HEAD" ,0 ,NULL ,NULL ); + head =refs_resolve_ref_unsafe (refs,"HEAD" ,NULL ,0 ,NULL ,NULL );if (!head)die (_ ("unable to resolve HEAD after creating commit" ));if (!strcmp (head,"HEAD" ))
@@ -4651,7 +4651,7 @@ static int apply_save_autostash_ref(struct repository *r, const char *refname,if (!refs_ref_exists (get_main_ref_store (r), refname))return 0 ; -if (!refs_resolve_ref_unsafe (get_main_ref_store (r), refname, +if (!refs_resolve_ref_unsafe (get_main_ref_store (r), refname,NULL ,RESOLVE_REF_READING , &stash_oid, &flag))return -1 ;if (flag &REF_ISSYMREF )
t/helper/test-ref-store.c +1 -1
@@ -203,7 +203,7 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)int flags;const char *ref; - ref =refs_resolve_ref_unsafe (refs, refname, resolve_flags, + ref =refs_resolve_ref_unsafe (refs, refname,NULL , resolve_flags, &oid, &flags);printf ("%s %s 0x%x\n" ,oid_to_hex (&oid), ref ? ref :"(null)" , flags);return ref ?0 :1 ;
worktree.c +3 -1
@@ -42,6 +42,7 @@ static void add_head_info(struct worktree *wt) target =refs_resolve_ref_unsafe (get_worktree_ref_store (wt),"HEAD" , +NULL ,0 , &wt->head_oid, &flags);if (!target)
@@ -446,7 +447,7 @@ int is_shared_symref(const struct worktree *wt, const char *symref, } refs =get_worktree_ref_store (wt); - symref_target =refs_resolve_ref_unsafe (refs, symref,0 , + symref_target =refs_resolve_ref_unsafe (refs, symref,NULL ,0 ,NULL , &flags);if ((flags &REF_ISSYMREF ) && symref_target && !strcmp (symref_target, target))
@@ -547,6 +548,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data)strbuf_worktree_ref (wt, &refname,"HEAD" );if (refs_resolve_ref_unsafe (get_main_ref_store (the_repository), refname.buf, +NULL ,RESOLVE_REF_READING , &oid, &flag)) ret =fn (refname.buf, &oid, flag, cb_data);
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator
Quoted 6 lines
refs.h +0
-0@@ -71,9 +71,10 @@ struct pack_refs_opts {struct ref_exclusions *exclusions;
refs.h struct pack_refs_opts { · line 80
Quoted 10 lines
refs.h +2
-1struct string_list *includes; }; -const char *refs_resolve_ref_unsafe (struct ref_store *refs, +const char *refname, +char **referent,int resolve_flags,struct object_id *oid,int *flags);
If referent is meant to be an out-parameter, it should sit next to oid that is also an out-parameter. And as a late-comer sibling, it should sit after its elder brother.
Also, I do not see the reason for the shuffling of blank lines. Shouldn't it be the other way around, even? After an unrelated definition of "struct pack_refs_opts", there is (and should be) a blank line, then the first line of the declaration of function.
Perhaps some fat-fingering.
Quoted 9 lines
refs.h +1
-0@@ -1928,6 +1928,7 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,const char *refs_resolve_ref_unsafe (struct ref_store *refs,const char *refname, +char **referent,int resolve_flags,struct object_id *oid,int *flags)
refs.h const char *refs_resolve_ref_unsafe(struct ref_store *refs, · line 1994
Quoted 5 lines
refs.h +2
-0@@ -1989,6 +1990,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, } *flags |= read_flags; +if (referent && read_flags &REF_ISSYMREF && sb_refname.len >0 ) + *referent = sb_refname.buf;
Is this safe? After this assignment, which "return" in this loop are you expecting to return from this function? If you fail to return from the function during this iteration, you'll clobber the same strbuf with the next refs_read_raw_ref(), but I do not see how you are ensuring that you'll return from the function without such corruption happening.
This assignment happens only when read_flags has REF_ISSYMREF set, so the next "if it is not, then return refname" would not even trigger. If RESOLVE_REF_NO_RECURSE bit is on in resolve_flags, then we'd return without further dereferencing, but if that is the only safe exit from the function, shouldn't you be guarding the function with something like
if (referent && !(resolve_flags & RESOLVE_REF_NO_RECURSE))
BUG("recursive dereference can will clobber *referent");to protect its callers from their own mistakes?
Another return before we start the next iteration of the loop and clobber sb_refname with another call to refs_read_raw_ref() is the error return codepath at the end of the loop, but that is a totally uninteresting case.
Or am I totally confused?
Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator
Hi Junio
Quoted 21 lines · Junio C Hamano
Quoted 6 lines
refs.h +0
-0@@ -71,9 +71,10 @@ struct pack_refs_opts {struct ref_exclusions *exclusions;refs.h struct pack_refs_opts { · line 80
Quoted 10 lines
refs.h +2
-1struct string_list *includes; }; -const char *refs_resolve_ref_unsafe (struct ref_store *refs, +const char *refname, +char **referent,int resolve_flags,struct object_id *oid,int *flags);If referent is meant to be an out-parameter, it should sit next to oid that is also an out-parameter. And as a late-comer sibling, it should sit after its elder brother.
Quoted 6 lines
Also, I do not see the reason for the shuffling of blank lines. Shouldn't it be the other way around, even? After an unrelated definition of "struct
pack_refs_opts", there is (and should be) a blank line, then the first line of the declaration of function.Perhaps some fat-fingering.
This was indeed a case of fat-fingering.
Quoted 33 lines
Quoted 10 lines
refs.h +1
-0@@ -1928,6 +1928,7 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,const char *refs_resolve_ref_unsafe (struct ref_store *refs,const char *refname, +char **referent,int resolve_flags,struct object_id *oid,int *flags)refs.h const char *refs_resolve_ref_unsafe(struct ref_store *refs, · line 1994
Quoted 5 lines
refs.h +2
-0@@ -1989,6 +1990,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, } *flags |= read_flags; +if (referent && read_flags &REF_ISSYMREF && sb_refname.len >0 ) + *referent = sb_refname.buf;Is this safe? After this assignment, which "return" in this loop are you expecting to return from this function? If you fail to return from the function during this iteration, you'll clobber the same strbuf with the next
refs_read_raw_ref(), but I do not see how you are ensuring that you'll return from the function without such corruption happening.This assignment happens only when
read_flagshasREF_ISSYMREFset, so the next "if it is not, then return refname" would not even trigger. IfRESOLVE_REF_NO_RECURSEbit is on inresolve_flags, then we'd return without further dereferencing, but if that is the only safe exit from the function, shouldn't you be guarding the function with something likeif (referent && !(resolve_flags & RESOLVE_REF_NO_RECURSE)) BUG("recursive dereference can will clobber *referent");
Quoted 8 lines
to protect its callers from their own mistakes?
Another return before we start the next iteration of the loop and clobber
sb_refnamewith another call torefs_read_raw_ref()is the error return codepath at the end of the loop, but that is a totally uninteresting case.Or am I totally confused?
Yeah good point. This probably not a good idea. In fact, perhaps adding another argument to this function is unnecessary. We already have refs_read_symbolic_ref and we can just make a separate call to that once we know that a ref is a symbolic reference. Though a separate call is less efficient, I'm not sure it's worth adding this parameter.
thanks
John
Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator
In reply to John Cai via GitGitGadgetQuoted 1 line · John Cai via GitGitGadget
From: John Cai <[email protected]>
[snip]
refs.c int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname, · line 1934
Quoted 8 lines
refs.c +1
-0@@ -1928,6 +1928,7 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,const char *refs_resolve_ref_unsafe (struct ref_store *refs,const char *refname, +char **referent,int resolve_flags,struct object_id *oid,int *flags)
I wonder whether this really should be a char **. You don't seem to be free'ing returned pointers anywhere, so this should probably at least be const char ** to indicate that memory is owned by the ref store. But that would require the ref store to inevitably release it, which may easily lead to bugs when the caller expects a different lifetime.
So how about we instead make this a struct strbuf *referent? This makes it quite clear who owns the memory. Furthermore, if the caller wants to resolve multiple refs, it would allow them to reuse the buffer for better-optimized allocation patterns.
[snip]
Quoted 9 lines
refs/ref-cache.c +1
-0@@ -34,12 +34,14 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry) } struct ref_entry *create_ref_entry(const char *refname, + const char *referent,
refs/ref-cache.c struct ref_dir *get_ref_dir(struct ref_entry *entry) · line 47
Quoted 10 lines
refs/ref-cache.c +1
-0const struct object_id *oid, int flag) {struct ref_entry *ref;FLEX_ALLOC_STR (ref, name, refname);oidcpy (&ref->u.value.oid, oid); + ref->u.value.referent = referent; ref->flag = flag;return ref; }
It's kind of hard to spot the actual changes inbetween all those changes to callsites. Is it possible to split the patch up such that we have one patch that changes all the callsites and one patch that does the actual changes to implement this?
Patrick
Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator
Quoted 13 lines · Patrick Steinhardt
refs.c int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname, · line 1934
Quoted 6 lines
const char *refs_resolve_ref_unsafe(struct
ref_store*refs,refs.c +1
-0const char *refname, + char **referent, int resolve_flags, struct object_id *oid, int *flags)I wonder whether this really should be a
char **. You don't seem to be free'ing returned pointers anywhere, so this should probably at least beconst char **to indicate that memory is owned by the ref store. But that would require the ref store to inevitably release it, which may easily lead to bugs when the caller expects a different lifetime.
Quoted 4 lines
So how about we instead make this a
struct strbuf *referent? This makes it quite clear who owns the memory. Furthermore, if the caller wants to resolve multiple refs, it would allow them to reuse the buffer for better-optimized allocation patterns.
Or return an allocated piece of memory via "char **". I think an interface that _requires_ the caller to use strbuf is not nice, if the caller is not expected to further _edit_ the returned contents using the strbuf API. If it is likely that the caller would want to perform further post-processing on the result, an interface based on strbuf is nice, but I do not think it applies to this case.
Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator
Quoted 20 lines · Junio C Hamano
Quoted 13 lines · Patrick Steinhardt
refs.c int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname, · line 1934
Quoted 6 lines
const char *refs_resolve_ref_unsafe(struct
ref_store*refs,refs.c +1
-0const char *refname, + char **referent, int resolve_flags, struct object_id *oid, int *flags)I wonder whether this really should be a
char **. You don't seem to be free'ing returned pointers anywhere, so this should probably at least beconst char **to indicate that memory is owned by the ref store. But that would require the ref store to inevitably release it, which may easily lead to bugs when the caller expects a different lifetime.Quoted 4 lines
So how about we instead make this a
struct strbuf *referent? This makes it quite clear who owns the memory. Furthermore, if the caller wants to resolve multiple refs, it would allow them to reuse the buffer for better-optimized allocation patterns.
Quoted 6 lines
Or return an allocated piece of memory via "char **". I think an interface that _requires_ the caller to use strbuf is not nice, if the caller is not expected to further _edit_ the returned contents using the strbuf API. If it is likely that the caller would want to perform further post-processing on the result, an interface based on strbuf is nice, but I do not think it applies to this case.
When iterating through refs this would incur one allocation per ref though, whereas using a struct strbuf would only require a single one.
Patrick
PATCH v2 2/3 refs: add referent to each_repo_ref_fn
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
There is no way for callers of the refs api functions that use iterators to get a hold of a direct value of a symbolic ref before its resolved in the callback functions, as either each_ref_fn nor each_repo_ref_fn have an argument for this.
This did not matter since the iterators did not hold onto the direct value of a reference anyway. But, the previous commit started to save the value of the direct reference in the iterator.
Add an argument to each_repo_ref_fn that gets passed the direct value of a reference.
--- builtin/replace.c | 1 + refs.c | 2 ++ refs.h | 1 + refs/iterator.c | 2 +- replace-object.c | 1 + 5 files changed, 6 insertions(+), 1 deletion(-)
builtin/replace.c +1 -0
@@ -48,6 +48,7 @@ struct show_data { };static int show_reference (struct repository *r,const char *refname, + const char *referent UNUSED,const struct object_id *oid, int flag UNUSED, void *cb_data) {
refs.c +2 -0
@@ -1664,6 +1664,7 @@ struct do_for_each_ref_help { static int do_for_each_ref_helper(struct repository *r UNUSED,const char *refname, +const char *referent,const struct object_id *oid,int flags,void *cb_data)
@@ -2565,6 +2566,7 @@ struct do_for_each_reflog_help { static int do_for_each_reflog_helper(struct repository *r UNUSED,const char *refname, +const char *referent, const struct object_id *oid UNUSED,int flags,void *cb_data)
refs.h +1 -0
@@ -311,6 +311,7 @@ typedef int each_ref_fn(const char *refname,*/ typedef int each_repo_ref_fn (struct repository *r,const char *refname, +const char *referent,const struct object_id *oid,int flags,void *cb_data);
refs/iterator.c +1 -1
@@ -449,7 +449,7 @@ int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *ite current_ref_iter = iter;while ((ok =ref_iterator_advance (iter)) ==ITER_OK ) { - retval =fn (r, iter->refname, iter->oid, iter->flags, cb_data); + retval =fn (r, iter->refname, iter->referent, iter->oid, iter->flags, cb_data);if (retval) {/* * If ref_iterator_abort() returns ITER_ERROR,
replace-object.c +1 -0
@@ -10,6 +10,7 @@static int register_replace_ref (struct repository *r,const char *refname, + const char *referent UNUSED,const struct object_id *oid, int flag UNUSED, void *cb_data UNUSED)
Signature · 1 line
gitgitgadget
PATCH v2 3/3 show-ref: add --symbolic-name option
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
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 teach show-ref a --symbolic-name option that will cause git-show-ref(1) to print out the value symbolic references points to.
--- Documentation/git-show-ref.txt | 21 ++++++++++++++++++- builtin/show-ref.c | 38 ++++++++++++++++++++++++---------- refs.c | 6 ++++++ refs.h | 2 +- t/t1403-show-ref.sh | 20 ++++++++++++++++++ 5 files changed, 74 insertions(+), 13 deletions(-)
Documentation/git-show-ref.txt +20 -1
@@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git show-ref' [--head] [-d | --dereference] [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags] - [--heads] [--] [<pattern>...] + [--heads] [--symbolic-name] [--] [<pattern>...] 'git show-ref' --verify [-q | --quiet] [-d | --dereference] [-s | --hash[=<n>]] [--abbrev[=<n>]] [--] [<ref>...]
@@ -58,6 +58,11 @@ OPTIONS Dereference tags into object IDs as well. They will be shown with `^{}` appended. +--symbolic-name:: + + Print out the value the reference points to without dereferencing. This + is useful to know the reference that a symbolic ref is pointing to. + -s:: --hash[=<n>]::
@@ -146,6 +151,20 @@ $ git show-ref --heads --hash ... ----------------------------------------------------------------------------- +When using `--symbolic-name`, the output is in the format: + +----------- +<oid> SP <ref> SP <symbolic-name> +----------- + +For example, + +----------------------------------------------------------------------------- +$ git show-ref --symbolic-name +b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/main +... +----------------------------------------------------------------------------- + EXAMPLES --------
builtin/show-ref.c +27 -11
@@ -12,7 +12,7 @@static const char *const show_ref_usage[] = {N_ ("git show-ref [--head] [-d | --dereference]\n" " [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n" -" [--heads] [--] [<pattern>...]" ), +" [--heads] [--symbolic-name] [--] [<pattern>...]" ),N_ ("git show-ref --verify [-q | --quiet] [-d | --dereference]\n" " [-s | --hash[=<n>]] [--abbrev[=<n>]]\n" " [--] [<ref>...]" ),
@@ -26,10 +26,13 @@ struct show_one_options {int hash_only;int abbrev;int deref_tags; +int symbolic_name; };static void show_one (const struct show_one_options *opts, -const char *refname,const struct object_id *oid) +const char *refname, +const char *referent, +const struct object_id *oid,const int is_symref) {const char *hex;struct object_id peeled;
@@ -44,7 +47,9 @@ static void show_one(const struct show_one_options *opts, hex =repo_find_unique_abbrev (the_repository, oid, opts->abbrev);if (opts->hash_only)printf ("%s\n" , hex); -else +else if (opts->symbolic_name & is_symref) { +printf ("%s %s ref:%s\n" , hex, refname, referent); + }else printf ("%s %s\n" , hex, refname);if (!opts->deref_tags)
@@ -63,8 +68,11 @@ struct show_ref_data {int show_head; }; -static int show_ref (const char *refname,const struct object_id *oid, - int flag UNUSED, void *cbdata) +static int show_ref_referent(struct repository *repo UNUSED, +const char *refname, +const char *referent, +const struct object_id *oid, +int flag,void *cbdata) {struct show_ref_data *data = cbdata;
@@ -91,11 +99,17 @@ static int show_ref(const char *refname, const struct object_id *oid, match: data->found_match++; -show_one (data->show_one_opts, refname, oid); +show_one (data->show_one_opts, refname, referent, oid, flag &REF_ISSYMREF );return 0 ; } +static int show_ref (const char *refname,const struct object_id *oid, +int flag,void *cbdata) +{ +return show_ref_referent (NULL , refname,NULL , oid, flag, cbdata); +} +static int add_existing (const char *refname, const struct object_id *oid UNUSED, int flag UNUSED, void *cbdata)
@@ -171,10 +185,11 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,while (*refs) {struct object_id oid; +int flags =0 ;if ((starts_with (*refs,"refs/" ) ||refname_is_safe (*refs)) && - !read_ref (*refs, &oid)) { -show_one (show_one_opts, *refs, &oid); + !read_ref_full (*refs,0 , &oid, &flags)) { +show_one (show_one_opts, *refs,NULL , &oid, flags &REF_ISSYMREF ); }else if (!show_one_opts->quiet)die ("'%s' - not a valid ref" , *refs);
@@ -208,11 +223,11 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,head_ref (show_ref, &show_ref_data);if (opts->heads_only || opts->tags_only) {if (opts->heads_only) -for_each_fullref_in ("refs/heads/" , show_ref, &show_ref_data); +for_each_ref_all ("refs/heads/" , show_ref_referent, &show_ref_data);if (opts->tags_only) -for_each_fullref_in ("refs/tags/" , show_ref, &show_ref_data); +for_each_ref_all ("refs/tags/" , show_ref_referent, &show_ref_data); }else { -for_each_ref (show_ref, &show_ref_data); +for_each_ref_all ("" , show_ref_referent, &show_ref_data); }if (!show_ref_data.found_match)return 1 ;
@@ -289,6 +304,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)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 ,"symbolic-name" , &show_one_opts.symbolic_name,N_ ("print out symbolic reference values" )),OPT_BOOL (0 ,"verify" , &verify,N_ ("stricter reference checking, " "requires exact ref path" )), OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
refs.c +6 -0
@@ -1734,6 +1734,12 @@ int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_dat DO_FOR_EACH_INCLUDE_BROKEN, cb_data); } +int for_each_ref_all (const char *prefix,each_repo_ref_fn fn,void *cb_data) +{ +return do_for_each_repo_ref (the_repository, prefix, fn,0 , +0 , cb_data); +} +int for_each_namespaced_ref (const char **exclude_patterns,each_ref_fn fn,void *cb_data) {
refs.h +1 -1
@@ -337,7 +337,6 @@ int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);int refs_for_each_remote_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data); -/* just iterates the head ref. */ int head_ref (each_ref_fn fn,void *cb_data);
@@ -381,6 +380,7 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data);int for_each_branch_ref (each_ref_fn fn,void *cb_data);int for_each_remote_ref (each_ref_fn fn,void *cb_data);int for_each_replace_ref (struct repository *r,each_repo_ref_fn fn,void *cb_data); +int for_each_ref_all (const char *prefix,each_repo_ref_fn fn,void *cb_data);/* iterates all refs that match the specified glob pattern. */ int for_each_glob_ref (each_ref_fn fn,const char *pattern,void *cb_data);
t/t1403-show-ref.sh +20 -0
@@ -286,4 +286,24 @@ test_expect_success '--exists with existing special ref' ' git show-ref --exists FETCH_HEAD ' +test_expect_success '--symbolic-name with a non symbolic ref' ' + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git show-ref --symbolic-name refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >actual && + test_cmp expect actual +' + +test_expect_success '--symbolic-name with symbolic ref' ' + test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" && + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && + git show-ref --symbolic-name SYMBOLIC_REF_A >actual && + test_cmp expect actual +' + test_done
Signature · 1 line
gitgitgadget
Re: [PATCH v2 3/3] show-ref: add --symbolic-name option
Hi John
Documentation/git-show-ref.txt OPTIONS · line 64
Quoted 4 lines · John Cai via GitGitGadget
+--symbolic-name::
Documentation/git-show-ref.txt +3
-0+ + Print out the value the reference points to without dereferencing. This + is useful to know the reference that a symbolic ref is pointing to.
It would be helpful to clarify that this prints the contents of the symbolic ref without dereferencing but also prints the OID after dereferencing the given ref.
Documentation/git-show-ref.txt $ git show-ref --heads --hash · line 164
Quoted 11 lines
Documentation/git-show-ref.txt +11
-0+When using `--symbolic-name`, the output is in the format: + +----------- +<oid> SP <ref> SP <symbolic-name> +----------- + +For example, + +----------------------------------------------------------------------------- +$ git show-ref --symbolic-name +b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/main
Do we really need the "ref:" prefix? It is not specified above and I think anyone calling this would have to remove the prefix before they could use the value.
t/t1403-show-ref.sh test_expect_success '--exists with existing special ref' ' · line 307
Quoted 10 lines
t/t1403-show-ref.sh +10
-0+test_expect_success '--symbolic-name with symbolic ref' ' + test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" && + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && + git show-ref --symbolic-name SYMBOLIC_REF_A >actual && + test_cmp expect actual +'
I think it would be nice to see a test along the lines of
git symbolic-ref refs/symref-c refs/heads/master git symbolic-ref refs/symref-b refs/symref-c && git symbolic-ref refs/symref-a refs/symref-b && git show-ref --symbolic-name refs/symref-a >actual && cat >expect <<\EOF && $(git show-ref -s --verify refs/heads/master) refs/heads/symref-a
refs/heads/symref-b
EOF
test_cmp expect actual
to show what this command is expected to return when there is a chain of symbolic references.
Best Wishes
Phillip
Re: [PATCH v2 3/3] show-ref: add --symbolic-name option
Hi Phillip,
Quoted 9 lines · Phillip Wood
Hi John
Documentation/git-show-ref.txt OPTIONS · line 64
Quoted 4 lines · John Cai via GitGitGadget
+--symbolic-name::
Documentation/git-show-ref.txt +3
-0+ + Print out the value the reference points to without dereferencing. This + is useful to know the reference that a symbolic ref is pointing to.It would be helpful to clarify that this prints the contents of the symbolic ref without dereferencing but also prints the OID after dereferencing the given ref.
Thanks, will clarify in the next version.
Quoted 14 lines
Documentation/git-show-ref.txt $ git show-ref --heads --hash · line 164
Quoted 12 lines
Documentation/git-show-ref.txt +11
-0+When using `--symbolic-name`, the output is in the format: + +----------- +<oid> SP <ref> SP <symbolic-name> +----------- + +For example, + +----------------------------------------------------------------------------- +$ git show-ref --symbolic-name +b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/mainDo we really need the "ref:" prefix? It is not specified above and I think anyone calling this would have to remove the prefix before they could use the value.
I can see how it would be more ergonimic to just have the value without the "ref: " prefix. I kept it because that's how the refs are represented on disk and git-symbolic-ref prints them out with the "ref: " prefix.
I don't have a strong preference, but I lean a bit towards keeping it consistent with the output of other commands.
Quoted 24 lines
t/t1403-show-ref.sh test_expect_success '--exists with existing special ref' ' · line 307
Quoted 12 lines
t/t1403-show-ref.sh +10
-0+test_expect_success '--symbolic-name with symbolic ref' ' + test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" && + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && + git show-ref --symbolic-name SYMBOLIC_REF_A >actual && + test_cmp expect actual +'I think it would be nice to see a test along the lines of
git symbolic-ref refs/symref-c refs/heads/master git symbolic-ref refs/symref-b refs/symref-c && git symbolic-ref refs/symref-a refs/symref-b && git show-ref --symbolic-name refs/symref-a >actual && cat >expect <<\EOF && $(git show-ref -s --verify refs/heads/master) refs/heads/symref-a refs/heads/symref-b EOF test_cmp expect actual
Quoted 1 line
to show what this command is expected to return when there is a chain of symbolic references.
good point, will add this in the next series.
Quoted 4 lines
Best Wishes
Phillip
thanks for the review!
John
Re: [PATCH v2 3/3] show-ref: add --symbolic-name option
Hi John
Quoted 15 lines · John Cai
Quoted 5 lines · Phillip Wood
Quoted 5 lines
Documentation/git-show-ref.txt +5
-0+When using `--symbolic-name`, the output is in the format: + +----------- +<oid> SP <ref> SP <symbolic-name> +-----------Quoted 8 lines
Documentation/git-show-ref.txt $ git show-ref --heads --hash · line 164
Quoted 6 lines
Documentation/git-show-ref.txt +6
-0+ +For example, + +----------------------------------------------------------------------------- +$ git show-ref --symbolic-name +b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/mainDo we really need the "ref:" prefix? It is not specified above and I think anyone calling this would have to remove the prefix before they could use the value.
Quoted 6 lines
I can see how it would be more ergonimic to just have the value without the "ref: " prefix. I kept it because that's how the refs are represented on disk and git-symbolic-ref prints them out with the "ref: " prefix.
I don't have a strong preference, but I lean a bit towards keeping it consistent with the output of other commands.
I agree it is a good idea to keep things consistent, and dropping the "ref:" prefix is consistent with other commands:
$ git symbolic-ref HEAD refs/heads/rebase-fix-signoff
Best Wishes
Phillip
Quoted 36 lines
Quoted 25 lines
t/t1403-show-ref.sh test_expect_success '--exists with existing special ref' ' · line 307
Quoted 13 lines
t/t1403-show-ref.sh +10
-0+test_expect_success '--symbolic-name with symbolic ref' ' + test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" && + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && + git show-ref --symbolic-name SYMBOLIC_REF_A >actual && + test_cmp expect actual +'I think it would be nice to see a test along the lines of
git symbolic-ref refs/symref-c refs/heads/master git symbolic-ref refs/symref-b refs/symref-c && git symbolic-ref refs/symref-a refs/symref-b && git show-ref --symbolic-name refs/symref-a >actual && cat >expect <<\EOF && $(git show-ref -s --verify refs/heads/master) refs/heads/symref-a refs/heads/symref-b EOF test_cmp expect actualQuoted 1 line
to show what this command is expected to return when there is a chain of symbolic references.
good point, will add this in the next series.
Quoted 4 lines
Best Wishes
Phillip
thanks for the review!
John
Re: [PATCH v2 3/3] show-ref: add --symbolic-name option
In reply to John Cai via GitGitGadgetQuoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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 teach show-ref a --symbolic-name option that will cause git-show-ref(1) to print out the value symbolic references points to.
I think it was Peff who shared a way to achieve this without actually introducing a new option via git for-each-ref --format=. Can we maybe provide some benchmarks to demonstrate that this variant is preferable over what's already possible?
Patrick
Quoted 232 lines
Signed-off-by John Cai <[email protected]>--- Documentation/git-show-ref.txt | 21 ++++++++++++++++++- builtin/show-ref.c | 38 ++++++++++++++++++++++++---------- refs.c | 6 ++++++ refs.h | 2 +- t/t1403-show-ref.sh | 20 ++++++++++++++++++ 5 files changed, 74 insertions(+), 13 deletions(-)Documentation/git-show-ref.txt +20
-1@@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git show-ref' [--head] [-d | --dereference] [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags] - [--heads] [--] [<pattern>...] + [--heads] [--symbolic-name] [--] [<pattern>...] 'git show-ref' --verify [-q | --quiet] [-d | --dereference] [-s | --hash[=<n>]] [--abbrev[=<n>]] [--] [<ref>...]@@ -58,6 +58,11 @@ OPTIONS Dereference tags into object IDs as well. They will be shown with `^{}` appended. +--symbolic-name:: + + Print out the value the reference points to without dereferencing. This + is useful to know the reference that a symbolic ref is pointing to. + -s:: --hash[=<n>]::@@ -146,6 +151,20 @@ $ git show-ref --heads --hash ----------------------------------------------------------------------------- +When using `--symbolic-name`, the output is in the format: + +----------- +<oid> SP <ref> SP <symbolic-name> +----------- + +For example, + +----------------------------------------------------------------------------- +$ git show-ref --symbolic-name +b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/main +... +----------------------------------------------------------------------------- + EXAMPLES --------builtin/show-ref.c +27
-11@@ -12,7 +12,7 @@static const char *const show_ref_usage[] = {N_ ("git show-ref [--head] [-d | --dereference]\n" " [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n" -" [--heads] [--] [<pattern>...]" ), +" [--heads] [--symbolic-name] [--] [<pattern>...]" ),N_ ("git show-ref --verify [-q | --quiet] [-d | --dereference]\n" " [-s | --hash[=<n>]] [--abbrev[=<n>]]\n" " [--] [<ref>...]" ),@@ -26,10 +26,13 @@ struct show_one_options {int hash_only;int abbrev;int deref_tags; +int symbolic_name; };static void show_one (const struct show_one_options *opts, -const char *refname,const struct object_id *oid) +const char *refname, +const char *referent, +const struct object_id *oid,const int is_symref) {const char *hex;struct object_id peeled;@@ -44,7 +47,9 @@ static void show_one(const struct show_one_options *opts, hex =repo_find_unique_abbrev (the_repository, oid, opts->abbrev);if (opts->hash_only)printf ("%s\n" , hex); -else +else if (opts->symbolic_name & is_symref) { +printf ("%s %s ref:%s\n" , hex, refname, referent); + }else printf ("%s %s\n" , hex, refname);if (!opts->deref_tags)@@ -63,8 +68,11 @@ struct show_ref_data {int show_head; }; -static int show_ref (const char *refname,const struct object_id *oid, - int flag UNUSED, void *cbdata) +static int show_ref_referent(struct repository *repo UNUSED, +const char *refname, +const char *referent, +const struct object_id *oid, +int flag,void *cbdata) {struct show_ref_data *data = cbdata;@@ -91,11 +99,17 @@ static int show_ref(const char *refname, const struct object_id *oid, match: data->found_match++; -show_one (data->show_one_opts, refname, oid); +show_one (data->show_one_opts, refname, referent, oid, flag &REF_ISSYMREF );return 0 ; } +static int show_ref (const char *refname,const struct object_id *oid, +int flag,void *cbdata) +{ +return show_ref_referent (NULL , refname,NULL , oid, flag, cbdata); +} +static int add_existing (const char *refname, const struct object_id *oid UNUSED, int flag UNUSED, void *cbdata)@@ -171,10 +185,11 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,while (*refs) {struct object_id oid; +int flags =0 ;if ((starts_with (*refs,"refs/" ) ||refname_is_safe (*refs)) && - !read_ref (*refs, &oid)) { -show_one (show_one_opts, *refs, &oid); + !read_ref_full (*refs,0 , &oid, &flags)) { +show_one (show_one_opts, *refs,NULL , &oid, flags &REF_ISSYMREF ); }else if (!show_one_opts->quiet)die ("'%s' - not a valid ref" , *refs);@@ -208,11 +223,11 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,head_ref (show_ref, &show_ref_data);if (opts->heads_only || opts->tags_only) {if (opts->heads_only) -for_each_fullref_in ("refs/heads/" , show_ref, &show_ref_data); +for_each_ref_all ("refs/heads/" , show_ref_referent, &show_ref_data);if (opts->tags_only) -for_each_fullref_in ("refs/tags/" , show_ref, &show_ref_data); +for_each_ref_all ("refs/tags/" , show_ref_referent, &show_ref_data); }else { -for_each_ref (show_ref, &show_ref_data); +for_each_ref_all ("" , show_ref_referent, &show_ref_data); }if (!show_ref_data.found_match)return 1 ;@@ -289,6 +304,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)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 ,"symbolic-name" , &show_one_opts.symbolic_name,N_ ("print out symbolic reference values" )),OPT_BOOL (0 ,"verify" , &verify,N_ ("stricter reference checking, " "requires exact ref path" )), OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,refs.c +6
-0@@ -1734,6 +1734,12 @@ int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_dat DO_FOR_EACH_INCLUDE_BROKEN, cb_data); } +int for_each_ref_all (const char *prefix,each_repo_ref_fn fn,void *cb_data) +{ +return do_for_each_repo_ref (the_repository, prefix, fn,0 , +0 , cb_data); +} +int for_each_namespaced_ref (const char **exclude_patterns,each_ref_fn fn,void *cb_data) {refs.h +1
-1@@ -337,7 +337,6 @@ int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);int refs_for_each_remote_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data); -/* just iterates the head ref. */ int head_ref (each_ref_fn fn,void *cb_data);@@ -381,6 +380,7 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data);int for_each_branch_ref (each_ref_fn fn,void *cb_data);int for_each_remote_ref (each_ref_fn fn,void *cb_data);int for_each_replace_ref (struct repository *r,each_repo_ref_fn fn,void *cb_data); +int for_each_ref_all (const char *prefix,each_repo_ref_fn fn,void *cb_data);/* iterates all refs that match the specified glob pattern. */ int for_each_glob_ref (each_ref_fn fn,const char *pattern,void *cb_data);t/t1403-show-ref.sh +20
-0@@ -286,4 +286,24 @@ test_expect_success '--exists with existing special ref' ' git show-ref --exists FETCH_HEAD ' +test_expect_success '--symbolic-name with a non symbolic ref' ' + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git show-ref --symbolic-name refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >actual && + test_cmp expect actual +' + +test_expect_success '--symbolic-name with symbolic ref' ' + test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" && + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && + git show-ref --symbolic-name SYMBOLIC_REF_A >actual && + test_cmp expect actual +' + test_doneSignature · 1 line
gitgitgadget
Re: [PATCH v2 3/3] show-ref: add --symbolic-name option
Quoted 10 lines · Patrick Steinhardt
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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 teach show-ref a --symbolic-name option that will cause git-show-ref(1) to print out the value symbolic references points to.
Quoted 4 lines
I think it was Peff who shared a way to achieve this without actually introducing a new option via
git for-each-ref --format=. Can we maybe provide some benchmarks to demonstrate that this variant is preferable over what's already possible?
Yes, I recall that discussion, and in fact I was a bit surprised to see that this iteration still went to the show-ref route.
Thanks for bringing it up.
Re: [PATCH v2 3/3] show-ref: add --symbolic-name option
In reply to Patrick SteinhardtHi Patrick,
Quoted 10 lines · Patrick Steinhardt
Quoted 8 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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 teach show-ref a --symbolic-name option that will cause git-show-ref(1) to print out the value symbolic references points to.
Quoted 4 lines
I think it was Peff who shared a way to achieve this without actually introducing a new option via
git for-each-ref --format=. Can we maybe provide some benchmarks to demonstrate that this variant is preferable over what's already possible?
Yes, it would be better to not introduce a new option. I've done a quick benchmark including my changes to add the unresolved symref to the iterator, and some changes to integrate this into ref-filter.c. Here are the results:
$ hyperfine --warmup 5 "git for-each-ref \ --format='%(refname) %(objectname) %(symref)'" "./git for-each-ref \ --format='%(refname) %(objectname) %(symref)'"
Benchmark 1: git for-each-ref --format='%(refname) %(objectname) %(symref)'
Time (mean ± σ): 213.5 ms ± 9.9 ms [User: 7.5 ms, System: 14.3 ms]
Range (min … max): 202.7 ms … 236.3 ms 14 runs
Benchmark 2: ./git for-each-ref --format='%(refname) %(objectname) %(symref)'
Time (mean ± σ): 10.8 ms ± 1.3 ms [User: 4.4 ms, System: 6.2 ms]
Range (min … max): 9.5 ms … 17.5 ms 189 runs
Summary
./git for-each-ref --format='%(refname) %(objectname) %(symref)' ran
19.72 ± 2.62 times faster than git for-each-ref --format='%(refname) %(objectname) %(symref)'
It looks like this gives us a nice speedup. I will send up a new version that improves git-for-each-ref instead.
thanks
John
Quoted 235 lines
Patrick
Quoted 232 lines
Signed-off-by John Cai <[email protected]>--- Documentation/git-show-ref.txt | 21 ++++++++++++++++++- builtin/show-ref.c | 38 ++++++++++++++++++++++++---------- refs.c | 6 ++++++ refs.h | 2 +- t/t1403-show-ref.sh | 20 ++++++++++++++++++ 5 files changed, 74 insertions(+), 13 deletions(-)Documentation/git-show-ref.txt +20
-1@@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git show-ref' [--head] [-d | --dereference] [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags] - [--heads] [--] [<pattern>...] + [--heads] [--symbolic-name] [--] [<pattern>...] 'git show-ref' --verify [-q | --quiet] [-d | --dereference] [-s | --hash[=<n>]] [--abbrev[=<n>]] [--] [<ref>...]@@ -58,6 +58,11 @@ OPTIONS Dereference tags into object IDs as well. They will be shown with `^{}` appended. +--symbolic-name:: + + Print out the value the reference points to without dereferencing. This + is useful to know the reference that a symbolic ref is pointing to. + -s:: --hash[=<n>]::@@ -146,6 +151,20 @@ $ git show-ref --heads --hash ----------------------------------------------------------------------------- +When using `--symbolic-name`, the output is in the format: + +----------- +<oid> SP <ref> SP <symbolic-name> +----------- + +For example, + +----------------------------------------------------------------------------- +$ git show-ref --symbolic-name +b75428bae1d090f60bdd4b67185f814bc8f0819d refs/heads/SYMBOLIC_REF ref:refs/heads/main +... +----------------------------------------------------------------------------- + EXAMPLES --------builtin/show-ref.c +27
-11@@ -12,7 +12,7 @@static const char *const show_ref_usage[] = {N_ ("git show-ref [--head] [-d | --dereference]\n" " [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n" -" [--heads] [--] [<pattern>...]" ), +" [--heads] [--symbolic-name] [--] [<pattern>...]" ),N_ ("git show-ref --verify [-q | --quiet] [-d | --dereference]\n" " [-s | --hash[=<n>]] [--abbrev[=<n>]]\n" " [--] [<ref>...]" ),@@ -26,10 +26,13 @@ struct show_one_options {int hash_only;int abbrev;int deref_tags; +int symbolic_name; };static void show_one (const struct show_one_options *opts, -const char *refname,const struct object_id *oid) +const char *refname, +const char *referent, +const struct object_id *oid,const int is_symref) {const char *hex;struct object_id peeled;@@ -44,7 +47,9 @@ static void show_one(const struct show_one_options *opts, hex =repo_find_unique_abbrev (the_repository, oid, opts->abbrev);if (opts->hash_only)printf ("%s\n" , hex); -else +else if (opts->symbolic_name & is_symref) { +printf ("%s %s ref:%s\n" , hex, refname, referent); + }else printf ("%s %s\n" , hex, refname);if (!opts->deref_tags)@@ -63,8 +68,11 @@ struct show_ref_data {int show_head; }; -static int show_ref (const char *refname,const struct object_id *oid, - int flag UNUSED, void *cbdata) +static int show_ref_referent(struct repository *repo UNUSED, +const char *refname, +const char *referent, +const struct object_id *oid, +int flag,void *cbdata) {struct show_ref_data *data = cbdata;@@ -91,11 +99,17 @@ static int show_ref(const char *refname, const struct object_id *oid, match: data->found_match++; -show_one (data->show_one_opts, refname, oid); +show_one (data->show_one_opts, refname, referent, oid, flag &REF_ISSYMREF );return 0 ; } +static int show_ref (const char *refname,const struct object_id *oid, +int flag,void *cbdata) +{ +return show_ref_referent (NULL , refname,NULL , oid, flag, cbdata); +} +static int add_existing (const char *refname, const struct object_id *oid UNUSED, int flag UNUSED, void *cbdata)@@ -171,10 +185,11 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,while (*refs) {struct object_id oid; +int flags =0 ;if ((starts_with (*refs,"refs/" ) ||refname_is_safe (*refs)) && - !read_ref (*refs, &oid)) { -show_one (show_one_opts, *refs, &oid); + !read_ref_full (*refs,0 , &oid, &flags)) { +show_one (show_one_opts, *refs,NULL , &oid, flags &REF_ISSYMREF ); }else if (!show_one_opts->quiet)die ("'%s' - not a valid ref" , *refs);@@ -208,11 +223,11 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,head_ref (show_ref, &show_ref_data);if (opts->heads_only || opts->tags_only) {if (opts->heads_only) -for_each_fullref_in ("refs/heads/" , show_ref, &show_ref_data); +for_each_ref_all ("refs/heads/" , show_ref_referent, &show_ref_data);if (opts->tags_only) -for_each_fullref_in ("refs/tags/" , show_ref, &show_ref_data); +for_each_ref_all ("refs/tags/" , show_ref_referent, &show_ref_data); }else { -for_each_ref (show_ref, &show_ref_data); +for_each_ref_all ("" , show_ref_referent, &show_ref_data); }if (!show_ref_data.found_match)return 1 ;@@ -289,6 +304,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)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 ,"symbolic-name" , &show_one_opts.symbolic_name,N_ ("print out symbolic reference values" )),OPT_BOOL (0 ,"verify" , &verify,N_ ("stricter reference checking, " "requires exact ref path" )), OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,refs.c +6
-0@@ -1734,6 +1734,12 @@ int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_dat DO_FOR_EACH_INCLUDE_BROKEN, cb_data); } +int for_each_ref_all (const char *prefix,each_repo_ref_fn fn,void *cb_data) +{ +return do_for_each_repo_ref (the_repository, prefix, fn,0 , +0 , cb_data); +} +int for_each_namespaced_ref (const char **exclude_patterns,each_ref_fn fn,void *cb_data) {refs.h +1
-1@@ -337,7 +337,6 @@ int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);int refs_for_each_remote_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data); -/* just iterates the head ref. */ int head_ref (each_ref_fn fn,void *cb_data);@@ -381,6 +380,7 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data);int for_each_branch_ref (each_ref_fn fn,void *cb_data);int for_each_remote_ref (each_ref_fn fn,void *cb_data);int for_each_replace_ref (struct repository *r,each_repo_ref_fn fn,void *cb_data); +int for_each_ref_all (const char *prefix,each_repo_ref_fn fn,void *cb_data);/* iterates all refs that match the specified glob pattern. */ int for_each_glob_ref (each_ref_fn fn,const char *pattern,void *cb_data);t/t1403-show-ref.sh +20
-0@@ -286,4 +286,24 @@ test_expect_success '--exists with existing special ref' ' git show-ref --exists FETCH_HEAD ' +test_expect_success '--symbolic-name with a non symbolic ref' ' + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git show-ref --symbolic-name refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >actual && + test_cmp expect actual +' + +test_expect_success '--symbolic-name with symbolic ref' ' + test_when_finished "git symbolic-ref -d refs/heads/SYMBOLIC_REF_A" && + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) && + cat >expect <<-EOF && + $commit_oid refs/heads/SYMBOLIC_REF_A ref:refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + EOF + git symbolic-ref refs/heads/SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME && + git show-ref --symbolic-name SYMBOLIC_REF_A >actual && + test_cmp expect actual +' + test_doneSignature · 1 line
gitgitgadget