From: Jonathan Tan <jonathantanmy@google.com>
Date: Thu, 16 Mar 2023 22:22:00 GMT
Subject: Re: [PATCH v2 7/8] config: report cached filenames in die_bad_number()
Message-ID: <20230316222200.320596-1-jonathantanmy@google.com>
In-Reply-To: <3c83d9535a037653c7de2d462a4df3a3c43a9308.1678925506.git.gitgitgadget@gmail.com>

Ah, thanks for spotting this bug! It is a minor one, but this now makes
me think that we should definitely do this refactoring of a struct
containing all the relevant config state and passing it to functions as
much as possible (as opposed to merely leaning towards the idea).

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Fix this by refactoring the current_config_* functions into variants
> that don't BUG() when we aren't reading config, and using the resulting
> functions in die_bad_number(). Refactoring is needed because "git config
> --get[-regexp] --type=int" parses the int value _after_ parsing the
> config file, which will run into the BUG().

You say "fix this", but are there actually 2 bugs (so, "fix these")?
Firstly, that BUG() is run into when invoking "git config" the way
you describe, and secondly, die_bad_number() only reading cf and not
checking kvi to see if anything's there. (I'm not sure how to reproduce
the latter, though.)

> Also, plumb "struct config_reader" into the new functions. This isn't
> necessary per se, but this generalizes better, so it might help us avoid
> yet another refactor.

Hmm...I thought this would be desired because we don't want the_reader
to be used from non-public functions anyway, so we can just state
that that is the reason (and not worry about using future refactors as
a justification).

The code itself looks good.