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

From: Glen Choo <chooglen@google.com>
Date: Mar 16, 2023, 23:05 UTC
Message-ID: <kl6lmt4cuwj4.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230316222200.320596-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 9 quoted lines

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

Show 9 quoted lines

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