git-rev-list: make --dense the default (and introduce "--sparse")

git-rev-list: make --dense the default (and introduce "--sparse")

[Linus Torvalds]

This actually does three things:

 - make "--dense" the default for git-rev-list. Since dense is a no-op if 
   no filenames are given, this doesn't actually change any historical 
   behaviour, but it's logically the right default (if we want to prune on 
   filenames, do it fully. The sparse "merge-only" thing may be useful, 
   but it's not what you'd normally expect)

 - make "git-rev-parse" show the default revision control before it shows 
   any pathnames.

   This was a real bug, but nobody would ever have noticed, because 
   the default thing tends to only make sense for git-rev-list, and 
   git-rev-list didn't use to take pathnames.

 - it changes "git-rev-list" to match the other commands that take a mix 
   of revisions and filenames - it no longer requires the "--" before 
   filenames (although you still need to do it if a filename could be 
   confused with a revision name, eg "gitk" in the git archive)

This all just makes for much more pleasant and obvous usage. Just doing a

	gitk t/

does the obvious thing: it will show the history as it concerns the "t/" 
subdirectory.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

NOTE! None of this will change anything that used to work before. But 
things that used to make git-rev-list die with a usage message will now 
possibly do things. In particular, it may do something that you don't 
expect.

For example, let's say that you mis-type a revision name, and do

	gitk v0.99.88

with one "8" too much. It used to cause gitk to die on you, echoing the 
usage string from git-rev-list. Now it will cause gitk to start up 
happily, and say "No commits selected", because there is no _file_ called 
"v0.99.88" anywhere, so git-rev-list will end up with empty output.

So it does change behaviour, even if it doesn't change anything that was 
_successful_ before.

For the same reasons, a

	git-rev-list $(git-rev-parse --default HEAD v0.99.88)

will now give an empty list rather than an error usage string.

(But doing

	git-rev-list v0.99.88

will still cause an error, since that doesn't have any revision at all!)

Comments? It really makes usage more "obvious", I think.


diff --git a/rev-list.c b/rev-list.c
index edf3b37..ac7a47f 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -28,7 +28,7 @@ static const char rev_list_usage[] =
 		      "  --merge-order [ --show-breaks ]\n"
 		      "  --topo-order";
 
-static int dense = 0;
+static int dense = 1;
 static int unpacked = 0;
 static int bisect_list = 0;
 static int tag_objects = 0;
@@ -619,7 +619,7 @@ static struct commit *get_commit_referen
 	struct object *object;
 
 	if (get_sha1(name, sha1))
-		usage(rev_list_usage);
+		return NULL;
 	object = parse_object(sha1);
 	if (!object)
 		die("bad object %s", name);
@@ -793,12 +793,12 @@ int main(int argc, const char **argv)
 			dense = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--sparse")) {
+			dense = 0;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
-			paths = get_pathspec(prefix, argv + i + 1);
-			if (paths) {
-				limited = 1;
-				diff_tree_setup_paths(paths);
-			}
+			i++;
 			break;
 		}
 
@@ -830,9 +830,20 @@ int main(int argc, const char **argv)
 			limited = 1;
 		}
 		commit = get_commit_reference(arg, flags);
+		if (!commit)
+			break;
 		handle_one_commit(commit, &list);
 	}
 
+	if (!list)
+		usage(rev_list_usage);
+
+	paths = get_pathspec(prefix, argv + i);
+	if (paths) {
+		limited = 1;
+		diff_tree_setup_paths(paths);
+	}
+
 	save_commit_buffer = verbose_header;
 	track_object_refs = 0;
 
diff --git a/rev-parse.c b/rev-parse.c
index 243f89f..adfc68c 100644
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -153,6 +153,7 @@ static void show_datestring(const char *
 
 static void show_file(const char *arg)
 {
+	show_default();
 	if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV))
 		show(arg);
 }
@@ -174,7 +175,6 @@ int main(int argc, char **argv)
 		if (*arg == '-') {
 			if (!strcmp(arg, "--")) {
 				as_is = 1;
-				show_default();
 				/* Pass on the "--" if we show anything but files.. */
 				if (filter & (DO_FLAGS | DO_REVS))
 					show_file(arg);

Re: git-rev-list: make --dense the default (and introduce "--sparse")

[Linus Torvalds]

On Tue, 25 Oct 2005, Linus Torvalds wrote:
> 
> This actually does three things:
> 
>  - make "--dense" the default for git-rev-list. Since dense is a no-op if 
>    no filenames are given, this doesn't actually change any historical 
>    behaviour, but it's logically the right default (if we want to prune on 
>    filenames, do it fully. The sparse "merge-only" thing may be useful, 
>    but it's not what you'd normally expect)
> 
>  - make "git-rev-parse" show the default revision control before it shows 
>    any pathnames.

Btw, if it wasn't obvious, this has some subtle (and very nice) side 
effects.

For example, try this:

	git log git-fetch.sh

and notice how "git log" automagically became able to give per-file logs. 
With _zero_ changes to git-log.sh itself ;)

And same old rev-confusion-avoidance:

	git log -- gitk

to make clear that you want to get the log of "gitk the file", rather than 
"gitk the branch".

Magic. 

		Linus

Re: git-rev-list: make --dense the default (and introduce "--sparse")

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Tue, 25 Oct 2005, Linus Torvalds wrote:
>> 
>> This actually does three things:
>> 
>>  - make "--dense" the default for git-rev-list...

Heads up.

I have not looked closely into what exactly, but the fourth
thing this does might be to break git-send-pack.

I usually use the tip of "pu" myself, but for tonight, I am
excluding the fetch-pack/upload-pack changes from Johannes when
building git for my own use, and using somewhere in the middle
of "pu" branch.  With this "--dense default" patch,
git-send-pack seems to send too few objects.  With this patch
reverted, git-send-pack seems to work again.

 +  [build] Revert "git-rev-list: make --dense the default (and introduce "--sparse")"
 ++ [pu^] Merge branch 'js-fat'
 ++ [pu^^2] Test in git-init-db if the filemode can be trusted
 ++ [pu~2] Merge branches 'cache-pack', 'lazy-subdir' and 'lt-dense'
 ++ [pu~2^4] git-rev-list: make --dense the default (and introduce "--sparse")
 ++ [pu~2^3] Create object subdirectories on demand (phase II)
 ++ [pu~2^2] Allow caching of generated pack for full cloning.
+++ [master] upload-pack: tighten request validation.

I'll take a look at the issue in the morning unless somebody
else beats me to it.

Re: git-rev-list: make --dense the default (and introduce "--sparse")

[Linus Torvalds]

On Wed, 26 Oct 2005, Junio C Hamano wrote:
> 
> I have not looked closely into what exactly, but the fourth
> thing this does might be to break git-send-pack.

Ack. And I see why.

What happens is that the new logic decides that if it can't look up a 
commit reference (ie "get_commit_reference()" returns NULL), the thing 
must be a pathname.

Fair enough.

But wrong.

The thing is, it may be a perfectly fine ref that _isn't_ a commit. In 
git, you have a tag that points to your PGP key, and in the kernel, I have 
a tag that points to a tree (and a direct ref that points to that tree 
too, for that matter). 

So the rule is (as for all the other programs that mix revs and pathnames) 
not that we only accept commit references, but _any_ valid object ref.

If the object then isn't a commit ref, git-rev-list will either ignore it, 
or add it to the list of non-commit objects (if using "--objects").

The solution is to move the "get_sha1()" out of get_commit_reference(), 
and into the callers. In fact, we already _have_ the SHA1 in the case of 
the handle_all() loop, since for_each_ref() will have done it for us, so 
this is the correct thing to do anyway. 

This patch (on top of the original one) does exactly that.

		Linus

----
diff --git a/rev-list.c b/rev-list.c
index ac7a47f..2b82b8a 100644
--- a/rev-list.c
+++ b/rev-list.c
@@ -613,13 +613,10 @@ static void add_pending_object(struct ob
 	add_object(obj, &pending_objects, name);
 }
 
-static struct commit *get_commit_reference(const char *name, unsigned int flags)
+static struct commit *get_commit_reference(const char *name, const unsigned char *sha1, unsigned int flags)
 {
-	unsigned char sha1[20];
 	struct object *object;
 
-	if (get_sha1(name, sha1))
-		return NULL;
 	object = parse_object(sha1);
 	if (!object)
 		die("bad object %s", name);
@@ -697,7 +694,7 @@ static struct commit_list **global_lst;
 
 static int include_one_commit(const char *path, const unsigned char *sha1)
 {
-	struct commit *com = get_commit_reference(path, 0);
+	struct commit *com = get_commit_reference(path, sha1, 0);
 	handle_one_commit(com, global_lst);
 	return 0;
 }
@@ -720,6 +717,7 @@ int main(int argc, const char **argv)
 		const char *arg = argv[i];
 		char *dotdot;
 		struct commit *commit;
+		unsigned char sha1[20];
 
 		if (!strncmp(arg, "--max-count=", 12)) {
 			max_count = atoi(arg + 12);
@@ -808,15 +806,19 @@ int main(int argc, const char **argv)
 		flags = 0;
 		dotdot = strstr(arg, "..");
 		if (dotdot) {
+			unsigned char from_sha1[20];
 			char *next = dotdot + 2;
-			struct commit *exclude = NULL;
-			struct commit *include = NULL;
 			*dotdot = 0;
 			if (!*next)
 				next = "HEAD";
-			exclude = get_commit_reference(arg, UNINTERESTING);
-			include = get_commit_reference(next, 0);
-			if (exclude && include) {
+			if (!get_sha1(arg, from_sha1) && !get_sha1(next, sha1)) {
+				struct commit *exclude;
+				struct commit *include;
+				
+				exclude = get_commit_reference(arg, from_sha1, UNINTERESTING);
+				include = get_commit_reference(next, sha1, 0);
+				if (!exclude || !include)
+					die("Invalid revision range %s..%s", arg, next);
 				limited = 1;
 				handle_one_commit(exclude, &list);
 				handle_one_commit(include, &list);
@@ -829,9 +831,9 @@ int main(int argc, const char **argv)
 			arg++;
 			limited = 1;
 		}
-		commit = get_commit_reference(arg, flags);
-		if (!commit)
+		if (get_sha1(arg, sha1) < 0)
 			break;
+		commit = get_commit_reference(arg, sha1, flags);
 		handle_one_commit(commit, &list);
 	}
 

Re: git-rev-list: make --dense the default (and introduce "--sparse")

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Wed, 26 Oct 2005, Junio C Hamano wrote:
>> 
>> I have not looked closely into what exactly, but the fourth
>> thing this does might be to break git-send-pack.
>
> Ack. And I see why.
>
>
> This patch (on top of the original one) does exactly that.

Thanks, this fixes it.