[PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

If the cache tree is not up-to-date, it will be updated first. So, now

	$ git show :Documentation/

will in effect show what files/directories are in the index' version
of the directory Documentation. The three commands

	$ git show :./
	$ git show :.
	$ git show :

are all equivalent and show the index' idea of the root directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sha1_name.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 6d7cd78..e3758cc 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -5,6 +5,7 @@
 #include "blob.h"
 #include "tree-walk.h"
 #include "refs.h"
+#include "cache-tree.h"
 
 static int find_short_object_filename(int len, const char *name, unsigned char *sha1)
 {
@@ -532,7 +533,7 @@ int get_sha1(const char *name, unsigned char *sha1)
 	 */
 	if (name[0] == ':') {
 		int stage = 0;
-		struct cache_entry *ce;
+		struct cache_entry *ce = NULL;
 		int pos;
 		if (namelen < 3 ||
 		    name[2] != ':' ||
@@ -561,6 +562,27 @@ int get_sha1(const char *name, unsigned char *sha1)
 			}
 			pos++;
 		}
+		if (namelen > 0 && cp[namelen - 1] == '/')
+			namelen--;
+		if (namelen == 1 && cp[0] == '.') {
+			namelen = 0;
+			cp = "";
+		}
+		if (namelen == 0 || (ce && ce_namelen(ce) > namelen &&
+					ce->name[namelen] == '/' &&
+					!memcmp(ce->name, cp, namelen))) {
+			struct cache_tree *tree =
+				cache_tree_find(active_cache_tree, cp);
+			if (!cache_tree_fully_valid(tree)) {
+				ret = cache_tree_update(active_cache_tree,
+						active_cache, active_nr, 0, 0);
+				if (ret < 0)
+					return ret;
+				tree = cache_tree_find(active_cache_tree, cp);
+			}
+			hashcpy(sha1, tree->sha1);
+			return 0;
+		}
 		return -1;
 	}
 	for (cp = name, bracket_depth = 0; *cp; cp++) {
-- 
1.4.4.2.ga854-dirty

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Junio C Hamano]

(1) Why is this needed?

(2) What does this do when the index is unmerged?

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Jakub Narebski]

Junio C Hamano wrote:

> (1) Why is this needed?

Probably for completeness.

> (2) What does this do when the index is unmerged?

I think it should show "git ls-files --unmerged --abbrev", perhaps...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

Hi,

On Fri, 22 Dec 2006, Jakub Narebski wrote:

> Junio C Hamano wrote:
> 
> > (2) What does this do when the index is unmerged?
> 
> I think it should show "git ls-files --unmerged --abbrev", perhaps...

Nah. I'd rather fail out, saying that because there are unmerged entries, 
there is no valid tree in the index.

Ciao,
Dscho

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Jakub Narebski]

Johannes Schindelin wrote:

> On Fri, 22 Dec 2006, Jakub Narebski wrote:
> 
>> Junio C Hamano wrote:
>> 
>>> (2) What does this do when the index is unmerged?
>> 
>> I think it should show "git ls-files --unmerged --abbrev", perhaps...
> 
> Nah. I'd rather fail out, saying that because there are unmerged entries, 
> there is no valid tree in the index.

Well, 'git diff' on unmerged IIRC does 'git diff -cc', so why not 'git show :'
do 'git ls-files --unmerged --abbrev'...
-- 
Jakub Narebski
Poland

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

Hi,

On Thu, 21 Dec 2006, Junio C Hamano wrote:

> (1) Why is this needed?

It was asked recently. It also serves well when you want to show the 
index.

> (2) What does this do when the index is unmerged?

I have no idea, and ATM no time to test. Will do later.

Ciao,
Dscho

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

Hi,

On Thu, 21 Dec 2006, Junio C Hamano wrote:

> (2) What does this do when the index is unmerged?

	$ git show :
	a1: unmerged (f16f906ab60483c100d1241dfc39868de9ec9fcb)
	a1: unmerged (cf84443e49e1b366fac938711ddf4be2d4d1d9e9)
	a1: unmerged (fd7923529855d0b274795ae3349c5e0438333979)
	fatal: ambiguous argument ':': unknown revision or path not in the 
	working tree. Use '--' to separate paths from revisions

We could be a bit nicer, but I think the first three lines speak for 
themselves. This is all stderr BTW.

Ciao,
Dscho

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Alex Riesen]

Johannes Schindelin, Fri, Dec 22, 2006 03:19:21 +0100:
> 
> If the cache tree is not up-to-date, it will be updated first. So, now
> 
> 	$ git show :Documentation/
> 
> will in effect show what files/directories are in the index' version
> of the directory Documentation. The three commands
> 
> 	$ git show :./
> 	$ git show :.
> 	$ git show :
> 
> are all equivalent and show the index' idea of the root directory.

That is a bit unexpected if you're not in the root directory of
repository, but in some subdir of the working directory.
Why root? Why not the current directory relative to root?

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

Hi,

On Fri, 22 Dec 2006, Alex Riesen wrote:

> Johannes Schindelin, Fri, Dec 22, 2006 03:19:21 +0100:
> > 
> > If the cache tree is not up-to-date, it will be updated first. So, now
> > 
> > 	$ git show :Documentation/
> > 
> > will in effect show what files/directories are in the index' version
> > of the directory Documentation. The three commands
> > 
> > 	$ git show :./
> > 	$ git show :.
> > 	$ git show :
> > 
> > are all equivalent and show the index' idea of the root directory.
> 
> That is a bit unexpected if you're not in the root directory of
> repository, but in some subdir of the working directory.
> Why root? Why not the current directory relative to root?

Why root? Because you are not asking for the working directory. Use "ls" 
for that. You are asking for the index. If you git-show a commit, you 
don't expect the output to be restricted by the subdirectory you're in, 
either, right?

CIao,
Dscho

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Alex Riesen]

Johannes Schindelin, Fri, Dec 22, 2006 13:18:55 +0100:
> > > If the cache tree is not up-to-date, it will be updated first. So, now
> > > 
> > > 	$ git show :Documentation/
> > > 
> > > will in effect show what files/directories are in the index' version
> > > of the directory Documentation. The three commands
> > > 
> > > 	$ git show :./
> > > 	$ git show :.
> > > 	$ git show :
> > > 
> > > are all equivalent and show the index' idea of the root directory.
> > 
> > That is a bit unexpected if you're not in the root directory of
> > repository, but in some subdir of the working directory.
> > Why root? Why not the current directory relative to root?
> 
> Why root? Because you are not asking for the working directory. Use "ls" 
> for that. You are asking for the index. If you git-show a commit, you 
> don't expect the output to be restricted by the subdirectory you're in, 
> either, right?

Of course I do. You're breaking a very well-known and widely used
idiom here: the "." means current or at least somehow related to the
current directory. Why would you expect "git show :." to show
everything from root on if your current directory is "Documentation"?
Yes, it is a commit which "git show" displays, but the patches/diffs
are restricted by filenames, and that is where the old rules come into
play. And I would actually expect ":/" to filter from the root on, and
":." to mean the current directory.

[PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

If the cache tree is not up-to-date, it will be updated first. So, now

        $ git show :Documentation/

will in effect show what files/directories are in the index' version
of the directory Documentation. The commands

        $ git show :/
        $ git show :

are equivalent and show the index' idea of the root directory.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	'tis a resend of an earlier patch, but without support for the
	bogus ":." as equivalent to ":/".

	I find this feature highly convenient when I just want to see
	what files the index contains.

 sha1_name.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 6d7cd78..6baa7a4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -5,6 +5,7 @@
 #include "blob.h"
 #include "tree-walk.h"
 #include "refs.h"
+#include "cache-tree.h"
 
 static int find_short_object_filename(int len, const char *name, unsigned char *sha1)
 {
@@ -532,7 +533,7 @@ int get_sha1(const char *name, unsigned char *sha1)
 	 */
 	if (name[0] == ':') {
 		int stage = 0;
-		struct cache_entry *ce;
+		struct cache_entry *ce = NULL;
 		int pos;
 		if (namelen < 3 ||
 		    name[2] != ':' ||
@@ -561,6 +562,23 @@ int get_sha1(const char *name, unsigned char *sha1)
 			}
 			pos++;
 		}
+		if (namelen > 0 && cp[namelen - 1] == '/')
+			namelen--;
+		if (namelen == 0 || (ce && ce_namelen(ce) > namelen &&
+					ce->name[namelen] == '/' &&
+					!memcmp(ce->name, cp, namelen))) {
+			struct cache_tree *tree =
+				cache_tree_find(active_cache_tree, cp);
+			if (!cache_tree_fully_valid(tree)) {
+				ret = cache_tree_update(active_cache_tree,
+						active_cache, active_nr, 0, 0);
+				if (ret < 0)
+					return ret;
+				tree = cache_tree_find(active_cache_tree, cp);
+			}
+			hashcpy(sha1, tree->sha1);
+			return 0;
+		}
 		return -1;
 	}
 	for (cp = name, bracket_depth = 0; *cp; cp++) {

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Alex Riesen]

On 1/9/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 	'tis a resend of an earlier patch, but without support for the
> 	bogus ":." as equivalent to ":/".

Thanks :)

> 	I find this feature highly convenient when I just want to see
> 	what files the index contains.

I find it very useful too.

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 	'tis a resend of an earlier patch, but without support for the
> 	bogus ":." as equivalent to ":/".
>
> 	I find this feature highly convenient when I just want to see
> 	what files the index contains.

I do not understand; do you mean ls-files?

In any case, I wonder if this does a sane thing if you asked
"git show :3:t/" on a fully merged index.

> @@ -561,6 +562,23 @@ int get_sha1(const char *name, unsigned char *sha1)
>  			}
>  			pos++;
>  		}
> +		if (namelen > 0 && cp[namelen - 1] == '/')
> +			namelen--;
> +		if (namelen == 0 || (ce && ce_namelen(ce) > namelen &&
> +					ce->name[namelen] == '/' &&
> +					!memcmp(ce->name, cp, namelen))) {

I may be misreading the code, but what does ce point at?  Does
this get the index sort order correctly?  For example, would
this work?

	$ echo >t- && git add t-
        $ git show :t
	$ git show :t/

> +			struct cache_tree *tree =
> +				cache_tree_find(active_cache_tree, cp);
> +			if (!cache_tree_fully_valid(tree)) {
> +				ret = cache_tree_update(active_cache_tree,
> +						active_cache, active_nr, 0, 0);
> +				if (ret < 0)
> +					return ret;

This gracefully errs out when the index is unmerged but fails to
pretend the index knows about trees, if the unmerged part of
index is outside the directory the user specified.

In short, I am not sure if it is worth it, and especially if the
motivation is to pretend as if the index contains trees, I would
be opposed to it.  The index does _not_ contain trees, and
cache-tree is a pure optimization for the next write-tree.
Nothing more.

If it (pretending as if the index contains trees) is just a
means to achieve something else worthy, I would not necessarily
oppose to that goal, but I do not see what it is, and I do not
know if the approach is right...

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

Hi,

On Tue, 9 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	'tis a resend of an earlier patch, but without support for the
> > 	bogus ":." as equivalent to ":/".
> >
> > 	I find this feature highly convenient when I just want to see
> > 	what files the index contains.
> 
> I do not understand; do you mean ls-files?

It is the same as ls-files, except "show" is shorter, is labeled as 
porcelain, easier to remember, _and_ it runs the pager automatically.

> In any case, I wonder if this does a sane thing if you asked
> "git show :3:t/" on a fully merged index.

No. That slipped through. The code will just ignore the stage (if 
specified) for trees.

> > @@ -561,6 +562,23 @@ int get_sha1(const char *name, unsigned char *sha1)
> >  			}
> >  			pos++;
> >  		}
> > +		if (namelen > 0 && cp[namelen - 1] == '/')
> > +			namelen--;
> > +		if (namelen == 0 || (ce && ce_namelen(ce) > namelen &&
> > +					ce->name[namelen] == '/' &&
> > +					!memcmp(ce->name, cp, namelen))) {
> 
> I may be misreading the code, but what does ce point at?  Does
> this get the index sort order correctly?

It is still at what cache_name_pos(cp, namelen) pointed to, or NULL if 
the entry does not exist (e.g. there is no cache entry, or all cache 
entries are lexicographically smaller).

> For example, would this work?
> 
> 	$ echo >t- && git add t-
>         $ git show :t
> 	$ git show :t/

I think so: The code explicitely checks for a trailing '/'. (See second 
last line of the hunk you quoted.)

> > +			struct cache_tree *tree =
> > +				cache_tree_find(active_cache_tree, cp);
> > +			if (!cache_tree_fully_valid(tree)) {
> > +				ret = cache_tree_update(active_cache_tree,
> > +						active_cache, active_nr, 0, 0);
> > +				if (ret < 0)
> > +					return ret;
> 
> This gracefully errs out when the index is unmerged but fails to
> pretend the index knows about trees, if the unmerged part of
> index is outside the directory the user specified.

That is correct. But in that case, we cannot sanely ask the question "what 
would the tree object look like if we committed right now?"

> In short, I am not sure if it is worth it, and especially if the 
> motivation is to pretend as if the index contains trees, I would be 
> opposed to it.  The index does _not_ contain trees, and cache-tree is a 
> pure optimization for the next write-tree. Nothing more.

Well, it _is_ the method to construct tree objects for committing them.

> If it (pretending as if the index contains trees) is just a means to 
> achieve something else worthy, I would not necessarily oppose to that 
> goal, but I do not see what it is, and I do not know if the approach is 
> right...

I wanted to have it for teaching purposes. And maybe a little bit for 
completeness.

But then it turned out that I even use it before committing, like when I 
ask "what files would be in my next revision?" It is not a question 
arising daily, but sometimes it is interesting to see _before_ committing 
them. (Note that I am not only interested in the _modified_ files.)

But if you feel strongly about that feature, don't take the patch.

Ciao,
Dscho

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It is still at what cache_name_pos(cp, namelen) pointed to, or NULL if 
> the entry does not exist (e.g. there is no cache entry, or all cache 
> entries are lexicographically smaller).
>
>> For example, would this work?
>> 
>> 	$ echo >t- && git add t-
>>         $ git show :t
>> 	$ git show :t/
>
> I think so: The code explicitely checks for a trailing '/'. (See second 
> last line of the hunk you quoted.)

The comment was not about the trailing / version, but the one without.
Have you tried them?  

>> > +			struct cache_tree *tree =
>> > +				cache_tree_find(active_cache_tree, cp);
>> > +			if (!cache_tree_fully_valid(tree)) {
>> > +				ret = cache_tree_update(active_cache_tree,
>> > +						active_cache, active_nr, 0, 0);
>> > +				if (ret < 0)
>> > +					return ret;
>> 
>> This gracefully errs out when the index is unmerged but fails to
>> pretend the index knows about trees, if the unmerged part of
>> index is outside the directory the user specified.
>
> That is correct. But in that case, we cannot sanely ask the question "what 
> would the tree object look like if we committed right now?"

But that is not the question you are asking.  "git-ls-files t/"
does work even though you still have cache.h unmerged in the
index.

> But then it turned out that I even use it before committing, like when I 
> ask "what files would be in my next revision?" It is not a question 
> arising daily, but sometimes it is interesting to see _before_ committing 
> them. (Note that I am not only interested in the _modified_ files.)

So it really is about being able to say show while you mean ls-files.

I am not opposed to that goal at all.  I think it makes it
easier to type.  But I think using cache-tree is a wrong
approach.

Re: [PATCH] sha1_name(): accept ':directory/' to get at the cache_tree

[Johannes Schindelin]

Hi,

On Tue, 9 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > It is still at what cache_name_pos(cp, namelen) pointed to, or NULL if 
> > the entry does not exist (e.g. there is no cache entry, or all cache 
> > entries are lexicographically smaller).
> >
> >> For example, would this work?
> >> 
> >> 	$ echo >t- && git add t-
> >>         $ git show :t
> >> 	$ git show :t/
> >
> > I think so: The code explicitely checks for a trailing '/'. (See second 
> > last line of the hunk you quoted.)
> 
> The comment was not about the trailing / version, but the one without.
> Have you tried them?

Of course not! I had the impression that cache_name_pos() just _had_ to 
find the correct spot, but of course you are right: it does not.

> >> > +			struct cache_tree *tree =
> >> > +				cache_tree_find(active_cache_tree, cp);
> >> > +			if (!cache_tree_fully_valid(tree)) {
> >> > +				ret = cache_tree_update(active_cache_tree,
> >> > +						active_cache, active_nr, 0, 0);
> >> > +				if (ret < 0)
> >> > +					return ret;
> >> 
> >> This gracefully errs out when the index is unmerged but fails to
> >> pretend the index knows about trees, if the unmerged part of
> >> index is outside the directory the user specified.
> >
> > That is correct. But in that case, we cannot sanely ask the question "what 
> > would the tree object look like if we committed right now?"
> 
> But that is not the question you are asking.  "git-ls-files t/"
> does work even though you still have cache.h unmerged in the
> index.

Ah! I retract that. I did _not_ want the recursive file list. I wanted the 
equivalent of HEAD:t/ _after_ a commit.

> > But then it turned out that I even use it before committing, like when I 
> > ask "what files would be in my next revision?" It is not a question 
> > arising daily, but sometimes it is interesting to see _before_ committing 
> > them. (Note that I am not only interested in the _modified_ files.)
> 
> So it really is about being able to say show while you mean ls-files.
> 
> I am not opposed to that goal at all.  I think it makes it
> easier to type.  But I think using cache-tree is a wrong
> approach.

Well, you are the master.

Ciao,
Dscho