bug: --shallow-since misbehaves on old branch heads

To avoid this, if rev-list returns nothing, we abort the clone/fetch. The user could adjust their request (e.g. --shallow-since further back in the past) and retry.

Another possible option for this case is to fall back to a default depth (like depth 1). But I don't like too much magic that way because we may return something unexpected to the user.

Note that we need to die() in get_shallow_commits_by_rev_list() instead of just checking for empty result from its caller deepen_by_rev_list() and handling the error there. The reason is, empty result could be a valid case: if you have commits in year 2013 and you request --shallow-since=year.2000 then you should get a full clone (i.e. empty result).

Reported-by Andreas Krey <[email protected]>

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 shallow.c             |  3 +++
 t/t5500-fetch-pack.sh | 11 +++++++++++
 2 files changed, 14 insertions(+)

shallow.c +3 -0

index df4d44ea7a..44fdca1ace 100644
@@ -175,6 +175,9 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
 		die("revision walk setup failed");
 	traverse_commit_list(&revs, show_commit, NULL, &not_shallow_list);
 
+	if (!not_shallow_list)
+		die("no commits selected for shallow requests");
+
 	/* Mark all reachable commits as NOT_SHALLOW */
 	for (p = not_shallow_list; p; p = p->next)
 		p->item->object.flags |= not_shallow_flag;

t/t5500-fetch-pack.sh +11 -0

index 0680dec808..c8f2d38a58 100755
@@ -711,6 +711,17 @@ test_expect_success 'fetch shallow since ...' '
 	test_cmp expected actual
 '
 
+test_expect_success 'clone shallow since selects no commits' '
+	test_create_repo shallow-since-the-future &&
+	(
+	cd shallow-since-the-future &&
+	GIT_COMMITTER_DATE="100000000 +0700" git commit --allow-empty -m one &&
+	GIT_COMMITTER_DATE="200000000 +0700" git commit --allow-empty -m two &&
+	GIT_COMMITTER_DATE="300000000 +0700" git commit --allow-empty -m three &&
+	test_must_fail git clone --shallow-since "900000000 +0700" "file://$(pwd)/." ../shallow111
+	)
+'
+
 test_expect_success 'shallow clone exclude tag two' '
 	test_create_repo shallow-exclude &&
 	(

Quoted 3 lines · Nguyễn Thái Ngọc Duy

To avoid this, if rev-list returns nothing, we abort the clone/fetch. The user could adjust their request (e.g. --shallow-since further back in the past) and retry.

Yeah, that makes sense.

Quoted 3 lines

Another possible option for this case is to fall back to a default depth (like depth 1). But I don't like too much magic that way because we may return something unexpected to the user.

I agree that it would be a horrible fallback. I actually am wondering if we should just silently return no objects without even telling the user there is something unexpected happening. After all, the user may well be expecting with --shallow-since that is too recent that the fetch may not result in pulling anything new, and giving a "die" message, which now needs to be distinguished from other forms of die's like network connectivity or auth failures, is not all that helpful.

Quoted 12 lines · Junio C Hamano

Quoted 3 lines · Nguyễn Thái Ngọc Duy

To avoid this, if rev-list returns nothing, we abort the clone/fetch. The user could adjust their request (e.g. --shallow-since further back in the past) and retry.

Yeah, that makes sense.

Quoted 3 lines

Another possible option for this case is to fall back to a default depth (like depth 1). But I don't like too much magic that way because we may return something unexpected to the user.

Quoted 8 lines

I agree that it would be a horrible fallback. I actually am wondering if we should just silently return no objects without even telling the user there is something unexpected happening. After all, the user may well be expecting with --shallow-since that is too recent that the fetch may not result in pulling anything new, and giving a "die" message, which now needs to be distinguished from other forms of die's like network connectivity or auth failures, is not all that helpful.

An empty fetch is probably ok (though I would need to double check if anything bad would happen or git-fetch would give some helpful suggestion). git-clone on the other hand should actually clean this up with a good advice. I'll need to check and come back with v2 later.

Shallow clones with --shallow-since or --shalow-exclude work by running rev-list to get all reachable commits, then draw a boundary between reachable and unreachable and send "shallow" requests based on that.

The code does miss one corner case: if rev-list returns nothing, we'll have no border and we'll send no shallow requests back to the client (i.e. no history cuts). This essentially means a full clone (or a full branch if the client requests just one branch). One example is the oldest commit is older than what is specified by --shallow-since.

Quoted 5 lines · Nguyễn Thái Ngọc Duy

Shallow clones with --shallow-since or --shalow-exclude work by running rev-list to get all reachable commits, then draw a boundary between reachable and unreachable and send "shallow" requests based on that.

Quoted 5 lines

The code does miss one corner case: if rev-list returns nothing, we'll have no border and we'll send no shallow requests back to the client (i.e. no history cuts). This essentially means a full clone (or a full branch if the client requests just one branch). One example is the oldest commit is older than what is specified by --shallow-since.

"the newest commit is older than", isn't it? That is, the cutoff point specified is newer than the existing history.