Re: [PATCH 1/3] git-branch: add --track and --no-track options
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Mar 5, 2007, 14:50 UTC
- Message-ID
- <Pine.LNX.4.63.0703051520340.22628@wbgn013.biozentrum.uni-wuerzburg.de>
- In-Reply-To
- <esgm1m$33f$1@sea.gmane.org>
Hi,
it is a very good idea to read the information which remote tracks which branch from the config, i.e. if you branch "refs/remotes/hallo", to look if there is any remote information for that local ref.
On Mon, 5 Mar 2007, Paolo Bonzini wrote:
Show 18 quoted lines
> diff --git a/builtin-branch.c b/builtin-branch.c > index d371849..f39759b 100644 > --- a/builtin-branch.c > +++ b/builtin-branch.c > @@ -12,7 +12,7 @@ > #include "builtin.h" > > static const char builtin_branch_usage[] = > - "git-branch [-r] (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length> | --no-abbrev]]"; > + "git-branch [-r] (-d | -D) <branchname> | [--track | --no-track] [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length> | --no-abbrev]]"; > > #define REF_UNKNOWN_TYPE 0x00 > #define REF_LOCAL_BRANCH 0x01 > @@ -308,14 +308,105 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev) > free_ref_list(&ref_list); > } > > +
extra empty line.
Show 6 quoted lines
> +static void get_remote_branch_name(const char* value)
> +{
> + int len_first = -1, len_second = -1;
> + if (*value == '+')
> + value++;
> +> + /* Try an exact match first. */ > + sscanf(value, "refs/%*[^:]:%n", &len_first);
This is the first time I saw that sscanf format type. How portable is it?
> + if (len_first != -1
> + && !strcmp(value + len_first, start_ref)) {I really would rather do
const char *p;
if (!prefixcmp(value, "refs/") && (p = strchr(value, ':')) &&
!strcmp(p + 1, start_ref)) {> + /* Truncate the value before the colon. */ > + asprintf(&config_repo, "%.*s", len_first - 1, value);
asprintf() is a GNU extension. I guess it is better to just
config_repo = xstrdup(value); config_repo[p - value] = '\0';
Show 13 quoted lines
> + return;
> + }
> +
> + /* Try with a wildcard match now. */
> + sscanf(value, "refs/%*[^/]/*:%nrefs/remotes/%*[^/]/*%n",
> + &len_first, &len_second);
> + if (len_first != -1 && len_second != -1
> + && (len_second - 2) - len_first == remote_len + 13
> + && !strncmp(value + len_first, start_ref, remote_len + 13)) {
> + /* Replace the star with the remote branch name. */
> + asprintf(&config_repo, "%.*s%s",
> + len_first - 3, value,
> + start_ref + remote_len + 13);Same here:
else if (!prefixcmp(value, "refs/") && (p = strchr(value, ':')) &&
!memcmp(p + 1, start_ref, remote_len) &&
!strcmp(p + 1 + remote_len, "/*")) {
config_repo = xstrdup(value);
config_repo[p - value] = '\0';
}BTW I prefer to skip the curly brackets when there is only one statement in the block.
Just to be on the safe side, you might want to check here if there are more than one remotes "tracking" into start_ref. However, it might not be relevant in practice.
Show 9 quoted lines
> + }
> +}
> +
> +static int get_remote_config(const char* key, const char* value)
> +{
> + if (!prefixcmp(key, "remote.") &&
> + !strncmp(key + 7, start_ref + 13, remote_len)) {
> + if (config_track == -1
> + && !strcmp(key + 7 + remote_len, ".trackintolocalbranches"))FWIW I don't think .trackIntoLocalBranches" is needed. Opinions?
> +static void register_pull(const char *name, const char *real_ref, int track)
Maybe call it "set_branch_defaults()"?
> + die("what a long branch name you have!");
> + goto out;die() does not return, so no need to "goto out;"... But then, it might be nicer to return -1, i.e.
ret = error("what a long branch name you have!");and saying
if (!ret) {
git_config_set(key, value);
...
printf("Branch %s setup...
}
}
if (repo_config)
free(repo_config);
return ret;But I see you made it return "void", so you can skip the "return ret;".
Show 7 quoted lines
> @@ -333,7 +424,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) > 1)
> + die("Ambiguous object name: '%s'.", start_name);I know, I should have said that earlier, but I just found out myself: We have a config variable core.warnambiguousrefs, and maybe we should _not_ complain and set the defaults when the global variable warn_ambiguous_refs is 0.
Ciao, Dscho