Re: [PATCH] Make Git accept absolute path names for files within the work tree

From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Nov 29, 2007, 01:15 UTC
Message-ID: <200711290215.34237.robin.rosenberg@dewire.com>
In-Reply-To: <7vmysy7oav.fsf@gitster.siamese.dyndns.org>

onsdag 28 november 2007 skrev Junio C Hamano:

Show 21 quoted lines

> This looks somewhat tighter than the previous one, but still made me
> worried if the caller of prefix_path() has run the setup sequence enough
> so that calling get_git_work_tree() is safe, so I ended up auditing the
> callpath.  At least, I do not want to see that unconditional call to
> get_git_work_tree() when we do not need to do this "ah prefix got an
> unusual absolute path" stuff.
> 
>  * builtin-init-db.c uses prefix_path() to find where the template is
>    (this is mingw fallout change); in general, I do not think we would
>    want to trigger repository nor worktree discovery inside init-db,
>    although I suspect this particular callpath could be made Ok (because
>    it is taken only when template_dir is not absolute) if you do not
>    unconditionally call get_git_work_tree() in prefix_path().
> 
>  * config.c uses prefix_path() to find the ETC_GITCONFIG that is not
>    absolute (again, mingw fallout).  When git_config() is called, we
>    already should have discovered repository but worktree may not have
>    been found yet (config.worktree can be used to specify where it is,
>    so you have a chicken and egg problem).  Again, this particular
>    callpath happens to be Ok because this is used only for non-absolute
>    path, but that is a bit subtle.

I wonder if this usage in config and initdb is what prefix_path() was intended for. The interface is declared in cache.h and there are error conditions like '%s is outside repository'.

Maybe we should have a boolean indicating whether the arguments refer to filespecs or not to make this clear or rewite the mingw fallouts in some other way.

Show 6 quoted lines

>  * get_pathspec() uses prefix_path() for obvious reasons, and the prefix
>    it gets must have been discovered by finding out where the worktree
>    is, so by definition that one is safe.
> 
> Everybody else you would get from "git grep prefix_path" are after the
> proper setup, so they should all be safe.

Thanks for looking at the usages. I'll come up with some more tests too, though writing negative tests sometimes is a challenge. Tests all to easily fail for the wrong reason which is bad when we expect them to fail for the right reason.

-- robin