threads / discuss / 14272

Re: PATCH: allow ':/<oneline prefix>' notation to specify a specific file

Subject: Re: PATCH: allow ':/<oneline prefix>' notation to specify a specific file

## tl;dr

4 messages between Jul 3, 2008 and Jul 3, 2008.

replies: 3people: 3as markdown or json

Eric Raible· Jul 3, 2008, 08:52 UTC · lore

PATCH: allow ':/<oneline prefix>' notation to specify a specific file

Although the rev-parse documentation claims that the tree-ish:path/to/file syntax is applicable to all tree-ish forms this is not so when using the :/ "oneline prefix" syntax introduced in v1.5.0.1-227-g28a4d94.

This patch allows git show ":/PATCH: allow":sha1_name.c to show the change to the file changed by this patch.

Signed-off-by: Eric Raible <raible@gmail.com>
---
 sha1_name.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index b0b2167..a1acfcd 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -684,6 +684,7 @@ int get_sha1_with_mode(const char *name, unsigned
char *sha1, unsigned *mode)
 	int ret, bracket_depth;
 	int namelen = strlen(name);
 	const char *cp;
+	char *copy, *colon;

 	*mode = S_IFINVALID;
 	ret = get_sha1_1(name, namelen, sha1);
@@ -697,8 +698,18 @@ int get_sha1_with_mode(const char *name, unsigned
char *sha1, unsigned *mode)
 		int stage = 0;
 		struct cache_entry *ce;
 		int pos;
-		if (namelen > 2 && name[1] == '/')
-			return get_sha1_oneline(name + 2, sha1);
+		if (namelen > 2 && name[1] == '/') {
+			name += 2;
+			colon = strrchr(name, ':');
+			if (!get_sha1_oneline(name, sha1) || !colon)
+				return 0;
+			copy = xstrdup(name);
+			*(colon = strrchr(copy, ':')) = '\0';
+			ret = get_sha1_oneline(copy, sha1) ||
+				get_tree_entry(sha1, colon+1, sha1, mode);
+			free(copy);
+			return ret;
+		}
 		if (namelen < 3 ||
 		    name[2] != ':' ||
 		    name[1] < '0' || '3' < name[1])

-- 
1.5.6.1.1356.g3be5f.dirty
Junio C Hamano· Jul 3, 2008, 09:20 UTC · re: Eric Raible · lore
"Eric Raible" <raible@gmail.com> writes:
Show 15 quoted lines
> This patch allows git show ":/PATCH: allow":sha1_name.c to show the
> change to the file changed by this patch.
> ...
> @@ -697,8 +698,18 @@ int get_sha1_with_mode(const char *name, unsigned
> char *sha1, unsigned *mode)
>  		int stage = 0;
>  		struct cache_entry *ce;
>  		int pos;
> -		if (namelen > 2 && name[1] == '/')
> -			return get_sha1_oneline(name + 2, sha1);
> +		if (namelen > 2 && name[1] == '/') {
> +			name += 2;
> +			colon = strrchr(name, ':');
> +			if (!get_sha1_oneline(name, sha1) || !colon)
> +				return 0;

So when you have ":/A:B:C", you first try to look for string "A:B:C", and then when it fails try "A:B" and look for path C? I think this fallback makes sense, especially because this cannot break existing use for positive lookup (it _can_ be called a regression if you are checking to see if you have a commit that has A:B:C and you want the lookup to fail if there is A:B that happens to have path C, but I do not think we would care about that usage).

A few observations:
 (1) An obvious micro-optimization. Check if "A:B:C" is a rev, and if and
     only if it fails, run strrchr() to find fallback point;
 (2) When you do not find either "A:B:C" nor "A:B", perhaps try "A" and
     find "B:C" as a path in it?  IOW, you may want to fallback more than
     once;
 (3) If you are given ":/A~20" or ":/A^2", perhaps you would want to do
     something similar?
Other than that, I like what the patch is trying to do.
Johannes Schindelin· Jul 3, 2008, 12:55 UTC · re: Junio C Hamano · lore
Hi,
On Thu, 3 Jul 2008, Junio C Hamano wrote:
Show 25 quoted lines
> "Eric Raible" <raible@gmail.com> writes:
> 
> > This patch allows git show ":/PATCH: allow":sha1_name.c to show the
> > change to the file changed by this patch.
> > ...
> > @@ -697,8 +698,18 @@ int get_sha1_with_mode(const char *name, unsigned
> > char *sha1, unsigned *mode)
> >  		int stage = 0;
> >  		struct cache_entry *ce;
> >  		int pos;
> > -		if (namelen > 2 && name[1] == '/')
> > -			return get_sha1_oneline(name + 2, sha1);
> > +		if (namelen > 2 && name[1] == '/') {
> > +			name += 2;
> > +			colon = strrchr(name, ':');
> > +			if (!get_sha1_oneline(name, sha1) || !colon)
> > +				return 0;
> 
> So when you have ":/A:B:C", you first try to look for string "A:B:C", and
> then when it fails try "A:B" and look for path C?  I think this fallback
> makes sense, especially because this cannot break existing use for
> positive lookup (it _can_ be called a regression if you are checking to
> see if you have a commit that has A:B:C and you want the lookup to fail if
> there is A:B that happens to have path C, but I do not think we would care
> about that usage).

However, if you specify ambiguous information, you can end up with a commit when you expect a file.

I do not like the direction this is going; in hindsight, I think ":/<oneline>" was a serious mistake.

As I hinted in another mail, which should have been in the same mail thread, "log --grep" is so much more powerful and should supersede ":/<oneline>".

Let's grant ":/<oneline>" a quick and painless death.

Ciao, Dscho

← back to recent threads