Re: [PATCH] get_sha1: support relative path "<obj>:<sth>" syntax
From: Matthieu Moy <hidden>
Date: 2016-06-15 22:50:00
Nguyễn Thái Ngọc Duy [off-list ref] 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; + } + }
quoted hunk
@@ -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/