Re: [PATCH 4/4] builtin-remote: add set-head verb
flat view
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:10
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH 4/4] builtin-remote: add set-head verb · Jay Soffian <hidden>
- 2016-06-15 · Re: [PATCH 4/4] builtin-remote: add set-head verb · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH 4/4] builtin-remote: add set-head verb · Jay Soffian <hidden>
- 2016-06-15 · Re: [PATCH 4/4] builtin-remote: add set-head verb · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH 4/4] builtin-remote: add set-head verb · Jeff King <hidden>
Jay Soffian [off-list ref] writes:
Provide a porcelain command for setting/deleting $GIT_DIR/remotes/<remote>/HEAD.
The entire series looks sane from a very cursory look; especially the earlier ones are obviously good. Calling the subcommand a "verb" is somewhat new, though. Existing documentation for git commands that take multiple actions seem to call them subcommands, including "git-remote.txt" itself.
quoted hunk
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index fad983e..80f2cfe 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt@@ -76,6 +76,22 @@ the configuration file format. Remove the remote named <name>. All remote tracking branches and configuration settings for the remote are removed. +'set-head':: + +Sets or deletes the default branch (`$GIT_DIR/remotes/<name>/HEAD`) for +the named remote. Having a default branch for a remote is not required, +but allows the name of the remote to be specified in lieu of a specific +branch. For example, if the default branch for `origin` is set to +`master`, then `origin` may be specified wherever you would normally +specify `origin/master`. ++ +With `-d`, `$GIT_DIR/remotes/<name>/HEAD` is deleted. ++ +With `-a`, the remote is queried to determine its `HEAD`, then +`$GIT_DIR/remotes/<name>/HEAD` is set to the same branch. ++ +Use `<branch>` to set `$GIT_DIR/remotes/<name>/HEAD` explicitly. +
Hmph, what does "-a" stand for? I would have expected to see "-u" that stands for "update" here. Also it may be better to be more explicit about both the syntax and the semantics of `<branch>`. Do you expect "refs/remotes/<name>/master" or just "master" (I assume the latter)? Is it an error if the branch does not exist in the specified hierarchy? Can you force to set to a branch that does not exist in your tracking side (yet) but you know exists on the remote side already?
quoted hunk
diff --git a/builtin-remote.c b/builtin-remote.c index 465c87a..677e20e 100644 --- a/builtin-remote.c +++ b/builtin-remote.c@@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states) string_list_clear(&states->new, 0); string_list_clear(&states->stale, 0); string_list_clear(&states->tracked, 0); - free(states->head_name); + if (states->head_name) + free(states->head_name); }
Regression?
quoted hunk
@@ -777,6 +779,54 @@ static int show(int argc, const char **argv) return result; } +static int sethead(int argc, const char **argv)
set_head()?
quoted hunk
+{ + int opt_a = 0, opt_d = 0, result = 0; + struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; + char *head_name = NULL; + + struct option options[] = { + OPT_GROUP("set-head specific options"), + OPT_BOOLEAN('a', 0, &opt_a, + "set refs/remotes/<name>/HEAD according to remote"), + OPT_BOOLEAN('d', 0, &opt_d, "delete refs/remotes/<name>/HEAD"), + OPT_END() + }; + argc = parse_options(argc, argv, options, builtin_remote_usage, 0); + if ((argc == 1 && !(opt_a || opt_d)) || + ((argc == 2 && (opt_a || opt_d))) || argc < 1 || argc > 2) + usage_with_options(builtin_remote_usage, options);
The code will scale better, especially for a young subcommand that may acquire
new options, if the check is done by each codepath that deals with a
specific option to do this kind of check. That is, e.g.
if (opt_delete) {
error if the arg is not remote (alone)
do the "delete" thing
} else if (opt_update) {
error if the arg is not remote (alone)
do the "update" thing
} else {
error if the args are not (remote, branch)
do the "set" thing
}