Re: [PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 4, 2020, 23:48 UTC
- Message-ID
- <xmqqimlqbx61.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <de0f381284cfe90c1bd8521abb8d29e3529c981a.1578087730.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> + if (!o->update || o->dry_run) {
> + remove_marked_cache_entries(index, 0);
> + trace_performance_leave("check_updates");
> + return 0;
> + }OK, so the idea is that bunch of codepaths we see later are protected by "o->update && !o->dry_run" in the original and are not executed, so we can leave early here.
So the primary task of the reviewers of this patch is to see if the remainder of the function has some code that were *not* no-op when (!o->update || o->dry_run) is true in the current code, which would indicate possible behaviour change, and assess if the change in behaviour matters in the real world if there is.
> if (o->clone) > setup_collided_checkout_detection(&state, index);
If there were such a thing as dry-run of a clone, we will stop calling the report based on the thing we are setting up here? Presumably that does not happen in the current code---is that something we guarantee in the future evolution of the code, though?
> progress = get_progress(o);
get_progress() would yield NULL when !o->update, but o->dry_run does not influence it, so we would have called the progress API in today's code, if o->dry_run and o->update are both true.
Presumably, o->update and o->dry_run are not true at the same time in the current code---but even if in the future we start supporting the combination, dry-run should be skipping the filesystem update (which is the slow part) and lack of progress may not matter, I guess?
It seems to me that unpack_trees_options.dry_run can become true only in "git read-tree" when the -n (or --dry-run) option is given and no other codepath touches it. Am I reading the code correctly?
Similarly, unpack_trees_options.clone is turned on only in the builtin/clone.c::checkout(). It _might_ occur to future developers that "git clone --no-checkout" is better implemented by actually going through builtin/clone.c::checkout() with .dry_run turned on, instead of leaving that function early. That would for example allow collided_checkout() detection to still be done---in such a future, is the optimization this patch makes still safe, I wonder?
Show 7 quoted lines
> - if (o->update) > - git_attr_set_direction(GIT_ATTR_CHECKOUT); > + git_attr_set_direction(GIT_ATTR_CHECKOUT); > > - if (should_update_submodules() && o->update && !o->dry_run) > + if (should_update_submodules()) > load_gitmodules_file(index, NULL);
Good (no behaviour change because this wouldn't have been done under the early-exit condition anyway).
Show 9 quoted lines
>
> for (i = 0; i < index->cache_nr; i++) {
> @@ -388,18 +393,18 @@ static int check_updates(struct unpack_trees_options *o)
>
> if (ce->ce_flags & CE_WT_REMOVE) {
> display_progress(progress, ++cnt);
> - if (o->update && !o->dry_run)
> - unlink_entry(ce);
> + unlink_entry(ce);Good (no behaviour change because this wouldn't have been done under the early-exit condition anyway).
Show 9 quoted lines
> } > } > + > remove_marked_cache_entries(index, 0); > remove_scheduled_dirs(); > > - if (should_update_submodules() && o->update && !o->dry_run) > + if (should_update_submodules()) > load_gitmodules_file(index, &state);
Good (no behaviour change because this wouldn't have been done under the early-exit condition anyway).
>
> enable_delayed_checkout(&state);
> - if (has_promisor_remote() && o->update && !o->dry_run) {
> + if (has_promisor_remote()) {Good (no behaviour change because this wouldn't have been done under the early-exit condition anyway).
Show 11 quoted lines
> /*
> * Prefetch the objects that are to be checked out in the loop
> * below.
> @@ -431,15 +436,12 @@ static int check_updates(struct unpack_trees_options *o)
> ce->name);
> display_progress(progress, ++cnt);
> ce->ce_flags &= ~CE_UPDATE;
> - if (o->update && !o->dry_run) {
> - errs |= checkout_entry(ce, &state, NULL, NULL);
> - }
> + errs |= checkout_entry(ce, &state, NULL, NULL);Good (no behaviour change because this wouldn't have been done under the early-exit condition anyway).
Show 10 quoted lines
> } > } > stop_progress(&progress); > errs |= finish_delayed_checkout(&state, NULL); > - if (o->update) > - git_attr_set_direction(GIT_ATTR_CHECKIN); > + git_attr_set_direction(GIT_ATTR_CHECKIN); > > if (o->clone) > report_collided_checkout(index);
Behaviour around this one (and the corresponding setup) may make a difference before and after the patch to future developers (who may need to revert this change to achieve what they want to do), but I think it is a no-op clean-up for today's code.