On Wed, Oct 13, 2010 at 02:57:59PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@xxxxxx> writes:
>
> > If the work tree contains an untracked file x, and
> > unpack-trees wants to checkout a path x/*, the
> > file x is removed unconditionally.
> >
> > Instead, apply the same checks that are normally
> > used for untracked files, and abort if the file
> > cannot be removed.
> >
>
> Too short a line is also hard to read as too long a line.
Ok.
> > diff --git a/cache.h b/cache.h
> > index 2ef2fa3..f65d6f9 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -852,7 +852,7 @@ struct cache_def {
> >
> > extern int has_symlink_leading_path(const char *name, int len);
> > extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
> > -extern int has_symlink_or_noent_leading_path(const char *name, int len);
> > +extern int check_leading_path(const char *name, int len);
>
> This is an API regression. "check" in function name "check-blah" does not
> tell what you are checking (is it checking if the path is Ok? is it
> checking if the path has symlinked components? is it checking if some
> part of the path overlaps with tracked content? is it checking if you
> have already run lstat(2) on some of the leading path components?), nor
> for what purpose the check can be used (it is Ok to overwrite it?, is it
> Ok to create a directory there? is it Ok to create a file there?).
>
> At least the old name told us what it checked, didn't it?
Yes, but that's not what it does any more. I worded it vague on
purpose, because there is really no way to explain all it does in
one function name. This is the full description.
/*
* Return zero if path 'name' has a leading symlink component or
* if some leading path component does not exists.
*
* Return -1 if leading path exists and is a directory.
*
* Return path length if leading path exists and is neither a
* directory nor a symlink.
*/
But if you prefer, we can call it "verify_clean_leading_path"?
> The approach you took looks sound, though. What did you feel was "not
> exactly pretty" about it?
First, check_leading_path bothers me. It does too many things and
encodes all of that in one integer return value.
Second, I also have to copy the offending path manually to a local
path variable, even though lstat_cache already has that path stored
in a buffer.
Third, check_ok_to_remove expects a cache_entry. But in case of a
match in check_leading_path, we do not have one and pass NULL to
check_ok_to_rmove. It will not be used anyways, because
check_leading_path should return > 0 only for regular files, and
the cache_entry is only used for directories in
verify_clean_subdirectory.
If lstat returns with an error other than NOENT, or if
check_ok_to_remove is called with anything other than a directory
and cache_entry is NULL, we get a segmentation fault. Before, an
error was simply ignored. I don't know which is worse.
On the other hand, I think the patch fixes a nasty bug, so maybe we
can accept those issues for now.
Clemens
Attachment:
signature.asc
Description: Digital signature