Jay Soffian <jaysoffian@xxxxxxxxx> 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.
> 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?
> 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?
> @@ -777,6 +779,54 @@ static int show(int argc, const char **argv)
> return result;
> }
>
> +static int sethead(int argc, const char **argv)
set_head()?
> +{
> + 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
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
