Skip to content

[RFC] Revert/delay performance regression in 'git checkout -b' #317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[RFC] Revert/delay performance regression in 'git checkout -b' #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Documentation/config/checkout.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,11 @@ will checkout the '<something>' branch on another remote,
and by linkgit:git-worktree[1] when 'git worktree add' refers to a
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Wed, Aug 21, 2019 at 12:18:33PM -0700, Derrick Stolee via GitGitGadget wrote:
> This reverts commit 65f099b3988198f0fdf3ef7a21dc01c556d21fff, which
> removed logic for avoiding extra cost in "git checkout -b" in favor
> of the new "git switch -c". This will cause a performance issue for
> users in large repos.

It always makes me sad when I see a pull request, where all the
relevant information is only included in the PR, which will not be
part of the history, but not in the commit messages...


>  builtin/.checkout.c.swp           | Bin 0 -> 77824 bytes

> diff --git a/builtin/.checkout.c.swp b/builtin/.checkout.c.swp
> new file mode 100644
> index 0000000000000000000000000000000000000000..f6dad4abb02c265ee66b3f6f76d00d59b9b524a4
> GIT binary patch
> literal 77824
> zcmeIb37F(pRrf#a46<X03c_z1GgCe3>dYjdA(>2=r6*}KnI2}zLK2GV?&_Y(bXPT1
> z)icux34uRC00rR{;pJ5jL_ico7FiV85d;D8CG1O3By6%HioE<k-*fK$ZB^AXi9sKq

Uh-oh.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 8/21/2019 6:15 PM, SZEDER Gábor wrote:
> On Wed, Aug 21, 2019 at 12:18:33PM -0700, Derrick Stolee via GitGitGadget wrote:
>> This reverts commit 65f099b3988198f0fdf3ef7a21dc01c556d21fff, which
>> removed logic for avoiding extra cost in "git checkout -b" in favor
>> of the new "git switch -c". This will cause a performance issue for
>> users in large repos.
> 
> It always makes me sad when I see a pull request, where all the
> relevant information is only included in the PR, which will not be
> part of the history, but not in the commit messages...

An issue with the RFC quality at the moment. I'll do a better job
if we decide to move this direction.
 
>>  builtin/.checkout.c.swp           | Bin 0 -> 77824 bytes
> 
>> diff --git a/builtin/.checkout.c.swp b/builtin/.checkout.c.swp
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..f6dad4abb02c265ee66b3f6f76d00d59b9b524a4
>> GIT binary patch
>> literal 77824
>> zcmeIb37F(pRrf#a46<X03c_z1GgCe3>dYjdA(>2=r6*}KnI2}zLK2GV?&_Y(bXPT1
>> z)icux34uRC00rR{;pJ5jL_ico7FiV85d;D8CG1O3By6%HioE<k-*fK$ZB^AXi9sKq
> 
> Uh-oh.

Whoops! I thought I removed this in time to submit, but I guess I
did not.

-Stolee

remote branch. This setting might be used for other checkout-like
commands or functionality in the future.

checkout.optimizeNewBranch::
Optimizes the performance of "git checkout -b <new_branch>" when
using sparse-checkout. When set to true, git will not update the
repo based on the current sparse-checkout settings. This means it
will not update the skip-worktree bit in the index nor add/remove
files in the working directory to reflect the current sparse checkout
settings nor will it show the local changes.
133 changes: 126 additions & 7 deletions builtin/checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include "wt-status.h"
#include "xdiff-interface.h"

static int checkout_optimize_new_branch;

static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
N_("git checkout [<options>] [<branch>] -- <file>..."),
Expand Down Expand Up @@ -71,6 +73,11 @@ struct checkout_opts {
const char *ignore_unmerged_opt;
int ignore_unmerged;

/*
* If new checkout options are added, skip_merge_working_tree
* should be updated accordingly.
*/

const char *new_branch;
const char *new_branch_force;
const char *new_orphan_branch;
Expand Down Expand Up @@ -637,6 +644,112 @@ static void setup_branch_path(struct branch_info *branch)
branch->path = strbuf_detach(&buf, NULL);
}

/*
* Skip merging the trees, updating the index and working directory if and
* only if we are creating a new branch via "git checkout -b <new_branch>."
*/
static int skip_merge_working_tree(const struct checkout_opts *opts,
const struct branch_info *old_branch_info,
const struct branch_info *new_branch_info)
{
/*
* Do the merge if sparse checkout is on and the user has not opted in
* to the optimized behavior
*/
if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
return 0;

/*
* We must do the merge if we are actually moving to a new commit.
*/
if (!old_branch_info->commit || !new_branch_info->commit ||
!oideq(&old_branch_info->commit->object.oid,
&new_branch_info->commit->object.oid))
return 0;

/*
* opts->patch_mode cannot be used with switching branches so is
* not tested here
*/

/*
* opts->quiet only impacts output so doesn't require a merge
*/

/*
* Honor the explicit request for a three-way merge or to throw away
* local changes
*/
if (opts->merge || opts->force)
return 0;

/*
* --detach is documented as "updating the index and the files in the
* working tree" but this optimization skips those steps so fall through
* to the regular code path.
*/
if (opts->force_detach)
return 0;

/*
* opts->writeout_stage cannot be used with switching branches so is
* not tested here
*/

/*
* Honor the explicit ignore requests
*/
if (!opts->overwrite_ignore || opts->ignore_skipworktree ||
opts->ignore_other_worktrees)
return 0;

/*
* opts->show_progress only impacts output so doesn't require a merge
*/

/*
* opts->overlay_mode cannot be used with switching branches so is
* not tested here
*/

/*
* If we aren't creating a new branch any changes or updates will
* happen in the existing branch. Since that could only be updating
* the index and working directory, we don't want to skip those steps
* or we've defeated any purpose in running the command.
*/
if (!opts->new_branch)
return 0;

/*
* new_branch_force is defined to "create/reset and checkout a branch"
* so needs to go through the merge to do the reset
*/
if (opts->new_branch_force)
return 0;

/*
* A new orphaned branch requrires the index and the working tree to be
* adjusted to <start_point>
*/
if (opts->new_orphan_branch)
return 0;

/*
* Remaining variables are not checkout options but used to track state
*/

/*
* Do the merge if this is the initial checkout. We cannot use
* is_cache_unborn() here because the index hasn't been loaded yet
* so cache_nr and timestamp.sec are always zero.
*/
if (!file_exists(get_index_file()))
return 0;

return 1;
}

static int merge_working_tree(const struct checkout_opts *opts,
struct branch_info *old_branch_info,
struct branch_info *new_branch_info,
Expand Down Expand Up @@ -1020,7 +1133,6 @@ static int switch_branches(const struct checkout_opts *opts,
void *path_to_free;
struct object_id rev;
int flag, writeout_error = 0;
int do_merge = 1;

trace2_cmd_mode("branch");

Expand All @@ -1039,7 +1151,6 @@ static int switch_branches(const struct checkout_opts *opts,
BUG("'switch --orphan' should never accept a commit as starting point");
new_branch_info->commit = NULL;
new_branch_info->name = "(empty)";
do_merge = 1;
}

if (!new_branch_info->name) {
Expand All @@ -1048,12 +1159,16 @@ static int switch_branches(const struct checkout_opts *opts,
if (!new_branch_info->commit)
die(_("You are on a branch yet to be born"));
parse_commit_or_die(new_branch_info->commit);

if (opts->only_merge_on_switching_branches)
do_merge = 0;
}

if (do_merge) {
/* optimize the "checkout -b <new_branch> path */
if (skip_merge_working_tree(opts, &old_branch_info, new_branch_info)) {
if (!checkout_optimize_new_branch && !opts->quiet) {
if (read_cache_preload(NULL) < 0)
return error(_("index file corrupt"));
show_local_changes(&new_branch_info->commit->object, &opts->diff_options);
}
} else {
ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
if (ret) {
free(path_to_free);
Expand All @@ -1073,6 +1188,11 @@ static int switch_branches(const struct checkout_opts *opts,

static int git_checkout_config(const char *var, const char *value, void *cb)
{
if (!strcmp(var, "checkout.optimizenewbranch")) {
checkout_optimize_new_branch = git_config_bool(var, value);
return 0;
}

if (!strcmp(var, "diff.ignoresubmodules")) {
struct checkout_opts *opts = cb;
handle_ignore_submodules_arg(&opts->diff_options, value);
Expand Down Expand Up @@ -1747,7 +1867,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
opts.accept_ref = 1;
opts.accept_pathspec = 0;
opts.switch_branch_doing_nothing_is_ok = 0;
opts.only_merge_on_switching_branches = 1;
opts.implicit_detach = 0;
opts.can_switch_when_in_progress = 0;
opts.orphan_from_empty_tree = 1;
Expand Down
14 changes: 14 additions & 0 deletions t/t1090-sparse-checkout-scope.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ test_expect_success 'perform sparse checkout of master' '
test_path_is_file c
'

test_expect_success 'checkout -b checkout.optimizeNewBranch interaction' '
cp .git/info/sparse-checkout .git/info/sparse-checkout.bak &&
test_when_finished "
mv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout
git checkout master
" &&
echo "/b" >>.git/info/sparse-checkout &&
test "$(git ls-files -t b)" = "S b" &&
git -c checkout.optimizeNewBranch=true checkout -b fast &&
test "$(git ls-files -t b)" = "S b" &&
git checkout -b slow &&
test "$(git ls-files -t b)" = "H b"
'

test_expect_success 'merge feature branch into sparse checkout of master' '
git merge feature &&
test_path_is_file a &&
Expand Down