On Thu, Sep 5, 2019 at 12:27 PM SZEDER Gábor <szeder.dev@xxxxxxxxx> wrote:
>
> On Thu, Sep 05, 2019 at 08:47:35AM -0700, Elijah Newren wrote:
> > cmd_clean() had the following code structure:
> >
> > struct strbuf abs_path = STRBUF_INIT;
> > for_each_string_list_item(item, &del_list) {
> > strbuf_addstr(&abs_path, prefix);
> > strbuf_addstr(&abs_path, item->string);
> > PROCESS(&abs_path);
> > strbuf_reset(&abs_path);
> > }
> >
> > where I've elided a bunch of unnecessary details and PROCESS(&abs_path)
> > represents a big chunk of code rather than an actual function call. One
> > piece of PROCESS was:
> >
> > if (lstat(abs_path.buf, &st))
> > continue;
> >
> > which would cause the strbuf_reset() to be missed -- meaning that the
> > next path to be handled would have two paths concatenated. This path
> > used to use die_errno() instead of continue prior to commit 396049e5fb62
> > ("git-clean: refactor git-clean into two phases", 2013-06-25), but my
> > understanding of how correct_untracked_entries() works is that it will
> > prevent both dir/ and dir/file from being in the list to clean so this
> > should be dead code and the die_errno() should be safe. But I hesitate
> > to remove it since I am not certain. Instead, just fix it to avoid path
> > corruption in case it is possible to reach this continue statement.
> >
> > Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> > ---
> > builtin/clean.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/clean.c b/builtin/clean.c
> > index 6030842f3a..ccb6e23f0b 100644
> > --- a/builtin/clean.c
> > +++ b/builtin/clean.c
> > @@ -1028,8 +1028,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> > * recursive directory removal, so lstat() here could
> > * fail with ENOENT.
> > */
> > - if (lstat(abs_path.buf, &st))
> > + if (lstat(abs_path.buf, &st)) {
> > + strbuf_reset(&abs_path);
> > continue;
> > + }
>
> I wonder whether it would be safer to call strbuf_reset() at the start
> of each loop iteration instead of before 'continue'. That way we
> wouldn't have to worry about another 'continue' statements forgetting
> about it.
>
> It probably doesn't really matter in this particular case (considering
> that it's potentially dead code to begin with), but have a look at
> e.g. diff.c:show_stats() and its several strbuf_reset(&out) calls
> preceeding continue statements.
Ooh, I like that idea. I think I'll apply that here. I'll probably
leave diff.c:show_stats() as #leftoverbits for someone else, though I
really like the idea of fixing up other issues like this as you
suggest.
