From: Phillip Wood Date: Fri, 18 Jul 2025 14:40:30 GMT Subject: Re: [PATCH] config: remove unneeded struct field Message-ID: <2ff3a900-daea-4264-8509-70464952f74f@gmail.com> In-Reply-To: On 17/07/2025 18:31, Junio C Hamano wrote: > Phillip Wood writes: > >> I stumbled across this use-after-free while working on the deprecation >> of core.commentChar=auto. > > And that other topic also adds more uses of "is kvi->path set?", so > we'd need a bit of semantic conflict resolution, which was fun ;-) Oh sorry I'd updated it locally and forgotten that you'd need to do the same. > FYI, here is what I'd be using as merge-fix, a change that would be > squashed in when the named branch is merged in. > > I expect that this part would change a lot anyway, to make it fail > when "auto" is used under WITH_BREAKING_CHANGES, so the conflict > resolution for the current/previous round may not matter all that > much. This part of the code gets moved but doesn't really change. Your resolution looks good. I'm mostly there with the re-roll but it will be next week before it is ready to post to the. Thanks Phillip > > --- >8 --- > Subject: [PATCH] merge-fix/pw/3.0-commentchar-auto-deprecation > > Conflicts with pw/config-kvi-remove-path that removes kvi->path > field. Checking if kvi->path is set should be done by inspecting > kvi->origin_type and seeing if it is CONFIG_ORIGIN_FILE instead, and > otherwise kvi->filename is usable when it is CONFIG_ORIGIN_FILE. > --- > builtin/commit.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index a513709a51..04440fbd3f 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -771,10 +771,10 @@ static int comment_char_config_cb(const char *key, const char *value, > return 0; > > cfg->last_key_id = key_id; > - if (!kvi->path) { > + if (kvi->origin_type != CONFIG_ORIGIN_FILE) { > return 0; > - } else if (get_comment_key_flags(cfg, kvi->path, key_id)) { > - set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_TWICE); > + } else if (get_comment_key_flags(cfg, kvi->filename, key_id)) { > + set_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_TWICE); > } else { > struct comment_char_cfg_item *item; > > @@ -782,8 +782,8 @@ static int comment_char_config_cb(const char *key, const char *value, > item = &cfg->item[cfg->nr - 1]; > item->key_id = key_id; > item->scope = kvi->scope; > - item->path = xstrdup(kvi->path); > - set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_ONCE); > + item->path = xstrdup(kvi->filename); > + set_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_ONCE); > } > cfg->auto_set_in_file = value && !strcmp(value, "auto"); >