Re: [PATCH] [GSoC] remove_subtree(): reimplement using iterators

From: Daniel Ferreira (theiostream) <bnmvco@gmail.com>
Date: Mar 25, 2017, 01:02 UTC
Message-ID: <CAEA2_RJ6OjLjS5yGjvd+S0iH+Zv0Zs6StGMv3_9ON6hVzD-85A@mail.gmail.com>
In-Reply-To: <CAEA2_RLZztaRwcppwS45XfXO1n_VKw5547uScOhQON=ktttW8g@mail.gmail.com>

> On Fri, Mar 24, 2017 at 2:02 PM, Stefan Beller <sbeller@google.com> wrote:

> Welcome to the Git community!

Thank you!

> Please use a more imperative style. (e.g. s/Uses/Use/ ...
> s/and simplfying/which simplifies/)

Thank you. Will do in a second version of this patch.

Show 5 quoted lines

> Thanks for this link. It gives good context for reviewing the change,
> but it will not be good context to record as a commit message.
> (When someone looks at a commit message later on, they are usually trying
> to figure out what the author was thinking; if there were any special cases to
> be thought about. Was performance on the authors mind? etc)

> So I propose to put the link into the more informal section if a
> reroll is needed.

Perfect. I will remove it from the message.

Show 7 quoted lines

> Instead of constructing the path again here based on relative path
> and the path parameter, I wonder if we could use
>
>     if (unlink(diter->path))
>         ..
>
> here? Then we would not need the strbuf at all?

Yes, we can! Thank you for the pointer. Will be in the next version of the patch.

> Also we'd need to handle (empty) directories differently for removal?

From what I've tested, we do not need to do it.

> Do we need to check the return code of dir_iterator_advance
> for ITER_ERROR as well?

I believe not – it only tries to perform an operation if we have ITER_OK. Since ITER_ERROR would end up in a no-op anyway I don't see how a check for it would be useful.

Show 11 quoted lines

>
>
> >         }
> > -       closedir(dir);
> > +
> >         if (rmdir(path->buf))
> >                 die_errno("cannot rmdir '%s'", path->buf);
>
> This would remove the "top level" directory as given by path.
> When reading the dir-iterator code, I am not sure if this is
> also part of the yield in dir_iterator_advance.

I've tested it, and it does not yield in there.

Thank you for the advice, and as stated, will submit a v2 of the patch in short notice.

Thank you, Daniel.