From: Taylor Blau <me@ttaylorr.com>
To: Elijah Newren <newren@gmail.com>
Cc: Pratyush Yadav <me@yadavpratyush.com>,
Derrick Stolee <stolee@gmail.com>,
Phillip Wood <phillip.wood@dunelm.org.uk>,
Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 1/1] checkout: add simple check for 'git checkout -b'
Date: Thu, 29 Aug 2019 20:43:39 -0400 [thread overview]
Message-ID: <20190830004339.GA34082@syl.lan> (raw)
In-Reply-To: <CABPp-BF_uBTKT_5YmoMNoToiujuMdQia-OfxOPAJPrhT6jPbdA@mail.gmail.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
next prev parent reply other threads:[~2019-08-30 0:43 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-29 17:01 [PATCH 0/1] checkout: add simple check for 'git checkout -b' Derrick Stolee via GitGitGadget
2019-08-29 17:01 ` [PATCH 1/1] " Derrick Stolee via GitGitGadget
2019-08-29 17:25 ` Elijah Newren
2019-08-29 18:54 ` Phillip Wood
2019-08-29 20:07 ` Derrick Stolee
2019-08-29 20:30 ` Pratyush Yadav
2019-08-29 21:40 ` Pratyush Yadav
2019-08-30 0:19 ` Elijah Newren
2019-08-30 0:43 ` Taylor Blau [this message]
2019-08-30 16:56 ` Derrick Stolee
2019-08-30 17:18 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190830004339.GA34082@syl.lan \
--to=me@ttaylorr.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=me@yadavpratyush.com \
--cc=newren@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).