Re: [PATCH v3 0/3] Use default values from settings instead of config

From: Glen Choo <chooglen@google.com>
Date: Oct 5, 2021, 22:25 UTC
Message-ID: <kl6lh7dvw1lp.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <87lf37ll4k.fsf@evledraar.gmail.com>

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

Show 5 quoted lines

> I've looked this over carefully, it LGTM. I noticed some bugs in this
> area, but they pre-date your commits. I've pushed a
> "gc/use-repo-settings" to https://github.com/avar/git.git that you can
> check out, consider that branch partially as commentary, but feel free
> to pick/squash anything you find there.

Your review is very thoughtful, thank you :)

Show 9 quoted lines

>  * This is somewhat of a case of throwing rocks from a glass house, but
>    I think the commit messages could really be shorter/get to the
>    point. I.e. say right away what's being changed and why, instead of
>    giving the reader an overview of the whole control flow.
>
>    I just adjusted adusted 1/3 in that direction below, I think 2/3
>    could do with being squashed into it & adjusted, it's really the
>    exact same bug/improvement, just with a different config key that
>    behaves the same.

In hindsight, I think this is how I should have initially structured my commits because this improves the signal-noise ratio. I'll keep it in mind in the future for sure. I'm hesitant to send a v4, but I can be persuaded to do so :)

>  * The "test_unconfig" in your 2/3 is redundant, so is the "rm -rf" of
>    directories that can't exist by that point in 3/3.

These are redundant, but they serve a defensive purpose. By being extra defensive, I was hoping to make it clear to the reader that the test is *guaranteed* not to conflict with/rely on previous state and reduce the number of greps needed.

Show 6 quoted lines

>  * You use corrupting the graph as a shorthand for "did we run this
>    command?". See test_region for an approach that might be better,
>    i.e. just log with trace2 and grep that to see if we started the
>    relevant command. The test_region helper can't be used as-is for that
>    (you could manually grep the emitted PERF/EVENT output), but perhaps
>    that's easier/more reliable?

My intention here was not to assert "did git fsck call the function I expected?", but something more along the lines of "did git fsck catch the corrupted commit-graph?". Calling the function seems like an implementation detail, but the actual desired behavior is that git fsck catches the breakage.

Perhaps the test title should be reflect what I meant, like

  - git fsck (checks commit-graph when config set to true)
  + git fsck detects corrupted commit-graph

>  * This is again, something that pre-dates your patches, but I think
>    following the "is enabled?" behavior here for these variables in "git
>    fsck" is highly questionable. So just some thoughts after having
>    looked at that:

I think it depends on how we think about core.commitGraph (and others). Do we want Git to be able to pretend that commit-graph doesn't exist at all? If so, then we should skip the check in git fsck. Alternatively, is commit-graph actually part of Git's core offering? If so, core.commitGraph is more like a way to opt *out* of some commit-graph behavior, but fsck should still verify our core data structures. Given that we default to true, it sounds like more of the latter, but I'm not sure what use case this serves.

>    Just because your config doen't say "use the graph" doesn't mean
>    another process won't do so, and if the graph is broken they might
>    probably encounter an error.

While that's true, I think that the user assumes a certain degree of risk if they are using different commands with different config values. I don't think it's unreasonable to say that "git -c core.commitGraph=false fsck" won't catch issues that will arise in "git -c core.commitGraph=true foo".

Show 8 quoted lines

>    But in general for these auxiliary files (commit-graph, midix, *.rev
>    etc.) I think there's a good argument to be made that fsck should
>    *always* check them, and at most use the config to say something
>    like:
>
>        Hey, you've got a 100% broken commit-graph/midx/rev in your .git,
>        looks like the core.XYZ enabling it is off *right now* though, so
>        maybe we won't use it.  I hope you're feeling lucky...

I think "always check" sounds like the best way to maximize visibility for users, though I don't think core.commitGraph should control the severity of the warning/error. If we pursue "always check", I think we could adopt fsck.<msg-id> instead.