[PATCH] gc: remove redundant check for gc_auto_threshold

[PATCH] gc: remove redundant check for gc_auto_threshold

[Ævar Arnfjörð Bjarmason]

Checking gc_auto_threshold in too_many_loose_objects() was added in
17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.",
2007-09-17) when need_to_gc() itself was also reliant on
gc_auto_pack_limit before its early return:

    gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0

When that check was simplified to just checking "gc_auto_threshold <=
0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by
assigning 0 to gc.auto", 2008-03-19) this unreachable code should have
been removed. We only call too_many_loose_objects() from within
need_to_gc() itself, which will return if this condition holds, and in
cmd_gc() which will return before ever getting to "auto_gc &&
too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true
earlier in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I had this in my tree as part of some general gc cleanups I was
working on, but since it's trivially considered as a stand-alone topic
and unlikely to conflict with anything I or anyone else has planned
I'm sending it as a one-off.

 builtin/gc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 2b592260e9..5f25a35dfc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -157,9 +157,6 @@ static int too_many_loose_objects(void)
 	int num_loose = 0;
 	int needed = 0;
 
-	if (gc_auto_threshold <= 0)
-		return 0;
-
 	dir = opendir(git_path("objects/17"));
 	if (!dir)
 		return 0;
-- 
2.19.1.390.gf3a00b506f

Re: [PATCH] gc: remove redundant check for gc_auto_threshold

[Brandon Casey]

On Wed, Oct 10, 2018 at 12:32 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Checking gc_auto_threshold in too_many_loose_objects() was added in
> 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.",
> 2007-09-17) when need_to_gc() itself was also reliant on
> gc_auto_pack_limit before its early return:
>
>     gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0
>
> When that check was simplified to just checking "gc_auto_threshold <=
> 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by
> assigning 0 to gc.auto", 2008-03-19) this unreachable code should have
> been removed. We only call too_many_loose_objects() from within
> need_to_gc() itself, which will return if this condition holds, and in
> cmd_gc() which will return before ever getting to "auto_gc &&
> too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true
> earlier in the function.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> I had this in my tree as part of some general gc cleanups I was
> working on, but since it's trivially considered as a stand-alone topic
> and unlikely to conflict with anything I or anyone else has planned
> I'm sending it as a one-off.

Hmm, yeah you're right that the check seems to be redundant for the
current uses of too_many_loose_objects().  I don't feel strongly about
it, but I think an argument could be made that it makes sense for
too_many_loose_object() and too_many_packs() to each inspect the
configuration variable that controls them and detect when they're
disabled, rather than having one of them require that the check be
done beforehand.  Again, I don't feel strongly about it, but I'm not
sure this change actually improves the code.

-Brandon

Re: [PATCH] gc: remove redundant check for gc_auto_threshold

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> ...  Again, I don't feel strongly about it, but I'm not
> sure this change actually improves the code.

Yeah, in the context of the current caller, this is a safe change
that does not break anybody and reduces the number of instructions
executed in this codepath.  A mistaken caller may be added in the
future that fails to check auto-threashold beforehand, but that
won't lead to anything bad like looping for a large number of times,
so as long as the API contract into this helper function is clear
that callers are responsible to check beforehand, it is still not
too bad.

So, I'd throw this into "Meh - I won't regret applying it, but it is
not the end of the world if I forget to apply it, either" pile.

I _think_ a change that actually improves the code would be to
restructure so that there is a helper that is responsible for
guestimating the number of loose objects, and another that uses the
helper to see if there are too many loose objects.  The latter is
the only one tha needs to know about auto-threashold.  But we are
not in immdiate need for such a clean-up, I guess, unless somebody
is actively looking into revamping how auto-gc works and doing a
preparatory clean-up.

Re: [PATCH] gc: remove redundant check for gc_auto_threshold

[Brandon Casey]

On Wed, Oct 10, 2018 at 4:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Brandon Casey <drafnel@gmail.com> writes:
>
> > ...  Again, I don't feel strongly about it, but I'm not
> > sure this change actually improves the code.
>
> Yeah, in the context of the current caller, this is a safe change
> that does not break anybody and reduces the number of instructions
> executed in this codepath.  A mistaken caller may be added in the
> future that fails to check auto-threashold beforehand, but that
> won't lead to anything bad like looping for a large number of times,
> so as long as the API contract into this helper function is clear
> that callers are responsible to check beforehand, it is still not
> too bad.
>
> So, I'd throw this into "Meh - I won't regret applying it, but it is
> not the end of the world if I forget to apply it, either" pile.
>
> I _think_ a change that actually improves the code would be to
> restructure so that there is a helper that is responsible for
> guestimating the number of loose objects, and another that uses the
> helper to see if there are too many loose objects.  The latter is
> the only one tha needs to know about auto-threashold.  But we are
> not in immdiate need for such a clean-up, I guess, unless somebody
> is actively looking into revamping how auto-gc works and doing a
> preparatory clean-up.

Agreed on all points, and as usual, said better than I could :-)

-Brandon

Re: [PATCH] gc: remove redundant check for gc_auto_threshold

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 10 2018, Junio C Hamano wrote:

> Brandon Casey <drafnel@gmail.com> writes:
>
>> ...  Again, I don't feel strongly about it, but I'm not
>> sure this change actually improves the code.
>
> Yeah, in the context of the current caller, this is a safe change
> that does not break anybody and reduces the number of instructions
> executed in this codepath.  A mistaken caller may be added in the
> future that fails to check auto-threashold beforehand, but that
> won't lead to anything bad like looping for a large number of times,
> so as long as the API contract into this helper function is clear
> that callers are responsible to check beforehand, it is still not
> too bad.
>
> So, I'd throw this into "Meh - I won't regret applying it, but it is
> not the end of the world if I forget to apply it, either" pile.
>
> I _think_ a change that actually improves the code would be to
> restructure so that there is a helper that is responsible for
> guestimating the number of loose objects, and another that uses the
> helper to see if there are too many loose objects.  The latter is
> the only one tha needs to know about auto-threashold.  But we are
> not in immdiate need for such a clean-up, I guess, unless somebody
> is actively looking into revamping how auto-gc works and doing a
> preparatory clean-up.

Yeah, that's me :) I have some WIP gc cleanup, but want to sit on it a
bit before I submit it to think about the best way to do things.

So in the meantime I was sending out a few WIP bits that I expected
could be reviewed stand-alone.

So I'd prefer to have this applied. It's easy enough to understand that
shouldn't take long to prove to be correct & trickle down to "master",
and will make those subsequent patches easier to follow.

Re: [PATCH] gc: remove redundant check for gc_auto_threshold

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Yeah, that's me :) I have some WIP gc cleanup, but want to sit on it a
> bit before I submit it to think about the best way to do things.
>
> So in the meantime I was sending out a few WIP bits that I expected
> could be reviewed stand-alone.

I dunno.  Unless the real body of the changes that "depend" on this
small change comes before people forget the connection between them,
I think it is detrimental to churn the codebase like this.  If the
real body of the changes do not conflict with other topics in flight
when it materializes, then having this small clean-up as a
preparatory step in that real series would cost us nothing---that
clean-up would not conflict with other things either.  If the real
thing would conflict and need to be adjusted to play well with other
topics before submission, having this small clean-up as a
preparatory step in that real series would cost us nothing, either.