Extend "checkout --track" DWIM to support more cases
PATCH 1/1 Extend "checkout --track" DWIM to support more cases
The code handles additionally "refs/remotes/<something>/name", "remotes/<something>/name", and "refs/<namespace>/name". Test cases included.
---
Johannes has likable ideas :)
builtin-checkout.c | 20 +++++++++++++++----- t/t7201-co.sh | 23 ++++++++++++++++++++++- 2 files changed, 37 insertions(+), 6 deletions(-)
builtin-checkout.c +15 -5
@@ -447,11 +447,21 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)char *slash;if (!argc || !strcmp (argv[0 ],"--" ))die ("--track needs a branch name" ); - slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" )) - slash =strchr (slash +1 ,'/' ); -if (slash && !prefixcmp (argv[0 ],"remotes/" )) - slash =strchr (slash +1 ,'/' ); +if (!prefixcmp (argv[0 ],"remotes/" )) +/* skip the name of a remote */ + slash =strchr (argv[0 ] +8 ,'/' ); +else if (!prefixcmp (argv[0 ],"refs/" )) { +/* skip namespaces, try use the names of their +* branches, but for the target namespace +* (heads) demand a new name. Also skip the +* first element in "remotes" namespace */ +const char *ns = argv[0 ] +5 ; + slash = !prefixcmp (ns,"heads/" ) ?NULL : + !prefixcmp (ns,"remotes/" ) ? +strchr (ns +8 ,'/' ):strchr (ns,'/' ); + }else +/* otherwise - just skip the first element */ + slash =strchr (argv[0 ],'/' );if (!slash || !slash[1 ])die ("Missing branch name; try -b" ); opts.new_branch = slash +1 ;
t/t7201-co.sh +22 -1
@@ -340,9 +340,30 @@ test_expect_success \ test_expect_success \ 'checkout with --track fakes a sensible -b <name>' ' git update-ref refs/remotes/origin/koala/bear renamer && + git update-ref refs/new/koala/bear renamer && + git checkout --track origin/koala/bear && test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && - test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)"' + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" && + + git checkout master && git branch -D koala/bear && + + git checkout --track refs/remotes/origin/koala/bear && + test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" && + + git checkout master && git branch -D koala/bear && + + git checkout --track remotes/origin/koala/bear && + test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" && + + git checkout master && git branch -D koala/bear && + + git checkout --track refs/new/koala/bear && + test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" +' test_expect_success \ 'checkout with --track, but without -b, fails with too short tracked name' '
Signature · 1 line
1.6.0.22.g09248
Hi,
Quoted 18 lines · Alex Riesen
The code handles additionally "refs/remotes/<something>/name", "remotes/<something>/name", and "refs/<namespace>/name". Test cases included.
Signed-off-by Alex Riesen <[email protected]>---
Johannes has likable ideas :)
builtin-checkout.c | 20 +++++++++++++++----- t/t7201-co.sh | 23 ++++++++++++++++++++++- 2 files changed, 37 insertions(+), 6 deletions(-)builtin-checkout.c +0
-0
builtin-checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 454
Quoted 8 lines
builtin-checkout.c +0
-5@@ -447,11 +447,21 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)char *slash;if (!argc || !strcmp (argv[0 ],"--" ))die ("--track needs a branch name" ); - slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" )) - slash =strchr (slash +1 ,'/' ); -if (slash && !prefixcmp (argv[0 ],"remotes/" )) - slash =strchr (slash +1 ,'/' );
Why is this not enough? It strips refs/ if there is one, and remotes/ if there is one (possibly after stripping refs/). No?
Puzzled,
Dscho
Johannes Schindelin, Wed, Aug 20, 2008 21:52:23 +0200:
Quoted 10 lines · Alex Riesen
builtin-checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 454
Quoted 5 lines
builtin-checkout.c +0
-5- slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" )) - slash =strchr (slash +1 ,'/' ); -if (slash && !prefixcmp (argv[0 ],"remotes/" )) - slash =strchr (slash +1 ,'/' );Why is this not enough? It strips refs/ if there is one, and remotes/ if there is one (possibly after stripping refs/). No?
No. It strips refs/ OR remotes/ (because of prefixcmp with argv[0]). And I still wanted refs/<namespace>/something...
Hi,
Quoted 12 lines · Alex Riesen
Johannes Schindelin, Wed, Aug 20, 2008 21:52:23 +0200:
Quoted 10 lines · Alex Riesen
Review
Quoted 5 lines
(quoted diff) +0
-5- slash = strchr(argv[0], '/'); - if (slash && !prefixcmp(argv[0], "refs/")) - slash = strchr(slash + 1, '/'); - if (slash && !prefixcmp(argv[0], "remotes/")) - slash = strchr(slash + 1, '/');Why is this not enough? It strips refs/ if there is one, and remotes/ if there is one (possibly after stripping refs/). No?
Quoted 2 lines
No. It strips refs/ OR remotes/ (because of prefixcmp with argv[0]). And I still wanted refs/<namespace>/something...
Yes, you are correct. However, to fix my thinko, I deem this preferable:
-- snipsnap --
builtin-checkout.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
builtin-checkout.c +4 -2
@@ -448,8 +448,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)if (!argc || !strcmp (argv[0 ],"--" ))die ("--track needs a branch name" ); slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" )) - slash =strchr (slash +1 ,'/' ); +if (slash && !prefixcmp (argv[0 ],"refs/" )) { + argv[0 ] = slash +1 ; + slash =strchr (argv[0 ],'/' ); + }if (slash && !prefixcmp (argv[0 ],"remotes/" )) slash =strchr (slash +1 ,'/' );if (!slash || !slash[1 ])
Johannes Schindelin, Wed, Aug 20, 2008 22:16:19 +0200:
Quoted 16 lines
Quoted 3 lines
No. It strips refs/ OR remotes/ (because of prefixcmp with argv[0]). And I still wanted refs/<namespace>/something...
Yes, you are correct. However, to fix my thinko, I deem this preferable:
-- snipsnap --
builtin-checkout.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)builtin-checkout.c +0
-0
builtin-checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 454
Quoted 9 lines
builtin-checkout.c +4
-2@@ -448,8 +448,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)if (!argc || !strcmp (argv[0 ],"--" ))die ("--track needs a branch name" ); slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" )) - slash =strchr (slash +1 ,'/' ); +if (slash && !prefixcmp (argv[0 ],"refs/" )) { + argv[0 ] = slash +1 ; + slash =strchr (argv[0 ],'/' ); + }
Yes, I agree (and its shorter). The git-checkout manpage can be improved, too (no DWIM is obvious, except may be for the implementor).
Quoted 21 lines · Alex Riesen
Johannes Schindelin, Wed, Aug 20, 2008 22:16:19 +0200:
Quoted 20 lines
Quoted 3 lines
No. It strips refs/ OR remotes/ (because of prefixcmp with argv[0]). And I still wanted refs/<namespace>/something...
Yes, you are correct. However, to fix my thinko, I deem this preferable:
-- snipsnap --
builtin-checkout.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)builtin-checkout.c +0
-1@@ -448,8 +448,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)if (!argc || !strcmp (argv[0 ],"--" ))die ("--track needs a branch name" ); slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" ))
Quoted 8 lines
builtin-checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 454
Quoted 5 lines
builtin-checkout.c +4
-1- slash =strchr (slash +1 ,'/' ); +if (slash && !prefixcmp (argv[0 ],"refs/" )) { + argv[0 ] = slash +1 ; + slash =strchr (argv[0 ],'/' ); + }Yes, I agree (and its shorter). The git-checkout manpage can be improved, too (no DWIM is obvious, except may be for the implementor).
I think that makes sense. Care to send an appliable patch with documentation updates, please?
PATCH 1/1 Extend "checkout --track" DWIM to support more cases
The code handles additionally "refs/remotes/<something>/name", "remotes/<something>/name", and "refs/<namespace>/name".
---
Junio C Hamano, Thu, Aug 21, 2008 00:22:21 +0200:
Quoted 5 lines
Quoted 4 lines · Alex Riesen
Johannes Schindelin, Wed, Aug 20, 2008 22:16:19 +0200:
Quoted 3 lines
Quoted 2 lines
No. It strips refs/ OR remotes/ (because of prefixcmp with argv[0]). And I still wanted refs/<namespace>/something...
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Yes, you are correct. However, to fix my thinko, I deem this preferable:
Yes, I agree (and its shorter). The git-checkout manpage can be improved, too (no DWIM is obvious, except may be for the implementor).
I think that makes sense. Care to send an appliable patch with documentation updates, please?
I just have another quiet evening, so I did that. Johannes, I changed your fix a bit: I see that argv[0] is used later (or if I'm blind and it actually isn't, it may be used in future: I have a feeling that builtint-checkout.c will be popular place).
Documentation/git-checkout.txt | 18 +++++++++++++++--- builtin-checkout.c | 13 ++++++++----- t/t7201-co.sh | 23 ++++++++++++++++++++++- 3 files changed, 45 insertions(+), 9 deletions(-)
Documentation/git-checkout.txt +15 -3
@@ -64,9 +64,21 @@ OPTIONS given. Set it to `always` if you want this behavior when the start-point is either a local or remote branch. + -If no '-b' option was given, a name will be made up for you, by stripping -the part up to the first slash of the tracked branch. For example, if you -called 'git checkout --track origin/next', the branch name will be 'next'. +If no '-b' option was given, the name of the new branch will be +derived from the remote branch, by attempting to guess the name of the +branch on remote system and use it. The algorithm will remove a +prefixed refs/ and the part after refs/ up to a slash (that part +usually being a branch namespace, which makes it confusing to use for +branch names). If the part after refs/ was remotes/, than a part past +it is stripped too (it is expected to be the name of remote system). +Otherwise, the part up to the first slash is removed. +The algorithm aborts either if the given name has no slashes in it or +if the part left after stripping the prefixes is empty. In this case, +the name should be provided with '-b'. ++ +For example, if you called 'git checkout --track origin/next', the +branch name will be 'next', as it will for 'git checkout --track +remotes/origin/next'. --no-track:: Ignore the branch.autosetupmerge configuration variable.
builtin-checkout.c +8 -5
@@ -445,12 +445,15 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)/* --track without -b should DWIM */ if (opts.track && opts.track !=-1 && !opts.new_branch) {char *slash; -if (!argc || !strcmp (argv[0 ],"--" )) +char *argv0 = argv[0 ]; +if (!argc || !strcmp (argv0,"--" ))die ("--track needs a branch name" ); - slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" )) - slash =strchr (slash +1 ,'/' ); -if (slash && !prefixcmp (argv[0 ],"remotes/" )) + slash =strchr (argv0,'/' ); +if (slash && !prefixcmp (argv0,"refs/" )) { + argv0 = slash +1 ; + slash =strchr (argv0,'/' ); + } +if (slash && !prefixcmp (argv0,"remotes/" )) slash =strchr (slash +1 ,'/' );if (!slash || !slash[1 ])die ("Missing branch name; try -b" );
t/t7201-co.sh +22 -1
@@ -340,9 +340,30 @@ test_expect_success \ test_expect_success \ 'checkout with --track fakes a sensible -b <name>' ' git update-ref refs/remotes/origin/koala/bear renamer && + git update-ref refs/new/koala/bear renamer && + git checkout --track origin/koala/bear && test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && - test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)"' + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" && + + git checkout master && git branch -D koala/bear && + + git checkout --track refs/remotes/origin/koala/bear && + test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" && + + git checkout master && git branch -D koala/bear && + + git checkout --track remotes/origin/koala/bear && + test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" && + + git checkout master && git branch -D koala/bear && + + git checkout --track refs/new/koala/bear && + test "refs/heads/koala/bear" = "$(git symbolic-ref HEAD)" && + test "$(git rev-parse HEAD)" = "$(git rev-parse renamer)" +' test_expect_success \ 'checkout with --track, but without -b, fails with too short tracked name' '
Signature · 1 line
1.6.0.59.g8ae62
Documentation/git-checkout.txt OPTIONS · line 72
Quoted 6 lines · Alex Riesen
Documentation/git-checkout.txt +6
-0+If no '-b' option was given, the name of the new branch will be +derived from the remote branch, by attempting to guess the name of the +branch on remote system and use it. The algorithm will remove a +prefixed refs/ and the part after refs/ up to a slash (that part +usually being a branch namespace, which makes it confusing to use for +branch names). If the part after refs/ was remotes/, than a part past
s/than/then/
j.
Quoted 10 lines · Alex Riesen
The code handles additionally "refs/remotes/<something>/name", "remotes/<something>/name", and "refs/<namespace>/name".
Signed-off-by Alex Riesen <[email protected]>---
...
I just have another quiet evening, so I did that. Johannes, I changed your fix a bit: I see that argv[0] is used later (or if I'm blind and it actually isn't, it may be used in future: I have a feeling thatbuiltint-checkout.cwill be popular place).
Thanks; the documentation looks good to me.
Review
Quoted 12 lines
(quoted diff) +8
-0+ char *argv0 = argv[0]; + if (!argc || !strcmp(argv0, "--")) die ("--track needs a branch name"); + slash = strchr(argv0, '/'); + if (slash && !prefixcmp(argv0, "refs/")) { + argv0 = slash + 1; + slash = strchr(argv0, '/'); + } + if (slash && !prefixcmp(argv0, "remotes/")) slash = strchr(slash + 1, '/'); if (!slash || !slash[1]) die ("Missing branch name; try -b");
I however wonder if this is clearer.
* "enum branch_track" was unsigned; comparing equality with -1 was Ok but we couldn't say 0 < opts.track;
* argv[] is an array of constant strings; cannot point into it with opts.newbranch without making the latter also a constant string.
* the logic is to strip "refs/" if there is one, "remotes/" if there is one after that, and then strip one level after that unconditionally. No need to look explicitly for a slash while doing the first two steps.
cache.h | 1 + builtin-checkout.c | 26 +++++++++++++------------- 2 files changed, 14 insertions(+), 13 deletions(-)
cache.h +1 -0
@@ -451,6 +451,7 @@ enum safe_crlf {extern enum safe_crlf safe_crlf;enum branch_track { +BRANCH_TRACK_UNSPECIFIED =-1 ,BRANCH_TRACK_NEVER =0 ,BRANCH_TRACK_REMOTE ,BRANCH_TRACK_ALWAYS ,
builtin-checkout.c +13 -13
@@ -157,7 +157,7 @@ struct checkout_opts {int force;int writeout_error; -char *new_branch; +const char *new_branch;int new_branch_log;enum branch_track track; };
@@ -437,27 +437,27 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)git_config (git_default_config,NULL ); - opts.track =-1 ; + opts.track =BRANCH_TRACK_UNSPECIFIED ; argc =parse_options (argc, argv, options, checkout_usage,PARSE_OPT_KEEP_DASHDASH );/* --track without -b should DWIM */ -if (opts.track && opts.track !=-1 && !opts.new_branch) { -char *slash; -if (!argc || !strcmp (argv[0 ],"--" )) +if (0 < opts.track && !opts.new_branch) { +const char *argv0 = argv[0 ]; +if (!argc || !strcmp (argv0,"--" ))die ("--track needs a branch name" ); - slash =strchr (argv[0 ],'/' ); -if (slash && !prefixcmp (argv[0 ],"refs/" )) - slash =strchr (slash +1 ,'/' ); -if (slash && !prefixcmp (argv[0 ],"remotes/" )) - slash =strchr (slash +1 ,'/' ); -if (!slash || !slash[1 ]) +if (!prefixcmp (argv0,"refs/" )) + argv0 +=5 ; +if (!prefixcmp (argv0,"remotes/" )) + argv0 +=8 ; + argv0 =strchr (argv0,'/' ); +if (!argv0 || !argv0[1 ])die ("Missing branch name; try -b" ); - opts.new_branch = slash +1 ; + opts.new_branch = argv0 +1 ; } -if (opts.track ==-1 ) +if (opts.track ==BRANCH_TRACK_UNSPECIFIED ) opts.track = git_branch_track;if (opts.force && opts.merge)
2008/8/22 Junio C Hamano <[email protected]>:
Quoted 1 line
I however wonder if this is clearer.
It is :)
Quoted 5 lines
* "enum
branch_track" was unsigned; comparing equality with -1 was Ok but we couldn't say 0 < opts.track;* argv[] is an array of constant strings; cannot point into it with opts.newbranch without making the latter also a constant string.
Cleanup, but it is unrelated, isn't it?
Quoted 3 lines
* the logic is to strip "refs/" if there is one, "remotes/" if there is one after that, and then strip one level after that unconditionally. No need to look explicitly for a slash while doing the first two steps.
Maybe that should go in documentation instead of the piece I wrote
Quoted 5 lines · Alex Riesen
2008/8/22 Junio C Hamano <[email protected]>:
Quoted 1 line
I however wonder if this is clearer.
It is :)
Quoted 7 lines
Quoted 5 lines
* "enum
branch_track" was unsigned; comparing equality with -1 was Ok but we couldn't say 0 < opts.track;* argv[] is an array of constant strings; cannot point into it with opts.newbranch without making the latter also a constant string.
Cleanup, but it is unrelated, isn't it?
The code did not compile without it for me as I sometimes use -Werror.
Quoted 5 lines
Quoted 3 lines
* the logic is to strip "refs/" if there is one, "remotes/" if there is one after that, and then strip one level after that unconditionally. No need to look explicitly for a slash while doing the first two steps.
Maybe that should go in documentation instead of the piece I wrote
Oh I think what you wrote is fine. I tried to be more descriptive than simply saying "No need to look explicitly for a slash" while explaining the changes.