Robin Rosenberg <robin.rosenberg.lists@xxxxxxxxxx> writes:
> I will not surrender to the fierce competion on this subject. Here is
> an update with hopefully correct test cases this time.
Yay, that's the spirit!
> ... 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).
> 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.
> + 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 = 3
this 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");
Stray debugging fprintf.
> + 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)
Style. "i = 0".
> + 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
