Re: [PATCH 1/1] checkout: add simple check for 'git checkout -b'

[Taylor Blau <me@ttaylorr.com>]

Hi Elijah,

On Thu, Aug 29, 2019 at 05:19:44PM -0700, Elijah Newren wrote:
> On Thu, Aug 29, 2019 at 2:42 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> >
> > On 30/08/19 02:00AM, Pratyush Yadav wrote:
> > > On 29/08/19 04:07PM, Derrick Stolee wrote:
> > > > On 8/29/2019 2:54 PM, Phillip Wood wrote:
> > > > > Hi Stolee
> > > > >
> > > > > On 29/08/2019 18:01, Derrick Stolee via GitGitGadget wrote:
> > > > >> +
> > > > >> +    if (argc == 3 && !strcmp(argv[1], "-b")) {
> > > > >> +        /*
> > > > >> +         * User ran 'git checkout -b <branch>' and expects
> > > > >
> > > > > What if the user ran 'git checkout -b<branch>'? Then argc == 2.
> > > >
> > > > Good catch. I'm tempted to say "don't do that" to keep this
> > > > simple. They won't have incorrect results, just slower than
> > > > the "with space" option.
> > > >
> > > > However, if there is enough interest in correcting the "-b<branch>"
> > > > case, then I can make another attempt at this.
> > >
> > > You can probably do this with:
> > >
> > >   !strncmp(argv[1], "-b", 2)
> > >
> > > The difference is so little, might as well do it IMO.
> >
> > Actually, that is not correct. I took a quick look before writing this
> > and missed the fact that argc == 3 is the bigger problem.
> >
> > Thinking a little more about this, you can mix other options with
> > checkout -b, like --track. You can also specify <start_point>.
> >
> > Now I don't know enough about this optimization you are doing to know
> > whether we need to optimize when these options are given, but at least
> > for --track I don't see any reason not to.
> >
> > So maybe you are better off using something like getopt() (warning:
> > getopt modifies the input string so you probably want to duplicate it)
> > if you want to support all cases. Though for this simple case you can
> > probably get away by just directly scanning the argv list for "-b"
> > (using strncmp instead of strcmp to account for "-b<branch-name>)
>
> NO.  This would be unsafe to use if <start_point> is specified.  I
> think either -f or -m together with -b make no sense unless
> <start_point> is specified, but if they do make sense separately, I'm
> guessing this hack should not be used with those flags.  And
> additional flags may appear in the future that should not be used
> together with this hack.
>
> Personally, although I understand the desire to support any possible
> cases in general, *this is a performance hack*.  As such, it should be
> as simple and localized as possible.  I don't think supporting
> old-style stuck flags (-b$BRANCH) is worth complicating this.  I'm
> even leery of adding support for --track (do any users of huge repos
> use -b with --track?  Does anyone at all use --track anymore?  I'm not
> sure I've ever seen any user use that flag in the last 10 years other
> than myself.)  Besides, in the *worst* possible case, the command the
> user specifies works just fine...it just takes a little longer.  My
> opinion is that Stolee's patch is perfect as-is and should not be
> generalized at all.

I wholeheartedly agree with this, and pledge my $.02 towards it as well.
Now with a combined total of $.04, I think that this patch is ready for
queueing as-is.

> Just my $0.02,
> Elijah

Thanks,
Taylor