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

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

[Eric Raible]

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

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

[Junio C Hamano]

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

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.     

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

[Eric Raible]

On Thu, Jul 3, 2008 at 2:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

Exactly.  And as you pointed out it doesn't break existing usage by design.

>  (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;

Done.  This version is more careful about return values as well.

>  (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?

I agree these are nice but get_sha1_oneline() is time-consuming as is
and I'm not familiar enough with the code to attempt a complete
refactorization.

And without refactoring get_sha1_oneline() would need to be called
repeated for each ~, ^, or : (working backward) until it succeeds.

I'll leave it to Johannes to refactor get_sha1_oneline to handle the
":/A~20" or ":/A^2" cases if he chooses to, as it's currently beyond
my git-fu.

An alternative suggested by Peff:
	:/ should stop eating text at the first ':', and allow '\:' for
	a literal colon and '\\' for a literal backslash.

is arguably better because it's completely unambiguous and allows
for a simpler implementation but it's not backwards compatible.
I'd be willing to come up with patch for that if people agree that
it's worth it.

Please note that the commit message uses the current subject,
so if this is applied it should be kept consistent.

- Eric

----- 8< -----

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.

For instance, to see the file changed by this patch one could say:
	git show ":/Allow ':/<oneline prefix>'":sha1_name.c

Note that this should but doesn't handle ~ and ^, so it will fail when
trying to show the parent of this commit message by using
	git show ":/Allow ':/<oneline prefix>'"^

Signed-off-by: Eric Raible <raible@gmail.com>
---
 sha1_name.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index b0b2167..415b4ce 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -697,8 +697,20 @@ 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] == '/') {
+			char *copy, *colon;
+			name += 2;
+			ret = get_sha1_oneline(name, sha1);
+			if (!ret || !(colon = strrchr(name, ':')))
+				return ret;
+			copy = xstrdup(name);
+			copy[colon - name] = '\0';
+			ret = get_sha1_oneline(copy, sha1);
+			if (!ret)
+				ret = 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.g3885.dirty

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

[Johannes Schindelin]

Hi,

is there a reason you break the mail thread quite a couple of times?  It 
makes it really hard to follow when you get 300+ mails a day.

Thankyouverymuch,
Dscho

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

[Johannes Schindelin]

Hi,

On Thu, 3 Jul 2008, Junio C Hamano wrote:

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