On Fri, Sep 30 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >>> The end result is OK in that the configuration variable does not >>> exist in the resulting repository, but we should do better, by using >>> git_config_set_gently() and ignoring only the specific error that is >>> returned when removing a missing configuration variable. >> ... >> I was curious to follow up on your suggestion in your 3rd paragraph, so >> I tried implementing this in the config API, results below, if you're >> interested in it assume my SOB. > > Did I make any suggestion? I am assuming that what I left in the > quote above is the paragraph you are referring to, and that is not a > suggestion but a description of what the patch did, so I am puzzled. I think I just misread "by using git_config_set_gently() and ignoring only the specific error that is returned when removing a missing configuration variable." as "an alternative of this might do this via the config API"... >> But, having done that I discovered that your code here has a bug, >> admittedly a pretty obscure one. The CONFIG_NOTHING_SET flag on "set" >> doesn't mean "nothing to set, because it's there already", it means >> "either <that>, or the key is multi-value and I'm bailing out". > > Ah, OK, so in short, _gently() is still unusable to use for that. I > guess it means that the approach taken by v1 would be a better > solution, then. As you noted it's got a TOCTOU instead, so we might wipe away good config entirely. I think between the two I'd go with v2, bugs and all, it's pretty unlikely that someone has two "description" entries.
