Re: [PATCH v2] git-clone: Add option --branch to override initial branch
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:19
Tor Arne Vestbø [off-list ref] 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ø <redacted>
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.
quoted hunk
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.
quoted hunk
@@ -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.