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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2007, 08:59 UTC
Message-ID: <7vvebz7hpw.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0708010129530.14781@racer.site>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 19 quoted lines

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