Tor Arne Vestbø <torarnv@xxxxxxxxx> 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@xxxxxxxxx>
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.
--
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
