Re: [PATCH] get_sha1: support relative path "<obj>:<sth>" syntax

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> Currently :path and ref:path can be used to refer to a specific object
> in index or ref respectively. "path" component is absolute path. This
> patch allows "path" to be written as "./path" or "../path", which is
> relative to user's original cwd.
>
> This does not work in commands that startup_info is NULL

Grammar: shouldn't it be "commands for which startup_info is NULL"?

> (i.e. non-builtin ones).

Does that include "git show"? That's probably the most common use-case
for the <object>:<path> syntax, perhaps worth mentionning here.

Also, what happens when it doesn't work? I guess it falls back to the
previous behavior, which tries to do a detailed diagnosis and provides
a general error message. But probably the error messages should be
reworded to include "syntax not supported by this command" or so,
'cause it's really weird for a user to be able to say HEAD:./foo in
some places and not in others.

>  The idea is old although I don't remember if anybody has made any
>  attempt to realize it: use './' and '../' to specify the given path
>  is relative, not absolute.

I gave it a try, but I was not aware of the startup_info thing, and
tried passing more arguments to get_sha1*, and you can guess I quickly
gave up ;-).

> +			if (startup_info && cp[0] == '.' &&
> +			    (cp[1] == '/' || (cp[1] == '.' && cp[2] == '/'))) {

Nothing performance-critical here, so I'd rather have something more
readable, like

startup_info && (!prefixcmp(cp, "./") || !prefixcmp(cp, "../"))


> +				new_path = prefix_path(startup_info->prefix,
> +						       strlen(startup_info->prefix),
> +						       cp);

free(cp); here?

> +				cp = new_path;
> +			}
> +		}

> @@ -1122,6 +1134,17 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
>  		}
>  		if (!get_sha1_1(name, cp-name, tree_sha1)) {
>  			const char *filename = cp+1;
> +			char *new_filename = NULL;
> +
> +			if (startup_info &&
> +			    filename[0] == '.' &&
> +			    (filename[1] == '/' ||
> +			     (filename[1] == '.' && filename[2] == '/'))) {
> +				new_filename = prefix_path(startup_info->prefix,
> +							   strlen(startup_info->prefix),
> +							   filename);

Same 2 questions as above.

Also, that would be nice is this came with a testcase :-)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/