> 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.
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html