Re: [PATCH] Make Git accept absolute path names for files within the work tree
Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
> I will not surrender to the fierce competion on this subject. Here is
> an update with hopefully correct test cases this time.
> ... Like Linus, this code does not resolve symlinks,
> but I forgot to state that it is by design.
Perhaps state it in the commit log message?
> static const char *add_prefix(const char *prefix, const char *path)
> {
> + return prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
> }Ok; prefix_path can get NULL prefix (not complaining; just a reminder in the following discussion).
Show 10 quoted lines
> diff --git a/setup.c b/setup.c
> index 2c7b5cb..1f0ec79 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -4,9 +4,62 @@
> static int inside_git_dir = -1;
> static int inside_work_tree = -1;
>
> +static
> +const char *strip_work_tree_path(const char *prefix, int len, const char *path)
Style. "static" not on its own line.
> +{
> + const char *work_tree = get_git_work_tree();
> + int n = strlen(work_tree);Preconditions.
* prefix could be NULL or path to the subdirectory the user's
non-absolute path should be relative to, expressed as a relative path
to the top of the work tree, including a trailing slash. len is the
length of the prefix string.
* path was determined by the caller to be absolute.
* It is assumed that get_git_work_tree() always gives absolute path,
and without trailing slash.
* Does prefix always NULL if we are at the top, and never "", I wonder.
But lets assume that, too.
> + if (strncmp(path, work_tree, n))
> + return path;
If the given path is outside the work tree, return absolute as-is. After this point we know path matches the work tree
> + if (!prefix && !path[n])
> + return path + n;
If we are at the top of the work tree and path names the top of the work tree, then we return "".
> + if (!prefix) {
> + if (path[n] == '/')
> + return path + n + 1;If we are at the top of the work tree and the path names the top of the work tree followed by a slash and then something, that is a path inside the work tree. Return relative to the top of the work tree.
Show 6 quoted lines
> + else
> + if (path[n])
> + return path;
> + else
> + return path + n;
> + }
Style. "else if" would give you shallower indentation. We know path[n] was not slash, and if it is not NUL then path is not inside the work tree but is a neighbour (e.g. worktree is /a/b and path is /a/bc). Return absolute. Otherwise the path names the top of the work tree itself so we return "".
Now at this point, we know we are in a subdirectory, because the above if (!prefix) part always return. So the test for prefix here is unnecessary.
> + if (prefix && !path[n])
> + return path;
If we are in a subdirectory, and path names the top of the work tree, we return it as-is (i.e. absolute). This feels a bit inconsistent with the part that follows, which tries to make things relative by using "../", doesn't it?
> + if (strncmp(path + n + 1, prefix, len - 1)) {For !prefix case we have determined path is not merely a neighbour, but we haven't checked that in this codepath. If the parameters were like this:
path = /axbc/e
work_tree = /a
n = 2
prefix = bc/
len = 3this check says "fine, path is under prefix and we won't add ../ uplevels". You need to have
if (path[n] != '/')
return path;before this strncmp() for it to work, don't you?
In addition, by comparing (len - 1) excluding the trailing slash of prefix, I think you would let
path = /a/bcye
slip through as well. That is inside the work_tree but outside of your prefix.
> + fprintf(stderr,"prefix mismatch\n");
> + char *np;
> + int i;
> + int d=0;
Style "d = 0" (and "decl after statement").
> + for (i = 0; i < len; ++i)
Style. Distracts the reader by forcing him to wonder needlessly if there is a particular reason for pre-increment of i instead of the usual post-increment.
> + if (prefix[i] == '/')
> + d++;
> + np = xmalloc(strlen(path + n) + d * 3 + 1);
At this point (assuming that the above if (strncmp()) rejected the path outside the prefix correctly), we know that we would need to go d levels up to reach the top of the work tree.
> + for (i=0; i < d * 3; i += 3)
> + strcpy(np + i, "../");
> + strcpy(np + i, path + n + 1);
As path+n+1 is relative to the work tree, this will make it relative, which is good.
> + path = np;
> + return np;
> + }
Assuming the if (strncmp()) above correctly handled the path outside prefix, we are dealing with the path that is inside prefix at this point. (len+n) is the length of the prefix directory expressed as an absolute path.
> + if (path[len + n] == '/')
> + return path + len + n + 1;
So strip the absolute prefix would make the result relative to the prefix directory. Nice.
> + else
> + if (path[len + n])
> + return path;
The same comment on "else if" applies. path[len+n] was not slash so path was not inside the prefix after all. Oops? The "if outside prefix we uplevel with ../" logic above should have handled this case and we should not be here.
> + else
> + return path + len + n;
> +}
path[len+n] was NUL, which means taht the user named the prefix directory, and we return "".
Isn't this _overly_ complicated? I think what this function wants to do is:
* See if path is outside the work tree, and return absolute if so.
* Come up with the absolute path for the prefix (if NULL then that is
the same as work tree) directory, without a trailing slash, and call
it X.
* Is path the same as the X? If so, "" is what you want.
* Is path a prefix of the "X/"? If so strip "X/" and return.
* Find the longuest common leading directory of path and "X/" and call
it "C/". Note that this is guaranteed to be inside work tree because
we rejected paths outside work tree upfront.
* Count slashes between "C/" and "X/" and come up with necessary
uplevel "../". Strip "C/" from path and prepend the uplevel.
Anatol Pomozov
