Re: [PATCH 5/5] do not overwrite files in leading path

[Clemens Buchacher <drizzd@aon.at>]

[-- Attachment #1: Type: text/plain, Size: 3140 bytes --]

On Wed, Oct 13, 2010 at 02:57:59PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> 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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]