[PATCH 0/1] unpack-trees: exit check_updates() early if updates are not wanted

[PATCH 0/1] unpack-trees: exit check_updates() early if updates are not wanted

[Elijah Newren via GitGitGadget]

In trying to understand check_updates(), I found I was able to simplify the
function by making it exit early when updates are not wanted.

Elijah Newren (1):
  unpack-trees: exit check_updates() early if updates are not wanted

 unpack-trees.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)


base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-686%2Fnewren%2Fsimplify-check-updates-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-686/newren/simplify-check-updates-v1
Pull-Request: https://github.com/git/git/pull/686
-- 
gitgitgadget

[PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

check_updates() has a lot of code that repeatedly checks whether
o->update or o->dry_run are set.  (Note that o->dry_run is a
near-synonym for !o->update, but not quite as per commit 2c9078d05bf2
("unpack-trees: add the dry_run flag to unpack_trees_options",
2011-05-25).)  In fact, this function almost turns into a no-op whenever
the condition
   !o->update || o->dry_run
is met.  Simplify the code by checking this condition at the beginning
of the function, and when it is true, do the few things that are
relevant and return early.

There are a few things that make the conversion not quite obvious:
  * The fact that check_updates() does not actually turn into a no-op
    when updates are not wanted may be slightly surprising.  However,
    commit 33ecf7eb61 (Discard "deleted" cache entries after using them
    to update the working tree, 2008-02-07) put the discarding of
    unused cache entries in check_updates() so we still need to keep
    the call to remove_marked_cache_entries().  It's possible this
    call belongs in another function, but it is certainly needed as
    tests will fail if it is removed.
  * The original called remove_scheduled_dirs() unconditionally.
    Technically, commit 7847892716 (unlink_entry(): introduce
    schedule_dir_for_removal(), 2009-02-09) should have made that call
    conditional, but it didn't matter in practice because
    remove_scheduled_dirs() becomes a no-op when all the calls to
    unlink_entry() are skipped.  As such, we do not need to call it.
  * When (o->dry_run && o->update), the original would have two calls
    to git_attr_set_direction() surrounding a bunch of skipped updates.
    These two calls to git_attr_set_direction() cancel each other out
    and thus can be omitted when o->dry_run is true just as they
    already are when !o->update.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 unpack-trees.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 2399b6818b..4c68dbdb43 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -372,15 +372,20 @@ static int check_updates(struct unpack_trees_options *o)
 	state.refresh_cache = 1;
 	state.istate = index;
 
+	if (!o->update || o->dry_run) {
+		remove_marked_cache_entries(index, 0);
+		trace_performance_leave("check_updates");
+		return 0;
+	}
+
 	if (o->clone)
 		setup_collided_checkout_detection(&state, index);
 
 	progress = get_progress(o);
 
-	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);
 
 	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);
 		}
 	}
+
 	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);
 
 	enable_delayed_checkout(&state);
-	if (has_promisor_remote() && o->update && !o->dry_run) {
+	if (has_promisor_remote()) {
 		/*
 		 * 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);
 		}
 	}
 	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);
-- 
gitgitgadget

Re: [PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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.

Re: [PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted

[Elijah Newren]

Hi Junio,

On Sat, Jan 4, 2020 at 3:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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?

setup_collided_checkout_detection() clears the CE_MATCHED flag for
each cache entry, which can be updated via way of checkout_entry()'s
call to mark_colliding_entries().  Then the trailing
report_collided_checkout() at the end of the function will report any
paths with CE_MATCHED set.  However, when we're in a dry-run, all
checkout_entry() calls are skipped, so we won't detect ever detect any
collided entries.  As such, there's no point in calling either the
setup_collided_checkout_detection() or report_collided_checkout()
funtions.

Sorry for forgetting to include this in the commit message.

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

The point of the progress display was to show how long it takes to do
the updates.  For a dry-run, that time is zero because we skip the
updates.  And if we're skipping the updates, shouldn't we also skip
measuring how long it takes to do them?

Yeah, I should have included this in the commit message too.

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

I hadn't checked previously, but that looks right to me.

> 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 it is intended that a report of colliding paths be given when doing
a dry-run using unpack_trees, then the current code is quite buggy
because it'll never report anything.  In order to report anything, the
mechanism for detecting them would need to be drastically reworked.
As such, I'd view the optimization as at least making it more obvious
why the code won't report collisions with dry-run rather than
pretending it will.  Or are you saying that I've just uncovered a bug
and it should really be fixed rather than exploited for an
optimization?

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

[PATCH v2 0/1] unpack-trees: exit check_updates() early if updates are not wanted

[Elijah Newren via GitGitGadget]

In trying to understand check_updates(), I found I was able to simplify the
function by making it exit early when updates are not wanted.

Changes since v1:

 * Added two bullet points to the end of the commit message to explain the
   questions Junio brought up about function calls that are skipped by the
   early return.

Elijah Newren (1):
  unpack-trees: exit check_updates() early if updates are not wanted

 unpack-trees.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)


base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-686%2Fnewren%2Fsimplify-check-updates-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-686/newren/simplify-check-updates-v2
Pull-Request: https://github.com/git/git/pull/686

Range-diff vs v1:

 1:  de0f381284 ! 1:  dd27727332 unpack-trees: exit check_updates() early if updates are not wanted
     @@ -33,6 +33,21 @@
              These two calls to git_attr_set_direction() cancel each other out
              and thus can be omitted when o->dry_run is true just as they
              already are when !o->update.
     +      * The code would previously call setup_collided_checkout_detection()
     +        and report_collided_checkout() even when o->dry_run.  However, this
     +        was just an expensive no-op because
     +        setup_collided_checkout_detection() merely cleared the CE_MATCHED
     +        flag for each cache entry, and report_collided_checkout() reported
     +        which ones had it set.  Since a dry-run would skip all the
     +        checkout_entry() calls, CE_MATCHED would never get set and thus
     +        no collisions would be reported.  Since we can't detect the
     +        collisions anyway without doing updates, skipping the collisions
     +        detection setup and reporting is an optimization.
     +      * The code previously would call get_progress() and
     +        display_progress() even when (!o->update || o->dry_run).  This
     +        served to show how long it took to skip all the updates, which is
     +        somewhat useless.  Since we are skipping the updates, we can skip
     +        showing how long it takes to skip them.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      

-- 
gitgitgadget

[PATCH v2 1/1] unpack-trees: exit check_updates() early if updates are not wanted

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

check_updates() has a lot of code that repeatedly checks whether
o->update or o->dry_run are set.  (Note that o->dry_run is a
near-synonym for !o->update, but not quite as per commit 2c9078d05bf2
("unpack-trees: add the dry_run flag to unpack_trees_options",
2011-05-25).)  In fact, this function almost turns into a no-op whenever
the condition
   !o->update || o->dry_run
is met.  Simplify the code by checking this condition at the beginning
of the function, and when it is true, do the few things that are
relevant and return early.

There are a few things that make the conversion not quite obvious:
  * The fact that check_updates() does not actually turn into a no-op
    when updates are not wanted may be slightly surprising.  However,
    commit 33ecf7eb61 (Discard "deleted" cache entries after using them
    to update the working tree, 2008-02-07) put the discarding of
    unused cache entries in check_updates() so we still need to keep
    the call to remove_marked_cache_entries().  It's possible this
    call belongs in another function, but it is certainly needed as
    tests will fail if it is removed.
  * The original called remove_scheduled_dirs() unconditionally.
    Technically, commit 7847892716 (unlink_entry(): introduce
    schedule_dir_for_removal(), 2009-02-09) should have made that call
    conditional, but it didn't matter in practice because
    remove_scheduled_dirs() becomes a no-op when all the calls to
    unlink_entry() are skipped.  As such, we do not need to call it.
  * When (o->dry_run && o->update), the original would have two calls
    to git_attr_set_direction() surrounding a bunch of skipped updates.
    These two calls to git_attr_set_direction() cancel each other out
    and thus can be omitted when o->dry_run is true just as they
    already are when !o->update.
  * The code would previously call setup_collided_checkout_detection()
    and report_collided_checkout() even when o->dry_run.  However, this
    was just an expensive no-op because
    setup_collided_checkout_detection() merely cleared the CE_MATCHED
    flag for each cache entry, and report_collided_checkout() reported
    which ones had it set.  Since a dry-run would skip all the
    checkout_entry() calls, CE_MATCHED would never get set and thus
    no collisions would be reported.  Since we can't detect the
    collisions anyway without doing updates, skipping the collisions
    detection setup and reporting is an optimization.
  * The code previously would call get_progress() and
    display_progress() even when (!o->update || o->dry_run).  This
    served to show how long it took to skip all the updates, which is
    somewhat useless.  Since we are skipping the updates, we can skip
    showing how long it takes to skip them.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 unpack-trees.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 2399b6818b..4c68dbdb43 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -372,15 +372,20 @@ static int check_updates(struct unpack_trees_options *o)
 	state.refresh_cache = 1;
 	state.istate = index;
 
+	if (!o->update || o->dry_run) {
+		remove_marked_cache_entries(index, 0);
+		trace_performance_leave("check_updates");
+		return 0;
+	}
+
 	if (o->clone)
 		setup_collided_checkout_detection(&state, index);
 
 	progress = get_progress(o);
 
-	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);
 
 	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);
 		}
 	}
+
 	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);
 
 	enable_delayed_checkout(&state);
-	if (has_promisor_remote() && o->update && !o->dry_run) {
+	if (has_promisor_remote()) {
 		/*
 		 * 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);
 		}
 	}
 	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);
-- 
gitgitgadget

Re: [PATCH v2 1/1] unpack-trees: exit check_updates() early if updates are not wanted

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> check_updates() has a lot of code that repeatedly checks whether
> o->update or o->dry_run are set.  (Note that o->dry_run is a
> near-synonym for !o->update, but not quite as per commit 2c9078d05bf2
> ("unpack-trees: add the dry_run flag to unpack_trees_options",
> 2011-05-25).)  In fact, this function almost turns into a no-op whenever
> the condition
>    !o->update || o->dry_run
> is met.  Simplify the code by checking this condition at the beginning
> of the function, and when it is true, do the few things that are
> relevant and return early.

Thanks; will queue.