Re: [PATCH v8] format-patch: allow a non-integral version numbers

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2021, 05:45 UTC
Message-ID: <xmqqpmztcb6r.fsf@gitster.g>
In-Reply-To: <CAPig+cQBATCe4XFt1k0_EfYvb61_RVgTO0NGy6Ykg7frNPbtpQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 16 quoted lines

> This new example raises the question about what happens if the
> argument to --reroll-count contains characters which don't belong in
> pathnames. For instance, what happens if `--reroll-count=1/2` is
> specified? Most likely, it will fail trying to write the
> "v1/2-whatever.patch" file to a nonexistent directory named "v1".
>
>> diff --git a/log-tree.c b/log-tree.c
>> @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename,
>> +       if (info->reroll_count)
>> +               strbuf_addf(filename, "v%s-", info->reroll_count);
>>         strbuf_addf(filename, "%04d-%s", nr, subject);
>
> To protect against that problem, you may need to call
> format_sanitized_subject() manually after formatting "v%s-". (I'm just
> looking at this code for the first time, so I could be hopelessly
> wrong. There may be a better way to fix it.)

This kind of discovery of what I and others missed is why I love to see reviews on this list ;-)

Yes, slash is of course very problematic, but what we've been doing to the patch filenames was to ensure that they will be free of $IFS whitespaces and shell glob special characters as well, and we should treat the "reroll count" just like the other end-user controlled input, i.e. the title of the patch, and sanitize it the same way.

So I am pretty sure format_sanitized_subject() is the right way to go.