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

From: Clemens Buchacher <drizzd@aon.at>
Date: Oct 13, 2010, 22:34 UTC
Message-ID: <20101013223425.GB2558@localhost>
In-Reply-To: <7v1v7tkb6g.fsf@alter.siamese.dyndns.org>

On Wed, Oct 13, 2010 at 02:57:59PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

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

Show 20 quoted lines

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