Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch
- From
Junio C Hamano <junkio@cox.net>
- Date
- Mar 2, 2007, 09:52 UTC
- Message-ID
- <7vtzx4ezvm.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <45E7E53E.5020704@gnu.org>
Paolo Bonzini <paolo.bonzini@gmail.com> writes:
> A rather standard (in 1.5) procedure for branching off a remote archive > is:
Much easier to read, thanks.
Although I'll still nitpick a few points...
> > git checkout -b branchname remote/upstreambranch > git config --add branch.branchname.remote remote > git config --add branch.branchname.merge refs/heads/upstreambranch
Probably a rather standard procedure would be to fork from remote tracking branch of where you cloned from, i.e. under 'remotes/origin'. I think the above examples are a bit easier to read if you said:
In order to track and build on top of a branch 'topic' you
track from your upstream repository, you often would end up
doing this sequence: $ git checkout -b topic origin/topic
$ git config --add branch.branchname.remote origin
$ git config --add branch.branchname.merge refs/heads/topic to fork your own 'topic' branch from the corresponding
branch you track from the 'origin' repository, and set up
two configuration variables so that 'git pull' without
parameters does the right thing while you are on your own
'topic' branch. This commit teaches --track option to git-branch, so that
"git branch --track topic origin/topic" performs the latter
two actions when creating your 'topic' branch. By setting
configuration variable 'branch.trackremotebranches' to true,
you do not have to pass --track option explicitly (the
configuration variable is off by default, and there is a
--no-track option to countermand it even if the variable is
set).Signed-off-by: ...
I do not think a porcelain level command 'branch' should introduce core.* configuration variables.
I have a feeling that "git checkout -b" and "git checkout -B" should be taught to explicitly use "git branch --no-track" and "git branch --track" to create a new branch (currently it does not even use "git branch" as far as I can tell). With your patch, I suspect that you have to say "git branch topic origin/topic" and then "git checkout topic", which means you made the three-step process into two steps, but you could have made it into one step. I'll send out an untested patch to git-checkout so that you can try it out in a separate message.
Show 27 quoted lines
> diff --git a/builtin-branch.c b/builtin-branch.c
> index d0179b0..96658ff 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -308,15 +308,34 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
> free_ref_list(&ref_list);
> }
>
> +static void register_pull (const char *name, const char *remote_branch_name)
> +{
> + char *slash = strchr(remote_branch_name, '/');
> + char key[1024], value[1024];
> +
> + if (strlen(remote_branch_name) >= 1024 - 11
> + || strlen(name) >= 1024 - 15)
> + die ("what a long branch name you have!");
> +
> + snprintf(key, sizeof(key), "branch.%s.remote", name);
> + snprintf(value, sizeof(value), "%.*s", slash - remote_branch_name,
> + remote_branch_name);
> + git_config_set(key, value);
> +
> + snprintf(key, sizeof(key), "branch.%s.merge", name);
> + snprintf(value, sizeof(value), "refs/heads/%s", slash + 1);
> + git_config_set(key, value);
> +}
> + - (minor style) No SP between "register_pull" and "(". Found
elsewhere as well.- (minor style) I tend to prefer pure declarations before decls with initializer. I.e. "char key[], value[]" first then "char *slash".
- (discipline) Not 1024 in the comparison. sizeof(key) or sizeof(value).
- (micronit) Is it true that both strlen() tests are about long *branch* names?
- (style and discipline) If you use snprintf(), it is usually easier to check its return value to see if you would have overflowed, without having the if() statement to check the length upfront. As the code gets updated, you may need to change the snprintf() format strings later, and you can forget making a matching change to the condition in if() statement with the patch above.
- (moderately serious) The code blindly trusts that "refs/remotes/foo/bar" tracks "refs/heads/bar" from remote named "foo", which is a bit disturbing. With the default configuration git-clone and git-remote creates, it always is the case, but I suspect you might want to at least verify that assumption (the user can have different settings in the config), if not figuring them out by reading the existing configuration yourself.
Show 11 quoted lines
> @@ -333,7 +354,9 @@ static void create_branch(const char *name, const char *start_name,
> if (start_sha1)
> /* detached HEAD */
> hashcpy(sha1, start_sha1);
> - else if (get_sha1(start_name, sha1))
> + else if (dwim_ref(start_name, strlen(start_name), sha1, &real_ref))
> + remote = !prefixcmp(real_ref, "refs/remotes/");
> + else
> die("Not a valid object name: '%s'.", start_name);
>
> if ((commit = lookup_commit_reference(sha1)) == NULL)- (pure question) What happens if dwim_ref() returns more than one?
Show 16 quoted lines
> diff --git a/config.c b/config.c
> index 0ff413b..49df7bd 100644
> --- a/config.c
> +++ b/config.c
> @@ -294,6 +294,11 @@ int git_default_config(const char *var, const char *value)
> return 0;
> }
>
> + if (!strcmp(var, "core.trackremotebranches")) {
> + track_remote_branches = git_config_bool(var, value);
> + return 0;
> + }
> +
> if (!strcmp(var, "core.legacyheaders")) {
> use_legacy_headers = git_config_bool(var, value);
> return 0;- (mild objection) Does this belong to git_default_config()? I would have expected this to appear in git_branch_config().
Show 12 quoted lines
> diff --git a/environment.c b/environment.c > index 570e32a..e440d05 100644 > --- a/environment.c > +++ b/environment.c > @@ -17,6 +17,7 @@ int assume_unchanged; > int prefer_symlink_refs; > int is_bare_repository_cfg = -1; /* unspecified */ > int log_all_ref_updates = -1; /* unspecified */ > +int track_remote_branches = 0; > int warn_ambiguous_refs = 1; > int repository_format_version; > char *git_commit_encoding;
- (style and discipline) No need to initialize global int to 0. BSS would take care of it.