Re: [PATCH 4/4] Clean up work-tree handling

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Wed, 1 Aug 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/builtin-ls-files.c b/builtin-ls-files.c
> > index 61577ea..d36181a 100644
> > --- a/builtin-ls-files.c
> > +++ b/builtin-ls-files.c
> > @@ -469,9 +469,11 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
> >  		break;
> >  	}
> >  
> > -	if (require_work_tree &&
> > -			(!is_inside_work_tree() || is_inside_git_dir()))
> > -		die("This operation must be run in a work tree");
> > +	if (require_work_tree && !is_inside_work_tree()) {
> > +		const char *work_tree = get_git_work_tree();
> > +		if (!work_tree || chdir(work_tree))
> > +			die("This operation must be run in a work tree");
> > +	}
> >  
> >  	pathspec = get_pathspec(prefix, argv + i);
> >  
> 
> Similarly to this change, I am wondering if we would want to fix
> verify_non_filename() in setup.c, which does this:
> 
> /*
>  * Verify a filename that we got as an argument for a pathspec
>  * entry. Note that a filename that begins with "-" never verifies
>  * as true, because even if such a filename were to exist, we want
>  * it to be preceded by the "--" marker (or we want the user to
>  * use a format like "./-filename")
>  */
> void verify_filename(const char *prefix, const char *arg)
> {
> ...
> }
> 
> /*
>  * Opposite of the above: the command line did not have -- marker
>  * and we parsed the arg as a refname.  It should not be interpretable
>  * as a filename.
>  */
> void verify_non_filename(const char *prefix, const char *arg)
> {
>         const char *name;
>         struct stat st;
> 
>         if (!is_inside_work_tree() || is_inside_git_dir())
>                 return;
>         if (*arg == '-')
>                 return; /* flag */
>         name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
>         if (!lstat(name, &st))
>                 die("ambiguous argument '%s': both revision and filename\n"
>                     "Use '--' to separate filenames from revisions", arg);
>         if (errno != ENOENT)
>                 die("'%s': %s", arg, strerror(errno));
> }
> 
> At this point, we are given an ambiguous parameter, that could
> be naming a path in the work tree.  If we are not in the work
> tree, then it is understandable that we do not have to barf.
> The other check (i.e. "|| is_inside_git_dir()") does not hurt
> (iow, it is not an incorrect check per-se), because if you did
> "cd .git && git log HEAD" then the HEAD parameter cannot be
> naming the path ".git/HEAD" in the work tree, but (1) that is
> already covered by .git/ being "outside of work tree", and (2)
> it is not something this function wants to check anyway
> (i.e. "can the parameter be naming a file in the work tree?").
> 
> Am I mistaken and/or confused?

I think you are completely right.  Inside a bare repository, "git log 
FETCH_HEAD" should not need to complain.  And I think that the 
"is_inside_git_dir()" could be _replaced_ by "!is_inside_work_tree()", 
since that is the intent of that call.

Ciao,
Dscho