Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
>> 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.)
There is actually only one bug (the latter). That is tested by the new
test I added in this patch. To reproduce it, we need:
- To iterate a config_set (git_config() or repo_config() will suffice),
in which case the config_kvi is set, but not cf.
- Then in the config_fn_t we pass to it, we call git_parse_int() on an
invalid number, which will result in die_bad_number(), which prints
the less specific message.
The former case isn't a bug. We never ran into the BUG() when invoking
"git config" because die_bad_number() doesn't use current_* prior to
this patch (which is where the BUG() is). t1300:'invalid unit'
demonstrates that we print the correct message (and that we don't
BUG()):
test_expect_success 'invalid unit' '
git config aninvalid.unit "1auto" &&
test_cmp_config 1auto aninvalid.unit &&
test_must_fail git config --int --get aninvalid.unit 2>actual &&
test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
'
(which is a good signal that I should probably reword the commit
message)
>
>> 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).
Ah, good point, thanks.
