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

On Fri, Mar 24, 2017 at 2:02 PM, Stefan Beller [off-list ref] 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.

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.

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.

quoted

        }
-       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.