Re: [PATCH 4/4] builtin-remote: add set-head verb

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2009, 10:09 UTC
Message-ID: <7vtz6yabsy.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1234515275-91263-5-git-send-email-jaysoffian@gmail.com>

Jay Soffian <jaysoffian@gmail.com> 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.

Show 24 quoted lines

> 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?

Show 12 quoted lines

> 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?

Show 5 quoted lines

> @@ -777,6 +779,54 @@ static int show(int argc, const char **argv)
>  	return result;
>  }
>  
> +static int sethead(int argc, const char **argv)

set_head()?

Show 16 quoted lines

> +{
> +	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
	}