Re: [PATCH] Extend "checkout --track" DWIM to support more cases

Alex Riesen [off-list ref] writes:

The code handles additionally "refs/remotes/<something>/name",
"remotes/<something>/name", and "refs/<namespace>/name".

Signed-off-by: Alex Riesen <redacted>
---
...
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).

Thanks; the documentation looks good to me.

+		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(-)

diff --git i/cache.h w/cache.h
index 928ae9f..a097a95 100644
--- i/cache.h
+++ w/cache.h

@@ -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,

diff --git i/builtin-checkout.c w/builtin-checkout.c
index e95eab9..b380ad6 100644
--- i/builtin-checkout.c
+++ w/builtin-checkout.c

@@ -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 [off-list ref]:

I however wonder if this is clearer.

It is :)

 * "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 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

"Alex Riesen" [off-list ref] writes:

2008/8/22 Junio C Hamano [off-list ref]:

quoted

I however wonder if this is clearer.

It is :)

quoted

 * "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

 * 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.