patchMake Git accept absolute path names for files within the work tree

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               |   16 ++++++++++++++
 t/t3904-abspatharg.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 0 deletions(-)
 create mode 100755 t/t3904-abspatharg.sh

Was it this simple?

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

Decl after statement.

I do not think there is fundamental reason to object to this change, as long as the prefixing is done to the path that is trying to name a path in the working tree.

Also some codepath that does not require any work tree may want to call prefix_path(). I do not know what would happen in such a case.

Although I didn't look at all the callers, I think the caller from config.c is not talking about a path in the work tree, and not all users of config.c need to have work-tree.

tisdag 27 november 2007 skrev Junio C Hamano:

[...]

Stupid C..

ok, I'll add a test for whether we have a work tree.

Oh dear. That wasn't on master when I looked. That one seems to be the only "other" use and it is wrapped inside an if (!absolute_path()) tests so my magic does not conflict there.

-- robin

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

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

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.

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

Hmmm.  Looks a bit ugly but Ok.

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 && ... ) &&".

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

onsdag 28 november 2007 skrev Junio C Hamano:

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.

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

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

I wondered the same, but my vote would be to make them separate functions. You would need a function to abstract out pasting of paths to ease MinGW people even without this "ah absolute -- let's make it relative to work tree first" hack, anyway.

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

Could you structure this part to read like this, into a separate function:

static const char *strip_work_tree_path(const char *prefix, int len, const char *path)
{
	...
}

const char *prefix_path(const char *prefix, int len, const char *path)
{
	const char *orig = path;

	if (is_absolute_path(path))
		path = strip_work_tree_path(prefix, len, path);
	...

About the part that would be moved out of line with such a restructuring,

This makes me wonder what happens if after stripping the worktree path path does not match the prefix.

Robin Rosenberg schrieb:

Please use "$(pwd)/bfile" (d-quotes) throughout. Otherwise the test would fail if conducted in a directory with spaces.

-- Hannes

tisdag 27 november 2007 skrev Johannes Sixt:

AFAIK, None of the tests works in directories with spaces. But I'll fix it, just in case, so noone can blame /me/ for the tests breaking in /my home/. I cannot verify it because the testing infrastructure breaks before the actual tests start executing.

-- robin