checkout: add simple check for 'git checkout -b'

From: Derrick Stolee <[email protected]>

The 'git switch' command was created to separate half of the behavior of 'git checkout'. It specifically has the mode to do nothing with the index and working directory if the user only specifies to create a new branch and change HEAD to that branch. This is also the behavior most users expect from 'git checkout -b', but for historical reasons it also performs an index update by scanning the working directory. This can be slow for even moderately-sized repos.

A performance fix for 'git checkout -b' was introduced by fa655d8411 (checkout: optimize "git checkout -b <new_branch>" 2018-08-16). That change includes details about the config setting checkout.optimizeNewBranch when the sparse-checkout feature is required. The way this change detected if this behavior change is safe was through the skip_merge_working_tree() method. This method was complex and needed to be updated as new options were introduced.

This behavior was essentially reverted by 65f099b ("switch:
no worktree status unless real branch switch happens" 2019-03-29). Instead, two members of the checkout_opts struct were used to distinguish between 'git checkout' and 'git switch':

* switch_branch_doing_nothing_is_ok
* only_merge_on_switching_branches

These settings have opposite values depending on if we start in cmd_checkout or cmd_switch.

The message for 64f099b includes "Users of big repos are encouraged to move to switch." Making this change while 'git switch' is still experimental is too aggressive.

Create a happy medium between these two options by making 'git checkout -b <branch>' behave just like 'git switch', but only if we read exactly those arguments. This must be done in cmd_checkout to avoid the arguments being consumed by the option parsing logic.

This differs from the previous change by fa644d8 in that the config option checkout.optimizeNewBranch remains deleted. This means that 'git checkout -b' will ignore the index merge even if we have a sparse-checkout file. While this is a behavior change for 'git checkout -b', it matches the behavior of 'git switch -c'.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/checkout.c | 9 +++++++++
 1 file changed, 9 insertions(+)

builtin/checkout.c +9 -1

index 6123f732a2..116200cf90 100644
@@ -1713,6 +1713,15 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
        opts.overlay_mode = -1;
        opts.checkout_index = -2;    /* default on */
        opts.checkout_worktree = -2; /* default on */
+
+       if (argc == 3 && !strcmp(argv[1], "-b")) {
+               /*
+                * User ran 'git checkout -b <branch>' and expects
+                * the same behavior as 'git switch -c <branch>'.
+                */
+               opts.switch_branch_doing_nothing_is_ok = 0;
+               opts.only_merge_on_switching_branches = 1;
+       }
 
        options = parse_options_dup(checkout_options);
        options = add_common_options(&opts, options);
--

gitgitgadget

From: Derrick Stolee <[email protected]>

The 'git switch' command was created to separate half of the behavior of 'git checkout'. It specifically has the mode to do nothing with the index and working directory if the user only specifies to create a new branch and change HEAD to that branch. This is also the behavior most users expect from 'git checkout -b', but for historical reasons it also performs an index update by scanning the working directory. This can be slow for even moderately-sized repos.

A performance fix for 'git checkout -b' was introduced by fa655d8411 (checkout: optimize "git checkout -b <new_branch>" 2018-08-16). That change includes details about the config setting checkout.optimizeNewBranch when the sparse-checkout feature is required. The way this change detected if this behavior change is safe was through the skip_merge_working_tree() method. This method was complex and needed to be updated as new options were introduced.

This behavior was essentially reverted by 65f099b ("switch:
no worktree status unless real branch switch happens" 2019-03-29). Instead, two members of the checkout_opts struct were used to distinguish between 'git checkout' and 'git switch':

* switch_branch_doing_nothing_is_ok
* only_merge_on_switching_branches

These settings have opposite values depending on if we start in cmd_checkout or cmd_switch.

The message for 64f099b includes "Users of big repos are encouraged to move to switch." Making this change while 'git switch' is still experimental is too aggressive.

Create a happy medium between these two options by making 'git checkout -b <branch>' behave just like 'git switch', but only if we read exactly those arguments. This must be done in cmd_checkout to avoid the arguments being consumed by the option parsing logic.

This differs from the previous change by fa644d8 in that the config option checkout.optimizeNewBranch remains deleted. This means that 'git checkout -b' will ignore the index merge even if we have a sparse-checkout file. While this is a behavior change for 'git checkout -b', it matches the behavior of 'git switch -c'.

Signed-off-by Derrick Stolee <[email protected]>

---
  builtin/checkout.c | 9 +++++++++
  1 file changed, 9 insertions(+)

builtin/checkout.c +0 -0

index 6123f732a2..116200cf90 100644

builtin/checkout.c +5 -0

+		 * the same behavior as 'git switch -c <branch>'.
+		 */
+		opts.switch_branch_doing_nothing_is_ok = 0;
+		opts.only_merge_on_switching_branches = 1;
+	}
 
  	options = parse_options_dup(checkout_options);
  	options = add_common_options(&opts, options);

Hi Stolee

Quoted 4 lines · Derrick Stolee via GitGitGadget

+
+    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.

Quoted 8 lines · Phillip Wood

Hi Stolee

Quoted 4 lines · Derrick Stolee via GitGitGadget

+
+    if (argc == 3 && !strcmp(argv[1], "-b")) {
+        /*
+         * User ran 'git checkout -b <branch>' and expects

Quoted 1 line

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.

Quoted 15 lines · Derrick Stolee

Quoted 9 lines · Phillip Wood

Hi Stolee

Quoted 4 lines · Derrick Stolee via GitGitGadget

+
+    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.

Quoted 2 lines

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.

Quoted 16 lines · Pratyush Yadav

Quoted 15 lines · Derrick Stolee

Quoted 9 lines · Phillip Wood

Hi Stolee

builtin/checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 1719

Quoted 4 lines · Derrick Stolee via GitGitGadget

builtin/checkout.c +4 -0

+
+    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.

Quoted 8 lines

Quoted 2 lines

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>)

Quoted 37 lines · Pratyush Yadav

Quoted 16 lines · Pratyush Yadav

Quoted 15 lines · Derrick Stolee

Quoted 9 lines · Phillip Wood

Hi Stolee

builtin/checkout.c int cmd_checkout(int argc, const char **argv, const char *prefix) · line 1719

Quoted 4 lines · Derrick Stolee via GitGitGadget

builtin/checkout.c +4 -0

+
+    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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Just my $0.02,
Elijah

Hi Elijah,

Quoted 11 lines · Elijah Newren

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.

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.