Re: [PATCH 1/3] checkout: make orphaned commit checks configurable

[Junio C Hamano <gitster@pobox.com>]

"Wyatt Holden via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Wyatt Holden <wyattholden@microsoft.com>
>
> When leaving a detached HEAD, checkout enumerates refs and walks
> history to warn about commits left behind. This work can dominate
> checkout time even after the working tree has been updated.

Interesting.

I often am on a detached HEAD but never realized this particular
check was expensive.  The size of changes between the HEAD and the
next branch would affect the time it needs to update the working
tree (the larger the difference, the more expensive to complete the
branch switching).  What factors are involved in making this
"orphaned commits" check more expensive?  The total number of
branches?  The length of commit chain we are leaving behind?  I am
trying to see if this is something we can aim to optimize or if it
is a lost cause and a cop-out to introduce an option to disable the
check altogether is the best we can do.

> In an automated checkout of an internal monorepo, unpack_trees ended
> about 12 seconds into a roughly 204-second run. Stack dumps captured
> the main thread in orphaned_commit_warning(), enumerating loose refs
> and opening ref files. Using --quiet reduced the observed checkout
> time to about 14 seconds, but also suppressed unrelated output.
> These observations motivate an opt-out; they do not establish how
> much of the delay came from enumeration versus history traversal.

So, in short, the answer to my earlier question is "we don't know".

> Add checkout.orphanedCommitWarning, enabled by default, so checkout
> and switch can skip the check while retaining normal checkout output.
> Automated callers can opt out when they do not need a warning about
> leaving detached commits behind.

Sounds sensible.

> Follow the configuration precedent of 06b324c1d71 (status: add
> status.aheadbehind setting, 2019-06-18). Unlike status, introduce no
> dedicated command-line option; callers can use git -c for a one-off
> override.

I am not sure how justifiable the design decision to leave the
command line option out is.

The automation can run with "-q" to omit the overhead altogether, as
well as squelching any other output the automation would not read
anyway, so I would not worry too much about them.

But for human users, what are they losing by passing "-q" when they
do not want to pay the cost of this check because we are not giving
them an easy to use command line option?  What other output are they
declining by "--quiet"?  "The following paths have local changes"
list?  Anything else?

> Add a Trace2 region around the check to make its cost visible in
> future investigations and to verify that disabling the setting skips
> the routine rather than merely hiding its warning. Document the
> setting and test both checkout and switch.

Sounds sensible.

> @@ -1132,6 +1134,7 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne
>  	struct rev_info revs;
>  	struct object *object = &old_commit->object;
>  
> +	trace2_region_enter("checkout", "orphaned_commit_warning", the_repository);
>  	repo_init_revisions(the_repository, &revs, NULL);
>  	setup_revisions(0, NULL, &revs, NULL);
>  
> @@ -1155,6 +1158,7 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne
>  	/* Clean up objects used, as they will be reused. */
>  	repo_clear_commit_marks(the_repository, ALL_REV_FLAGS);
>  	release_revisions(&revs);
> +	trace2_region_leave("checkout", "orphaned_commit_warning", the_repository);
>  }


Quite straightforward.  There is no early return in the middle, so
entering at the beginning with leaving at the end is the only change
necessary.

> @@ -1253,8 +1257,14 @@ static int switch_branches(const struct checkout_opts *opts,
>  		}
>  	}
>  
> -	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)
> -		orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
> +	if (!opts->quiet && !old_branch_info.path && old_branch_info.commit &&
> +	    new_branch_info->commit != old_branch_info.commit) {
> +		if (opts->orphaned_commit_warning)
> +			orphaned_commit_warning(old_branch_info.commit, new_branch_info->commit);
> +		else
> +			describe_detached_head(_("Previous HEAD position was"),
> +					       old_branch_info.commit);
> +	}

OK.  The call into suggest_reattach() via orphaned_commit_warning()
is the more expensive part, and it also is where "Previous HEAD
was" comes, which we do not want to lose.  It makes sense.

Another thing I noticed is that the loop in suggest_reattach() stops
showing each commit after showing ORPHAN_CUTOFF but keeps counting.
I wonder if it becomes palatable to introduce a configuration to
stop it early, instead of disabling the orphaned_commit_warning()
altogether?