Re: [PATCH v2 7/8] config: report cached filenames in die_bad_number()

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 16, 2023, 22:22 UTC
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:

Show 5 quoted lines

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