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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2007, 08:43 UTC
Message-ID: <7vmysy7oav.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1196205847-22968-1-git-send-email-robin.rosenberg@dewire.com>

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

Show 39 quoted lines

> This patch makes it possible to drag files and directories from
> a graphical browser and drop them onto a shell and feed them
> to common git operations without editing away the path to the
> root of the work tree.
>
> Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> ---
>  setup.c               |   19 +++++++++++++++++
>  t/t3904-abspatharg.sh |   55 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 0 deletions(-)
>  create mode 100755 t/t3904-abspatharg.sh
>
> diff --git a/setup.c b/setup.c
> index f512ea0..ffc30bf 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -7,6 +7,25 @@ static int inside_work_tree = -1;
>  const char *prefix_path(const char *prefix, int len, const char *path)
>  {
>  	const char *orig = path;
> +	const char *work_tree = get_git_work_tree();
> +	if (is_absolute_path(path) && work_tree) {
> +		int n = strlen(work_tree);
> +		if (!strncmp(path, work_tree, n) && (path[n] == '/' || !path[n])) {
> +			if (path[n])
> +				path += n + 1;
> +			else
> +				path += n;
> +
> +			if (prefix && !strncmp(path, prefix, len - 1)) {
> +				if (path[len - 1] == '/')
> +					path += len;
> +				else
> +					if (!path[len - 1])
> +						path += len - 1;
> +			}
> +		}
> +	}
> +

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.

 * 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.

Show 22 quoted lines

> diff --git a/t/t3904-abspatharg.sh b/t/t3904-abspatharg.sh
> new file mode 100755
> index 0000000..cd4a52e
> --- /dev/null
> +++ b/t/t3904-abspatharg.sh
> @@ -0,0 +1,55 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2007 Robin Rosenberg
> +#
> +
> +test_description='Test absolute filename arguments to various git
> +commands.  Absolute arguments pointing to a location within the git
> +work tree should behave the same as relative arguments.  '
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'add files using absolute path names' '
> +echo a >afile &&
> +echo b >bfile &&
> +git-add afile &&
> +git-add "$(pwd)/bfile" &&

This looks quite dense. With indent like other existing tests this would become a bit easier to read.

> +test "afile bfile" = "$(echo $(git ls-files))"

Hmmm.  Looks a bit ugly but Ok.

Show 7 quoted lines

> +mkdir x &&
> +cd x &&
> +echo c >cfile &&
> +echo d >dfile &&
> +git-add cfile &&
> +git-add "$(pwd)" &&
> +cd .. &&

If this sequence fails in the middle, the next test will execute in a wrong directory. Instead of "cd there && ... && cd .. &&", it is safer to do "( cd there && ... ) &&".

> +test "afile bfile x/cfile x/dfile" = "$(echo $(git ls-files))" &&
> +git ls-files x >f1 &&
> +git ls-files "$(pwd)/x" >f2 &&
> +diff f1 f2

"diff -u" is much easier for people who ends up reading the output when something goes wrong.

Cases that are still not covered:

	try to add a file in the parent directory from a subdirectory,
	using absolute path;

	try to add a file in a directory that is too high (i.e. outside
	the work tree), using absolute path;

The latter I think should fail. People tend to forget writing negative tests when they are too excited with their shiny new features.

Perhaps adding something like:

	try to add a sub-subdirectory using an absolute path, and make
	sure paths in subdirectory are not added;

would also be prudent to catch future possible breakages (off by one cutting the common directory prefix one level too deep or something), but that is probably just me who tends to be paranoid.

Exactly the same comments apply to tests for other commands. Also I think mv, rm, reset and checkout take pathnames.