Re: [PATCH v5 6/6] add worktree.guessRemote config option
- From
Thomas Gummerer <t.gummerer@gmail.com>
- Date
- Nov 27, 2017, 20:59 UTC
- Message-ID
- <20171127205918.GB32057@hank>
- In-Reply-To
- <xmqq8tescj9p.fsf@gitster.mtv.corp.google.com>
On 11/27, Junio C Hamano wrote:
Show 54 quoted lines
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > +worktree.guessRemote::
> > + With `add`, if no branch argument, and neither of `-b` nor
> > + `-B` nor `--detach` are given, the command defaults to
> > + creating a new branch from HEAD. If `worktree.guessRemote` is
> > + set to true, `worktree add` tries to find a remote-tracking
> > + branch whose name uniquely matches the new branch name. If
> > + such a branch exists, it is checked out and set as "upstream"
> > + for the new branch. If no such match can be found, it falls
> > + back to creating a new branch from the current HEAD.
>
> Unlike the part I commented on in the previous step, this one is
> clear that the feature only kicks in for 'add <path>' without
> anything else, which is good.
>
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 15cb1600ee..426aea8761 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -33,8 +33,19 @@ struct add_opts {
> >
> > static int show_only;
> > static int verbose;
> > +static int guess_remote;
> > static timestamp_t expire;
> >
> > +static int git_worktree_config(const char *var, const char *value, void *cb)
> > +{
> > + if (!strcmp(var, "worktree.guessremote")) {
> > + guess_remote = git_config_bool(var, value);
> > + return 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> It is a lot more consistent with the established practice if this
> function had
>
> return git_default_config(var, value, cb);
>
> instead of "return 0" at the end, and then have the call to
>
> git_config(git_default_config, NULL);
>
> we have in cmd_worktree() replaced with
>
> git_config(git_worktree_config, NULL);
>
> That would avoid having to scan the entire set of config keys once
> in cmd_worktree() and then again in add(), the latter one only
> looking for a single variable.Makes sense, I missed that. I'll fix it in the re-roll. I'll wait a few days to see if there are any more comments on the series and then re-roll it with the suggested changes.
Thanks for the review!