Re: [PATCH v3] git-clean: Display more accurate delete messages

From: Zoltan Klinger <zoltan.klinger@gmail.com>
Date: Jan 3, 2013, 23:21 UTC
Message-ID: <CAKJhZwS6VUwWoX1QmNL19asNt1B3dPsDeg5-JTzq8FMd1WYkSw@mail.gmail.com>
In-Reply-To: <7vfw2j2vlp.fsf@alter.siamese.dyndns.org>

Show 8 quoted lines

> The updated code structure is much nicer than the previous round,
> but I am somewhat puzzled how return value of remove_dirs() and
> &gone relate to each other.  Surely when gone is set to zero,
> remove_dirs() is reporting that the directory it was asked to remove
> recursively did not go away, so it must report failure, no?  Having
> the &gone flag looks redundant and checking for gone in some places
> while checking for the return value for others feels like an
> invitation for future bugs.

The return value of remove_dirs() has an overall effect on the exit
code of git-clean, and &gone indicates whether the directory we asked
remove_dirs() to delete was actually removed. If all goes well  in
remove_dirs() the return code is 0 and gone flag is 1. If file or
subdirectory delete fails return code is 1 and the gone flag is set to
0. The special case is when remove_dirs() is asked to remove an
untracked git repo that should be ignored. In this case remove_dirs()
is not going to remove the directory so the gone flag is set to zero
but it is not an error so the return value will be set to zero too.

Show 5 quoted lines

> Also the remove_dirs() function seems to replace the use of
> remove_dir_recurse() from dir.c by copying large part of it, with
> error message sprinkled.  Does remove_dir_recurse() still get used
> by other codepaths?  If so, do the remaining callsites benefit from
> using this updated version?

In dir.c the remove_dir_recurse() is a private function that is called by the public remove_dir_recursively() wrapper function. The remove_dir_recursively() function is called from the following places:

    builtin/clone.c:387:
    builtin/clone.c:392:
    builtin/rm.c:349:
    notes-merge.c:771:
    refs.c:1527:
    sequencer.c:27:
    transport.c:247:
    transport.c:393:

The messages that remove_dirs() prints out are very specific to git-clean and they are not really relevant in the above places where remove_dir_recursively() is called from. Also, the remove logic for files is slightly different in remove_dirs() when it comes to handling a failed file delete. While remove_dirs() continues removing other files in the same directory upon failure, remove_dir_recurse() will stop at the first error. So perhaps having the remove_dirs() in builtin/clean.c is OK.