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

Linus Torvalds [off-list ref] writes:

On Tue, 25 Oct 2005, Linus Torvalds wrote:

quoted

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.

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);
 	}