From: ZheNing Hu <adlternative@gmail.com>
Date: Sat, 11 Dec 2021 06:34:46 GMT
Subject: Re: [PATCH 2/2] checkout: introduce "--to-branch" option
Message-ID: <CAOLTT8SLQXbH_GGF1sArVSsZ1b=poqMPhdSWOkWPS0f_SGoONw@mail.gmail.com>
In-Reply-To: <CAP8UFD0MK++3QdMAvJ6Az0LZyXUWXRJcRQPG1THKw4CFW0yRLg@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> 于2021年12月10日周五 16:34写道：

> >  [verse]
> >  'git checkout' [-q] [-f] [-m] [<branch>]
> >  'git checkout' [-q] [-f] [-m] --detach [<branch>]
> > -'git checkout' [-q] [-f] [-m] [--detach] <commit>
> > +'git checkout' [-q] [-f] [-m] [--detach] [-w|--to-branch] <commit>
>
> It's a bit strange that --detach can be used along with the new
> option, as the purpose of the new option is to not detach. It makes
> one wonder what happens when both --detach and --to-branch are used.
>

When both --detach and --to-branch are used, --detach will work...
Of course, it should be reasonable to prevent them from being used at the
same time.

> I wonder if all the following lines:
>
>       git checkout [-q] [-f] [-m] [<branch>]
>       git checkout [-q] [-f] [-m] --detach [<branch>]
>       git checkout [-q] [-f] [-m] [--detach] <commit>
>
> could be replaced with just:
>
>       git checkout [-q] [-f] [-m] [--detach|--to-branch] <commitish>
>

Well, it will be much more concise.

> >  'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
> >  'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...
> >  'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]
> > @@ -210,6 +210,12 @@ variable.
> >         `<commit>` is not a branch name.  See the "DETACHED HEAD" section
> >         below for details.
> >
> > +-w::
> > +--to-branch::
>
> Using a short option name like "-w" might not be a good idea at this
> point. Maybe if many people use the long option a lot, they will want
> a short option name, but we can add it then instead of using one of
> the few left right now.
>

Makes sense.

> > +       Rather than checking out a commit to work on it, checkout out
>
> "checking out a commit to work on it" might not describe well that it
> works when we pass a tag too and that we checkout the underlying
> commit in the detached HEAD mode by default.
>
> > +       to the unique branch on it. If there are multiple branches on
> > +       the commit, the checkout will fail.
>
> It might be a bit better to say that a branch "points to" a commit,
> rather than that it is "on" a commit.
>

OK.

> >  static const char * const checkout_usage[] = {
> >         N_("git checkout [<options>] <branch>"),
> > @@ -70,6 +71,7 @@ struct checkout_opts {
> >         int empty_pathspec_ok;
> >         int checkout_index;
> >         int checkout_worktree;
> > +       int to_branch;
> >         const char *ignore_unmerged_opt;
> >         int ignore_unmerged;
> >         int pathspec_file_nul;
> > @@ -1512,6 +1514,35 @@ static int checkout_branch(struct checkout_opts *opts,
> >                     (flag & REF_ISSYMREF) && is_null_oid(&rev))
> >                         return switch_unborn_to_new_branch(opts);
> >         }
> > +       if (opts->to_branch) {
> > +               struct ref_filter filter;
> > +               struct ref_array array;
> > +               int i;
> > +               int count = 0;
> > +               const char *unused_pattern = NULL;
> > +
> > +               memset(&array, 0, sizeof(array));
> > +               memset(&filter, 0, sizeof(filter));
> > +               filter.kind = FILTER_REFS_BRANCHES;
> > +               filter.name_patterns = &unused_pattern;
> > +               filter_refs(&array, &filter, filter.kind);
> > +               for (i = 0; i < array.nr; i++) {
> > +                       if (oideq(&array.items[i]->objectname, &new_branch_info->oid)) {
> > +                               if (count)
> > +                                       die(_("here are more than one branch on commit %s"), oid_to_hex(&new_branch_info->oid));
> > +                               count++;
> > +                               if (new_branch_info->refname)
> > +                                       free((char *)new_branch_info->refname);
> > +                               new_branch_info->refname = xstrdup(array.items[i]->refname);
> > +                               if (new_branch_info->path)
> > +                                       free((char *)new_branch_info->path);
> > +                               new_branch_info->path = xstrdup(array.items[i]->refname);
> > +                               new_branch_info->name = new_branch_info->path;
> > +                       }
> > +               }
> > +               ref_array_clear(&array);
>
> It might be my personal taste, but I would find it cleaner and easier
> to understand if a separate function to find the branch we are looking
> for was called, instead of adding all the code here.
>

You are right.

> > +       }
> > +
> >         return switch_branches(opts, new_branch_info);
> >  }
> >
> > @@ -1797,6 +1828,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> >                 OPT_BOOL('l', NULL, &opts.new_branch_log, N_("create reflog for new branch")),
> >                 OPT_BOOL(0, "guess", &opts.dwim_new_local_branch,
> >                          N_("second guess 'git checkout <no-such-branch>' (default)")),
> > +               OPT_BOOL('w', "to-branch", &opts.to_branch,
> > +                        N_("checkout to a branch from a commit or a tag")),
> >                 OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode (default)")),
> >                 OPT_END()
> >         };
> > diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> > index 93be1c0eae5..53e45cfe7fd 100755
> > --- a/t/t2018-checkout-branch.sh
> > +++ b/t/t2018-checkout-branch.sh
>
> I plan to look at the tests after we decide how the new option relates
> to --detach.
>
> Thanks!

Thanks.
--
ZheNing Hu