Re: [PATCH v2] git-clone: Add option --branch to override initial branch

[Junio C Hamano <gitster@pobox.com>]

Tor Arne Vestbø <torarnv@gmail.com> writes:

> The options --branch and -b allow the user to override the initial
> branch created and checked out by git-clone (normally this is the
> active branch of the remote repository).
>
> If the selected branch is not found the operation aborts.
>
> Signed-off-by: Tor Arne Vestbø <torarnv@gmail.com>

The semantics and desirability of the new feature have been already
discussed, and I am not convinced that it is necessary, in the sense that
I do not think I likely ever use this myself, but I am just one of git
users so that is not a strong basis for rejection.

I'll let others discuss more about the design issues, and will only talk
about code in this message.

> diff --git a/builtin-clone.c b/builtin-clone.c
> index c338910..5fc01ce 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -38,6 +38,7 @@ static int option_quiet, option_no_checkout, option_bare, option_mirror;
>  static int option_local, option_no_hardlinks, option_shared;
>  static char *option_template, *option_reference, *option_depth;
>  static char *option_origin = NULL;
> +static char *option_branch = NULL;

I see this was copied from the line immediately above, but please do not
initialize static variables to 0 or NULL.  BSS will take care of it.

> @@ -372,7 +375,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	const char *repo_name, *repo, *work_tree, *git_dir;
>  	char *path, *dir;
>  	int dest_exists;
> -	const struct ref *refs, *head_points_at, *remote_head, *mapped_refs;
> +	const struct ref *refs, *mapped_refs;
> +	const struct ref *remote_head = NULL;
> +	const struct ref *head_points_at = NULL;
>  	struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
>  	struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
>  	struct transport *transport = NULL;
> @@ -545,12 +550,31 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  		mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf);
>  
> -		head_points_at = locate_head(refs, mapped_refs, &remote_head);
> +		if (option_branch) {
> +		    const int offset = 11;
> +		    const char *branch = option_branch;

One indent level in git code equals a HT, i.e. 8 places.

> +		    if (!prefixcmp(branch, "refs/heads/"))
> +			branch += offset;

I suspect that you are trying to protect your code against somebody
miscounting the length of "refs/heads/" (perhaps when updating this
codepath in git version 47 that keeps local branches somewhere else, such
as "refs/local-heads/"), but this "const int offset" does not buy you
anything.  He will likely to leave "offset" to 11 just the same.

It is a different story if it were done like this:

		static const char heads_prefix[] = "refs/heads/";
                if (!prefixcmp(branch, heads_prefix))
                	branch += strlen(heads_prefix);

to let the compiler notice heads_prefix is a constant and optimize the
strlen() out, but I personally think it is overkill.

> +		    const struct ref *r;

We do not tolerate decl-after-statement.

> +		    for (r = mapped_refs; r; r = r->next) {
> +			if (!strcmp(r->name + offset, branch)) {
> +			    /* Override initial branch */
> +			    head_points_at = r;
> +			    remote_head = r;
> +			    break;
> +			}
> +		    }

This duplicates major part of what locate_head() does but with a different
target other than "master", doesn't it?

You would want to refactor this, but I think 'next/pu' already has some
refactoring of the locate_head() logic, so you may want to look at it and
either build your changes on top of it, or wait until that other topic to
stabilize.

> +		    if (!head_points_at)
> +			die("remote has no branch named '%s'.", option_branch);
> +
> +		} else {
> +		    head_points_at = locate_head(refs, mapped_refs, &remote_head);
> +		}

This falls into more personal taste than coding guideline, but it often is
easier to read to arrange your code:

	if (... condition ...) {
        	shorter codepath
	} else {
        	much
                longer
                code
                path
	}

For one thing, it is much easier to miss a short "else" clause hanging at
the end of loooong "if" part.