"Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> + 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?
> - 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).
>
> 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).
> }
> }
> +
> 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).
> /*
> * 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).
> }
> }
> 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.
