[PATCH] object-name: explain why <ref>~N fails in a shallow clone

[PATCH] object-name: explain why <ref>~N fails in a shallow clone

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

Asking for a commit's ancestor with <ref>~N or <ref>^N in a shallow
clone that does not have N commits of history locally fails with a
bare "is not a commit" error, with no indication that the repository
being shallow is the reason, or what to do about it.

Add a hint, shown when the walk runs out of parents exactly at a
recorded shallow boundary, explaining that history was intentionally
truncated there. When <ref> looks like <remote>/<branch> and <remote>
is configured, the suggested command names that remote and branch
directly. For <ref>~N it suggests the exact --deepen needed,
accounting for any history already present instead of just N. For
<ref>^N the suggestion is always --deepen=1, regardless of N: a
shallow boundary commit has no parents recorded locally at all, so
deepening by one generation fetches its complete real parent list in
one step, whether that commit turns out to have one parent or several.
The hint only fires when the search stops at an actual shallow
boundary, not merely because the repository happens to be shallow
elsewhere, so it does not misfire on a short history that is not
shallow-truncated.

The advice is threaded through GET_OID_QUIETLY so it is not shown
during the internal re-resolution some commands do while building a
better error message, which would otherwise print it twice for the
same failing argument.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
    object-name: explain why ~N fails in a shallow clone
    
    Asking for a commit's ancestor with <ref>~N in a shallow clone that
    doesn't have N commits of history locally fails with a "is not a commit"
    error, with no indication that the repository being shallow is the
    reason.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2413%2FHaraldNordgren%2Fshallow-history-advice-hint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2413/HaraldNordgren/shallow-history-advice-hint-v1
Pull-Request: https://github.com/git/git/pull/2413

 Documentation/config/advice.adoc |   4 ++
 advice.c                         |   1 +
 advice.h                         |   1 +
 object-name.c                    |  75 +++++++++++++++++++--
 t/t1500-rev-parse.sh             | 108 +++++++++++++++++++++++++++++++
 5 files changed, 184 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
index 81f80a9274..5b44037fff 100644
--- a/Documentation/config/advice.adoc
+++ b/Documentation/config/advice.adoc
@@ -128,6 +128,10 @@ all advice messages.
 		give directions on how to proceed from the current state.
 	sequencerInUse::
 		Shown when a sequencer command is already in progress.
+	shallowHistory::
+		Shown when `~<n>` or `^<n>` cannot resolve enough ancestors
+		because history stops at a shallow boundary, to suggest
+		fetching more history.
 	skippedCherryPicks::
 		Shown when linkgit:git-rebase[1] skips a commit that has already
 		been cherry-picked onto the upstream branch.
diff --git a/advice.c b/advice.c
index 63bf8b0c5f..e22706ead5 100644
--- a/advice.c
+++ b/advice.c
@@ -80,6 +80,7 @@ static struct {
 	[ADVICE_RM_HINTS]				= { "rmHints" },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
+	[ADVICE_SHALLOW_HISTORY]			= { "shallowHistory" },
 	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
 	[ADVICE_SPARSE_INDEX_EXPANDED]			= { "sparseIndexExpanded" },
 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
diff --git a/advice.h b/advice.h
index 66f6cd6a77..e80e3e82cc 100644
--- a/advice.h
+++ b/advice.h
@@ -47,6 +47,7 @@ enum advice_type {
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,
 	ADVICE_SET_UPSTREAM_FAILURE,
+	ADVICE_SHALLOW_HISTORY,
 	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_SPARSE_INDEX_EXPANDED,
 	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
diff --git a/object-name.c b/object-name.c
index 4eda8c8eac..0a18d1d2e3 100644
--- a/object-name.c
+++ b/object-name.c
@@ -22,6 +22,7 @@
 #include "repo-settings.h"
 #include "repository.h"
 #include "setup.h"
+#include "shallow.h"
 #include "midx.h"
 #include "commit-reach.h"
 #include "date.h"
@@ -824,9 +825,65 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 	return 0;
 }
 
+/*
+ * When a "name~<n>" or "name^<n>" walk runs out of parents at "commit",
+ * and that is because "commit" is where this shallow repository's history
+ * was cut off (rather than commit genuinely being a root commit), let the
+ * user know that fetching more history might be what they are after.
+ *
+ * "suggested_depth" is the --deepen value to recommend. For "name^<n>"
+ * this is always 1: deepening by one generation fetches "commit"'s real
+ * parent list in full, whatever it turns out to contain, regardless of
+ * which parent index <n> asked for. If "name" looks like
+ * "<remote>/<branch>" and <remote> is a configured remote, the suggested
+ * command names that remote and branch instead of leaving them as
+ * placeholders.
+ */
+static void advise_if_shallow_cutoff(struct repository *r,
+				     const char *name, int namelen,
+				     struct commit *commit,
+				     unsigned lookup_flags,
+				     int suggested_depth)
+{
+	struct commit_graft *graft;
+	const char *slash;
+	struct strbuf cmd = STRBUF_INIT;
+
+	if (lookup_flags & GET_OID_QUIETLY)
+		return;
+	if (!is_repository_shallow(r))
+		return;
+	graft = lookup_commit_graft(r, &commit->object.oid);
+	if (!graft || graft->nr_parent != -1)
+		return;
+
+	slash = memchr(name, '/', namelen);
+	if (slash) {
+		char *remote_candidate = xstrndup(name, slash - name);
+		if (remote_is_configured(remote_get(remote_candidate), 0))
+			strbuf_addf(&cmd, "git fetch --deepen=%d %s %.*s",
+				    suggested_depth, remote_candidate,
+				    (int)(name + namelen - (slash + 1)), slash + 1);
+		free(remote_candidate);
+	}
+	if (!cmd.len)
+		strbuf_addf(&cmd, "git fetch --deepen=%d <remote> <branch>",
+			    suggested_depth);
+
+	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
+			   _("'%.*s' does not have that many ancestors locally.\n"
+			     "History stops at %s because this repository is a\n"
+			     "shallow clone. To fetch more of it, try:\n"
+			     "\n"
+			     "  %s"),
+			   namelen, name, oid_to_hex(&commit->object.oid), cmd.buf);
+	strbuf_release(&cmd);
+}
+
 static enum get_oid_result get_parent(struct repository *r,
 				      const char *name, int len,
-				      struct object_id *result, int idx)
+				      struct object_id *result, int idx,
+				      unsigned lookup_flags)
 {
 	struct object_id oid;
 	enum get_oid_result ret = get_oid_1(r, name, len, &oid,
@@ -851,13 +908,15 @@ static enum get_oid_result get_parent(struct repository *r,
 		}
 		p = p->next;
 	}
+	advise_if_shallow_cutoff(r, name, len, commit, lookup_flags, 1);
 	return MISSING_OBJECT;
 }
 
 static enum get_oid_result get_nth_ancestor(struct repository *r,
 					    const char *name, int len,
 					    struct object_id *result,
-					    int generation)
+					    int generation,
+					    unsigned lookup_flags)
 {
 	struct object_id oid;
 	struct commit *commit;
@@ -871,8 +930,14 @@ static enum get_oid_result get_nth_ancestor(struct repository *r,
 		return MISSING_OBJECT;
 
 	while (generation--) {
-		if (repo_parse_commit(r, commit) || !commit->parents)
+		if (repo_parse_commit(r, commit))
 			return MISSING_OBJECT;
+		if (!commit->parents) {
+			/* Remaining "generation" plus this failed step is the actual gap. */
+			advise_if_shallow_cutoff(r, name, len, commit,
+						 lookup_flags, generation + 1);
+			return MISSING_OBJECT;
+		}
 		commit = commit->parents->item;
 	}
 	oidcpy(result, &commit->object.oid);
@@ -1119,9 +1184,9 @@ static enum get_oid_result get_oid_1(struct repository *r,
 		else if (num > INT_MAX)
 			return MISSING_OBJECT;
 		if (has_suffix == '^')
-			return get_parent(r, name, len1, oid, num);
+			return get_parent(r, name, len1, oid, num, lookup_flags);
 		/* else if (has_suffix == '~') -- goes without saying */
-		return get_nth_ancestor(r, name, len1, oid, num);
+		return get_nth_ancestor(r, name, len1, oid, num, lookup_flags);
 	}
 
 	ret = peel_onion(r, name, len, oid, lookup_flags);
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 4174ca40c3..4b46ba546d 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -193,6 +193,114 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '
 	test_cmp expect actual
 '
 
+check_shallow_history_advice () {
+	name=$1 oid=$2 cmd=$3 &&
+	grep '^hint:' err >actual &&
+	cat >expect <<-EOF &&
+	hint: '$name' does not have that many ancestors locally.
+	hint: History stops at $oid because this repository is a
+	hint: shallow clone. To fetch more of it, try:
+	hint:
+	hint:   $cmd
+	hint: Disable this message with "git config set advice.shallowHistory false"
+	EOF
+	test_cmp expect actual
+}
+
+test_expect_success 'shallowHistory advice on ~N beyond shallow boundary' '
+	test_commit shallow_advice_1 &&
+	test_commit shallow_advice_2 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice &&
+	test_when_finished "rm -rf shallow-advice" &&
+	oid=$(git -C shallow-advice rev-parse origin/main) &&
+	test_must_fail git -C shallow-advice rev-parse origin/main~1 2>err &&
+	check_shallow_history_advice origin/main "$oid" \
+		"git fetch --deepen=1 origin main"
+'
+
+test_expect_success 'shallowHistory advice accounts for depth already present' '
+	test_commit shallow_partial_1 &&
+	test_commit shallow_partial_2 &&
+	test_commit shallow_partial_3 &&
+	test_commit shallow_partial_4 &&
+	test_commit shallow_partial_5 &&
+	test_commit shallow_partial_6 &&
+	git clone --no-local --depth=3 --branch main --single-branch \
+		.git shallow-advice-partial &&
+	test_when_finished "rm -rf shallow-advice-partial" &&
+	(
+		cd shallow-advice-partial &&
+		oid=$(git rev-parse origin/main~2) &&
+		test_must_fail git rev-parse origin/main~5 2>err &&
+		check_shallow_history_advice origin/main "$oid" \
+			"git fetch --deepen=3 origin main" &&
+		git fetch --deepen=3 origin &&
+		git rev-parse origin/main~5 &&
+		test_must_fail git rev-parse origin/main~6
+	)
+'
+
+test_expect_success 'shallowHistory advice on ^N (first parent) beyond shallow boundary' '
+	test_commit shallow_caret_1 &&
+	test_commit shallow_caret_2 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-caret &&
+	test_when_finished "rm -rf shallow-advice-caret" &&
+	oid=$(git -C shallow-advice-caret rev-parse origin/main) &&
+	test_must_fail git -C shallow-advice-caret rev-parse origin/main^1 2>err &&
+	check_shallow_history_advice origin/main "$oid" \
+		"git fetch --deepen=1 origin main"
+'
+
+test_expect_success 'shallowHistory advice on ^N suggests deepen=1 even for a merge parent' '
+	test_commit shallow_merge_base &&
+	git checkout -q -b shallow-merge-side &&
+	test_commit shallow_merge_side1 &&
+	git checkout -q main &&
+	test_commit shallow_merge_main1 &&
+	git merge -q --no-ff shallow-merge-side -m "shallow merge commit" &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-merge &&
+	test_when_finished "rm -rf shallow-advice-merge" &&
+	(
+		cd shallow-advice-merge &&
+		oid=$(git rev-parse origin/main) &&
+		test_must_fail git rev-parse origin/main^2 2>err &&
+		check_shallow_history_advice origin/main "$oid" \
+			"git fetch --deepen=1 origin main" &&
+		git fetch -q --deepen=1 origin &&
+		git rev-parse origin/main^1 &&
+		git rev-parse origin/main^2
+	)
+'
+
+test_expect_success 'shallowHistory advice can be disabled' '
+	test_commit shallow_off_1 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-off &&
+	test_when_finished "rm -rf shallow-advice-off" &&
+	test_must_fail git -C shallow-advice-off \
+		-c advice.shallowHistory=false rev-parse origin/main~1 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'shallowHistory advice not shown for a non-shallow repository' '
+	test_must_fail git rev-parse HEAD~100000 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'shallowHistory advice not shown when resolution succeeds' '
+	test_commit shallow_ok_1 &&
+	test_commit shallow_ok_2 &&
+	test_commit shallow_ok_3 &&
+	git clone --no-local --depth=3 --branch main --single-branch \
+		.git shallow-advice-ok &&
+	test_when_finished "rm -rf shallow-advice-ok" &&
+	git -C shallow-advice-ok rev-parse origin/main~1 >actual 2>err &&
+	test_grep ! "^hint:" err
+'
+
 test_expect_success 'rev-parse --show-object-format in repo' '
 	test_oid algo >expect &&
 	git rev-parse --show-object-format >actual &&

base-commit: d38352cd43ab9745686d697872408bc3249a153f
-- 
gitgitgadget

Re: [PATCH] object-name: explain why <ref>~N fails in a shallow clone

[D. Ben Knoble]

[This is not a complete review, just something I noticed]

On Sun, Sep 20, 2026 at 5:55 AM Harald Nordgren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Asking for a commit's ancestor with <ref>~N or <ref>^N in a shallow
> clone that does not have N commits of history locally fails with a
> bare "is not a commit" error, with no indication that the repository
> being shallow is the reason, or what to do about it.
>
> Add a hint, shown when the walk runs out of parents exactly at a
> recorded shallow boundary, explaining that history was intentionally
> truncated there. When <ref> looks like <remote>/<branch> and <remote>
> is configured, the suggested command names that remote and branch
> directly.

Here, we mention the most common "ref" cases directly, and it might
make sense to restrict this feature only to the use of refs (since we
might need such a ref to unshallow via fetch?).

> For <ref>~N it suggests the exact --deepen needed,
> accounting for any history already present instead of just N. For
> <ref>^N the suggestion is always --deepen=1, regardless of N:

But here…

> diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
> index 81f80a9274..5b44037fff 100644
> --- a/Documentation/config/advice.adoc
> +++ b/Documentation/config/advice.adoc
> @@ -128,6 +128,10 @@ all advice messages.
>                 give directions on how to proceed from the current state.
>         sequencerInUse::
>                 Shown when a sequencer command is already in progress.
> +       shallowHistory::
> +               Shown when `~<n>` or `^<n>` cannot resolve enough ancestors
> +               because history stops at a shallow boundary, to suggest
> +               fetching more history.
>         skippedCherryPicks::
>                 Shown when linkgit:git-rebase[1] skips a commit that has already
>                 been cherry-picked onto the upstream branch.

…and here, it's not clear to me what kinds of things trigger the hint.
The "~N" and "^N" syntax work with any commit-ish, after all!

Either way, the hint documentation should probably not show "bare"
"~N"/~^N", since that's not syntax accepted by Git. The
gitrevisions(7) manual uses "<rev>~<n>" and "<rev>^[<n>]".

Perhaps you meant "rev" instead of "ref" in the commit message? Hm.

-- 
D. Ben Knoble

Re: [PATCH] object-name: explain why <ref>~N fails in a shallow clone

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Asking for a commit's ancestor with <ref>~N or <ref>^N in a shallow
> clone that does not have N commits of history locally fails with a
> bare "is not a commit" error, with no indication that the repository
> being shallow is the reason, or what to do about it.

I am not sure if bringing up '^N' (the N-th parent of a merge) in an
attempt to be more complete helps readers or confuses them.  Unlike
'<rev>~N', where increasing N raises the required depth of a shallow
clone to make the target revision available, both '<rev>^1' and
'<rev>^43' of '<rev>' share the same depth.  If '<rev>' exists
locally and its first parent '<rev>^1' also does, it is likely that
'<rev>^2' is also available, as they are at the same depth from
'<rev>'.

The title of the commit does not share the problem, which is a good
thing ;-).

> Add a hint, shown when the walk runs out of parents exactly at a
> recorded shallow boundary, explaining that history was intentionally
> truncated there. When <ref> looks like <remote>/<branch> and <remote>
> is configured, the suggested command names that remote and branch

Good thinking.


As branch 'B' of remote 'R' is not necessarily stored locally at
'refs/remotes/R/B', implementing the semantics correctly and showing
the correct remote name and their branch name by reverse mapping R/B
back requires a bit of care, but it should not be impossibly hard.

> directly. For <ref>~N it suggests the exact --deepen needed,
> accounting for any history already present instead of just N. For
> <ref>^N the suggestion is always --deepen=1, regardless of N: a
> shallow boundary commit has no parents recorded locally at all, so
> deepening by one generation fetches its complete real parent list in
> one step, whether that commit turns out to have one parent or several.
> The hint only fires when the search stops at an actual shallow
> boundary, not merely because the repository happens to be shallow
> elsewhere, so it does not misfire on a short history that is not
> shallow-truncated.

I think Ben also mentioned this, but <ref> is probably better
written as <rev> in the above.  A ref(erence) like "master",
"origin/next", or "refs/remotes/origin/topic" are all rev(ision)s,
and this new advice feature is not limited to requests that are
made using references.

    When the revision <rev> is given as a remote-tracking branch,
    the remote and branch are exactly named in the suggested
    command.  For <rev>~N, it suggests ...

> The advice is threaded through GET_OID_QUIETLY so it is not shown
> during the internal re-resolution some commands do while building a
> better error message, which would otherwise print it twice for the
> same failing argument.

Nice.

> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>     object-name: explain why ~N fails in a shallow clone
>     
>     Asking for a commit's ancestor with <ref>~N in a shallow clone that
>     doesn't have N commits of history locally fails with a "is not a commit"
>     error, with no indication that the repository being shallow is the
>     reason.

It may not be intuitive to new users that in a shallow clone "git
log" stops in the middle, instead of going down to the beginning of
the history, downloading necessary objects on demand.  But fixing it
by adding such a feature is totally unrelated and outside the scope
of this topic ;-).

> diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
> index 81f80a9274..5b44037fff 100644
> --- a/Documentation/config/advice.adoc
> +++ b/Documentation/config/advice.adoc
> @@ -128,6 +128,10 @@ all advice messages.
>  		give directions on how to proceed from the current state.
>  	sequencerInUse::
>  		Shown when a sequencer command is already in progress.
> +	shallowHistory::
> +		Shown when `~<n>` or `^<n>` cannot resolve enough ancestors
> +		because history stops at a shallow boundary, to suggest
> +		fetching more history.

It is obvious that users would see such a message when they say

    $ git show HEAD~20
    $ git log HEAD~20..HEAD

but would they see the same when

    $ git log -20 HEAD
    $ git log --since=2.months HEAD

and internally HEAD~20 fails to resolve?  Should they see the same
hint?

> +	test_must_fail git -C shallow-advice rev-parse origin/main~1 2>err &&
> +	check_shallow_history_advice origin/main "$oid" \
> +		"git fetch --deepen=1 origin main"

This is very straight-forward.

> +		test_must_fail git rev-parse origin/main~5 2>err &&
> +		check_shallow_history_advice origin/main "$oid" \
> +			"git fetch --deepen=3 origin main" &&

Again, very straight-forward.

> +	test_must_fail git -C shallow-advice-caret rev-parse origin/main^1 2>err &&
> +	check_shallow_history_advice origin/main "$oid" \
> +		"git fetch --deepen=1 origin main"

Ditto.

> +		test_must_fail git rev-parse origin/main^2 2>err &&
> +		check_shallow_history_advice origin/main "$oid" \
> +			"git fetch --deepen=1 origin main" &&

Ditto.

> +test_expect_success 'shallowHistory advice not shown for a non-shallow repository' '
> +	test_must_fail git rev-parse HEAD~100000 2>err &&
> +	test_grep ! "^hint:" err
> +'

OK.

> +test_expect_success 'shallowHistory advice not shown when resolution succeeds' '
> +	test_commit shallow_ok_1 &&
> +	test_commit shallow_ok_2 &&
> +	test_commit shallow_ok_3 &&
> +	git clone --no-local --depth=3 --branch main --single-branch \
> +		.git shallow-advice-ok &&
> +	test_when_finished "rm -rf shallow-advice-ok" &&
> +	git -C shallow-advice-ok rev-parse origin/main~1 >actual 2>err &&
> +	test_grep ! "^hint:" err
> +'

OK.

I guess the answer to my earlier "does internally failing to resolve
due to graft point count?" is "no"?

Thanks.

Re: [PATCH] object-name: explain why <ref>~N fails in a shallow clone

[Harald Nordgren]

> It is obvious that users would see such a message when they say
>
>     $ git show HEAD~20
>     $ git log HEAD~20..HEAD
>
> but would they see the same when
>
>     $ git log -20 HEAD
>     $ git log --since=2.months HEAD
>
> and internally HEAD~20 fails to resolve?  Should they see the same
> hint?

But I think the 'log -20' doesn't even give an error, so where would
we show that message?


Harald

Re: [PATCH] object-name: explain why <ref>~N fails in a shallow clone

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

>> It is obvious that users would see such a message when they say
>>
>>     $ git show HEAD~20
>>     $ git log HEAD~20..HEAD
>>
>> but would they see the same when
>>
>>     $ git log -20 HEAD
>>     $ git log --since=2.months HEAD
>>
>> and internally HEAD~20 fails to resolve?  Should they see the same
>> hint?
>
> But I think the 'log -20' doesn't even give an error, so where would
> we show that message?

Where?  To their terminal ;-)?

To end users, "git log -20 HEAD" that stops only after showing 2
commits is just as puzzling as, if not more puzzling than, "git show
HEAD~20" that complains "What commit are you talking about?"

At least the latter has an error message that says the commit the
user thought they gave (i.e., HEAD~20) was not interpreted as a
commit, and that might give them enough clue to remind them that
they are the responsible ones for the unexpected beahviour by
initially cloning the repository with a "--depth" option.  On the
other hand, "git log -20" that stops way before they expect it would
does not even give them such a hint.

I thought that helping that case would have much more benefit, hence
my question.

[PATCH v2] shallow: advise when a walk stops at a shallow boundary

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

Asking for a commit's ancestor with <rev>~N in a shallow clone that
does not have N commits of history locally fails with a bare "is not
a commit" error, with no indication that the repository being shallow
is the reason, or what to do about it.

git log -<n> and git log --since=<date> have the opposite problem.
Instead of failing, they silently stop once they reach the shallow
boundary, with no error and nothing to suggest that more history
exists upstream and simply was not fetched.

Add a hint, shown when a walk runs out of parents exactly at a
recorded shallow boundary, not merely because the repository happens
to be shallow elsewhere. When the revision <rev> is given as a
remote-tracking branch, the remote and branch are exactly named in the
suggested command, found by reverse mapping through the remote's real
refspec instead of assuming a refs/remotes/<remote>/ layout, since a
refspec can put branches anywhere. For <rev>~N it suggests the exact
--deepen needed, accounting for history already present. For <rev>^N
it always suggests --deepen=1: a shallow boundary commit has no
parents recorded at all, so one more generation reveals its complete
real parent list regardless of which index was asked for.

The hint only suggests that older history might exist. A recorded
shallow boundary does not distinguish a depth cutoff from a commit
that is the true root: cloning with a depth that happens to exactly
match the available history still records that root commit as a
boundary, and only the remote knows for certain whether deepening
would fetch anything new.

git log -<n> and git log --since only show the hint when the last
commit they printed is that same recorded boundary. A plain git log
running out of history, or a git log -<n> or --since that is satisfied
well before reaching the boundary, stays silent as before. When the
output is paginated, the hint waits for the pager to close before
printing, since a full screen pager like less otherwise wipes it from
the terminal along with the rest of the output the moment it quits.

The advice is threaded through GET_OID_QUIETLY so it is not shown
during the internal re-resolution some commands do while building a
better error message, which would otherwise print it twice for the
same failing argument.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
    object-name: explain why ~N fails in a shallow clone
    
    Asking for a commit's ancestor with <rev>~N in a shallow clone that
    doesn't have N commits of history locally fails with a "is not a commit"
    error, with no indication that the repository being shallow is the
    reason.
    
    Changes in v2:
    
     * <ref> renamed to <rev> throughout. Restrict the commit message and
       advice.shallowHistory documentation to <rev>~N, since <rev>^N doesn't
       get deeper with a higher index the way <rev>~N does.
     * Bare <remote>/<branch> names are now resolved by reverse mapping
       through the remote's configured refspec (remote_find_tracking),
       instead of assuming branches live at refs/remotes/<remote>/<branch>.
     * Extend the shallow history hint to git log -<n> and git log
       --since=<date>, which previously stopped early at a shallow boundary
       with no indication anything was missing.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2413%2FHaraldNordgren%2Fshallow-history-advice-hint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2413/HaraldNordgren/shallow-history-advice-hint-v2
Pull-Request: https://github.com/git/git/pull/2413

Range-diff vs v1:

 1:  ba481667c9 ! 1:  305d22d33e object-name: explain why <ref>~N fails in a shallow clone
     @@ Metadata
      Author: Harald Nordgren <haraldnordgren@gmail.com>
      
       ## Commit message ##
     -    object-name: explain why <ref>~N fails in a shallow clone
     +    shallow: advise when a walk stops at a shallow boundary
      
     -    Asking for a commit's ancestor with <ref>~N or <ref>^N in a shallow
     -    clone that does not have N commits of history locally fails with a
     -    bare "is not a commit" error, with no indication that the repository
     -    being shallow is the reason, or what to do about it.
     +    Asking for a commit's ancestor with <rev>~N in a shallow clone that
     +    does not have N commits of history locally fails with a bare "is not
     +    a commit" error, with no indication that the repository being shallow
     +    is the reason, or what to do about it.
      
     -    Add a hint, shown when the walk runs out of parents exactly at a
     -    recorded shallow boundary, explaining that history was intentionally
     -    truncated there. When <ref> looks like <remote>/<branch> and <remote>
     -    is configured, the suggested command names that remote and branch
     -    directly. For <ref>~N it suggests the exact --deepen needed,
     -    accounting for any history already present instead of just N. For
     -    <ref>^N the suggestion is always --deepen=1, regardless of N: a
     -    shallow boundary commit has no parents recorded locally at all, so
     -    deepening by one generation fetches its complete real parent list in
     -    one step, whether that commit turns out to have one parent or several.
     -    The hint only fires when the search stops at an actual shallow
     -    boundary, not merely because the repository happens to be shallow
     -    elsewhere, so it does not misfire on a short history that is not
     -    shallow-truncated.
     +    git log -<n> and git log --since=<date> have the opposite problem.
     +    Instead of failing, they silently stop once they reach the shallow
     +    boundary, with no error and nothing to suggest that more history
     +    exists upstream and simply was not fetched.
     +
     +    Add a hint, shown when a walk runs out of parents exactly at a
     +    recorded shallow boundary, not merely because the repository happens
     +    to be shallow elsewhere. When the revision <rev> is given as a
     +    remote-tracking branch, the remote and branch are exactly named in the
     +    suggested command, found by reverse mapping through the remote's real
     +    refspec instead of assuming a refs/remotes/<remote>/ layout, since a
     +    refspec can put branches anywhere. For <rev>~N it suggests the exact
     +    --deepen needed, accounting for history already present. For <rev>^N
     +    it always suggests --deepen=1: a shallow boundary commit has no
     +    parents recorded at all, so one more generation reveals its complete
     +    real parent list regardless of which index was asked for.
     +
     +    The hint only suggests that older history might exist. A recorded
     +    shallow boundary does not distinguish a depth cutoff from a commit
     +    that is the true root: cloning with a depth that happens to exactly
     +    match the available history still records that root commit as a
     +    boundary, and only the remote knows for certain whether deepening
     +    would fetch anything new.
     +
     +    git log -<n> and git log --since only show the hint when the last
     +    commit they printed is that same recorded boundary. A plain git log
     +    running out of history, or a git log -<n> or --since that is satisfied
     +    well before reaching the boundary, stays silent as before. When the
     +    output is paginated, the hint waits for the pager to close before
     +    printing, since a full screen pager like less otherwise wipes it from
     +    the terminal along with the rest of the output the moment it quits.
      
          The advice is threaded through GET_OID_QUIETLY so it is not shown
          during the internal re-resolution some commands do while building a
     @@ Documentation/config/advice.adoc: all advice messages.
       	sequencerInUse::
       		Shown when a sequencer command is already in progress.
      +	shallowHistory::
     -+		Shown when `~<n>` or `^<n>` cannot resolve enough ancestors
     -+		because history stops at a shallow boundary, to suggest
     -+		fetching more history.
     ++		Shown when `<rev>~<n>` cannot resolve enough ancestors of
     ++		`<rev>` because history stops at a shallow boundary, to
     ++		suggest fetching more history.
       	skippedCherryPicks::
       		Shown when linkgit:git-rebase[1] skips a commit that has already
       		been cherry-picked onto the upstream branch.
     @@ advice.h: enum advice_type {
       	ADVICE_SPARSE_INDEX_EXPANDED,
       	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
      
     + ## builtin/log.c ##
     +@@
     + #include "commit-reach.h"
     + #include "promisor-remote.h"
     + #include "range-diff.h"
     ++#include "shallow.h"
     + #include "tmp-objdir.h"
     + #include "tree.h"
     + #include "userdiff.h"
     +@@ builtin/log.c: static void cmd_log_init(int argc, const char **argv, const char *prefix,
     + 	cmd_log_init_finish(argc, argv, prefix, rev, opt, cfg);
     + }
     + 
     ++static void advise_if_log_stopped_at_shallow_boundary(struct rev_info *rev,
     ++						       struct commit *last_shown)
     ++{
     ++	if (!last_shown)
     ++		return;
     ++	/* a plain "git log" running out of history is expected */
     ++	if (rev->max_count < 0 && rev->max_age == (timestamp_t)-1)
     ++		return;
     ++	if (!is_repository_shallow(the_repository))
     ++		return;
     ++	if (!commit_is_shallow_boundary(the_repository, &last_shown->object.oid))
     ++		return;
     ++	wait_for_pager();
     ++	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
     ++			   _("'%s' stopped at %s because this repository is a shallow\n"
     ++			     "clone, and might have more history upstream that was never fetched."),
     ++			   "git log",
     ++			   repo_find_unique_abbrev(the_repository,
     ++						    &last_shown->object.oid,
     ++						    DEFAULT_ABBREV));
     ++}
     ++
     + static int cmd_log_walk_no_free(struct rev_info *rev)
     + {
     + 	struct commit *commit;
     ++	struct commit *last_shown = NULL;
     + 	int saved_nrl = 0;
     + 	int saved_dcctc = 0;
     + 	int result;
     +@@ builtin/log.c: static int cmd_log_walk_no_free(struct rev_info *rev)
     + 	 * retain that state information if replacing rev->diffopt in this loop
     + 	 */
     + 	while ((commit = get_revision(rev)) != NULL) {
     ++		last_shown = commit;
     + 		if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
     + 			/*
     + 			 * We decremented max_count in get_revision,
     +@@ builtin/log.c: static int cmd_log_walk_no_free(struct rev_info *rev)
     + 		if (rev->diffopt.degraded_cc_to_c)
     + 			saved_dcctc = 1;
     + 	}
     ++	advise_if_log_stopped_at_shallow_boundary(rev, last_shown);
     + 	rev->diffopt.degraded_cc_to_c = saved_dcctc;
     + 	rev->diffopt.needed_rename_limit = saved_nrl;
     + 
     +
       ## object-name.c ##
      @@
       #include "repo-settings.h"
     @@ object-name.c: static int get_oid_basic(struct repository *r, const char *str, i
       	return 0;
       }
       
     ++struct remote_tracking_search {
     ++	char *dst;
     ++	char *remote_name;
     ++	char *branch_name;
     ++};
     ++
     ++static int search_remote_tracking(struct remote *remote, void *cb_data)
     ++{
     ++	struct remote_tracking_search *search = cb_data;
     ++	struct refspec_item query;
     ++
     ++	memset(&query, 0, sizeof(query));
     ++	query.dst = search->dst;
     ++	if (remote_find_tracking(remote, &query))
     ++		return 0;
     ++	search->remote_name = xstrdup(remote->name);
     ++	search->branch_name = query.src;
     ++	return 1;
     ++}
     ++
     ++/*
     ++ * If "name" resolves to a remote-tracking ref, find which configured
     ++ * remote it actually belongs to and what branch of that remote's it is,
     ++ * by reverse mapping through each remote's real fetch refspec instead
     ++ * of assuming the common "refs/remotes/<remote>/<branch>" layout,
     ++ * since a remote's refspec need not put its branches there at all. On a
     ++ * match, fill "remote_out" and "branch_out" with newly allocated copies
     ++ * and return true. Otherwise leave them untouched and return false.
     ++ */
     ++static bool parse_remote_tracking_name(struct repository *r,
     ++				       const char *name, int namelen,
     ++				       char **remote_out, char **branch_out)
     ++{
     ++	struct object_id oid;
     ++	char *real_ref = NULL;
     ++	struct remote_tracking_search search = { 0 };
     ++	const char *branch_name;
     ++
     ++	if (!repo_dwim_ref(r, name, namelen, &oid, &real_ref, 0) || !real_ref)
     ++		return false;
     ++
     ++	search.dst = real_ref;
     ++	if (!for_each_remote(search_remote_tracking, &search)) {
     ++		free(real_ref);
     ++		return false;
     ++	}
     ++	free(real_ref);
     ++
     ++	branch_name = search.branch_name;
     ++	skip_prefix(branch_name, "refs/heads/", &branch_name);
     ++	*remote_out = search.remote_name;
     ++	*branch_out = xstrdup(branch_name);
     ++	free(search.branch_name);
     ++	return true;
     ++}
     ++
      +/*
      + * When a "name~<n>" or "name^<n>" walk runs out of parents at "commit",
      + * and that is because "commit" is where this shallow repository's history
     -+ * was cut off (rather than commit genuinely being a root commit), let the
     ++ * was cut off (rather than commit being a real root commit), let the
      + * user know that fetching more history might be what they are after.
      + *
      + * "suggested_depth" is the --deepen value to recommend. For "name^<n>"
      + * this is always 1: deepening by one generation fetches "commit"'s real
      + * parent list in full, whatever it turns out to contain, regardless of
     -+ * which parent index <n> asked for. If "name" looks like
     -+ * "<remote>/<branch>" and <remote> is a configured remote, the suggested
     -+ * command names that remote and branch instead of leaving them as
     -+ * placeholders.
     ++ * which parent index <n> asked for.
      + */
      +static void advise_if_shallow_cutoff(struct repository *r,
      +				     const char *name, int namelen,
     @@ object-name.c: static int get_oid_basic(struct repository *r, const char *str, i
      +				     unsigned lookup_flags,
      +				     int suggested_depth)
      +{
     -+	struct commit_graft *graft;
     -+	const char *slash;
     ++	char *remote = NULL, *branch = NULL;
      +	struct strbuf cmd = STRBUF_INIT;
      +
      +	if (lookup_flags & GET_OID_QUIETLY)
      +		return;
      +	if (!is_repository_shallow(r))
      +		return;
     -+	graft = lookup_commit_graft(r, &commit->object.oid);
     -+	if (!graft || graft->nr_parent != -1)
     ++	if (!commit_is_shallow_boundary(r, &commit->object.oid))
      +		return;
      +
     -+	slash = memchr(name, '/', namelen);
     -+	if (slash) {
     -+		char *remote_candidate = xstrndup(name, slash - name);
     -+		if (remote_is_configured(remote_get(remote_candidate), 0))
     -+			strbuf_addf(&cmd, "git fetch --deepen=%d %s %.*s",
     -+				    suggested_depth, remote_candidate,
     -+				    (int)(name + namelen - (slash + 1)), slash + 1);
     -+		free(remote_candidate);
     -+	}
     -+	if (!cmd.len)
     ++	if (parse_remote_tracking_name(r, name, namelen, &remote, &branch))
     ++		strbuf_addf(&cmd, "git fetch --deepen=%d %s %s",
     ++			    suggested_depth, remote, branch);
     ++	else
      +		strbuf_addf(&cmd, "git fetch --deepen=%d <remote> <branch>",
      +			    suggested_depth);
     ++	free(remote);
     ++	free(branch);
      +
      +	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
      +			   _("'%.*s' does not have that many ancestors locally.\n"
      +			     "History stops at %s because this repository is a\n"
     -+			     "shallow clone. To fetch more of it, try:\n"
     ++			     "shallow clone, and might have more history upstream.\n"
     ++			     "To check, try:\n"
      +			     "\n"
      +			     "  %s"),
     -+			   namelen, name, oid_to_hex(&commit->object.oid), cmd.buf);
     ++			   namelen, name,
     ++			   repo_find_unique_abbrev(r, &commit->object.oid, DEFAULT_ABBREV),
     ++			   cmd.buf);
      +	strbuf_release(&cmd);
      +}
      +
     @@ object-name.c: static enum get_oid_result get_nth_ancestor(struct repository *r,
       	while (generation--) {
      -		if (repo_parse_commit(r, commit) || !commit->parents)
      +		if (repo_parse_commit(r, commit))
     - 			return MISSING_OBJECT;
     ++			return MISSING_OBJECT;
      +		if (!commit->parents) {
      +			/* Remaining "generation" plus this failed step is the actual gap. */
      +			advise_if_shallow_cutoff(r, name, len, commit,
      +						 lookup_flags, generation + 1);
     -+			return MISSING_OBJECT;
     + 			return MISSING_OBJECT;
      +		}
       		commit = commit->parents->item;
       	}
     @@ object-name.c: static enum get_oid_result get_oid_1(struct repository *r,
       
       	ret = peel_onion(r, name, len, oid, lookup_flags);
      
     + ## shallow.c ##
     +@@ shallow.c: int is_repository_shallow(struct repository *r)
     + 	return r->parsed_objects->is_shallow;
     + }
     + 
     ++int commit_is_shallow_boundary(struct repository *r,
     ++			       const struct object_id *oid)
     ++{
     ++	struct commit_graft *graft = lookup_commit_graft(r, oid);
     ++
     ++	return graft && graft->nr_parent == -1;
     ++}
     ++
     + static void reset_repository_shallow(struct repository *r)
     + {
     + 	r->parsed_objects->is_shallow = -1;
     +
     + ## shallow.h ##
     +@@ shallow.h: int register_shallow(struct repository *r, const struct object_id *oid);
     + int unregister_shallow(const struct object_id *oid);
     + int is_repository_shallow(struct repository *r);
     + 
     ++/*
     ++ * True if "oid" is where this shallow repository's history was cut off,
     ++ * i.e. a shallow-graft commit with no parents recorded locally, as
     ++ * opposed to a commit that is actually a root.
     ++ */
     ++int commit_is_shallow_boundary(struct repository *r,
     ++			       const struct object_id *oid);
     ++
     + /*
     +  * Lock for updating the $GIT_DIR/shallow file.
     +  *
     +
       ## t/t1500-rev-parse.sh ##
      @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '
       	test_cmp expect actual
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
      +	cat >expect <<-EOF &&
      +	hint: '$name' does not have that many ancestors locally.
      +	hint: History stops at $oid because this repository is a
     -+	hint: shallow clone. To fetch more of it, try:
     ++	hint: shallow clone, and might have more history upstream.
     ++	hint: To check, try:
      +	hint:
      +	hint:   $cmd
      +	hint: Disable this message with "git config set advice.shallowHistory false"
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
      +	git clone --no-local --depth=1 --branch main --single-branch \
      +		.git shallow-advice &&
      +	test_when_finished "rm -rf shallow-advice" &&
     -+	oid=$(git -C shallow-advice rev-parse origin/main) &&
     ++	oid=$(git -C shallow-advice rev-parse --short origin/main) &&
      +	test_must_fail git -C shallow-advice rev-parse origin/main~1 2>err &&
      +	check_shallow_history_advice origin/main "$oid" \
      +		"git fetch --deepen=1 origin main"
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
      +	test_when_finished "rm -rf shallow-advice-partial" &&
      +	(
      +		cd shallow-advice-partial &&
     -+		oid=$(git rev-parse origin/main~2) &&
     ++		oid=$(git rev-parse --short origin/main~2) &&
      +		test_must_fail git rev-parse origin/main~5 2>err &&
      +		check_shallow_history_advice origin/main "$oid" \
      +			"git fetch --deepen=3 origin main" &&
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
      +	git clone --no-local --depth=1 --branch main --single-branch \
      +		.git shallow-advice-caret &&
      +	test_when_finished "rm -rf shallow-advice-caret" &&
     -+	oid=$(git -C shallow-advice-caret rev-parse origin/main) &&
     ++	oid=$(git -C shallow-advice-caret rev-parse --short origin/main) &&
      +	test_must_fail git -C shallow-advice-caret rev-parse origin/main^1 2>err &&
      +	check_shallow_history_advice origin/main "$oid" \
      +		"git fetch --deepen=1 origin main"
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
      +	test_when_finished "rm -rf shallow-advice-merge" &&
      +	(
      +		cd shallow-advice-merge &&
     -+		oid=$(git rev-parse origin/main) &&
     ++		oid=$(git rev-parse --short origin/main) &&
      +		test_must_fail git rev-parse origin/main^2 2>err &&
      +		check_shallow_history_advice origin/main "$oid" \
      +			"git fetch --deepen=1 origin main" &&
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
       test_expect_success 'rev-parse --show-object-format in repo' '
       	test_oid algo >expect &&
       	git rev-parse --show-object-format >actual &&
     +
     + ## t/t4202-log.sh ##
     +@@ t/t4202-log.sh: test_expect_success 'log --invert-grep --grep --author' '
     + 	test_cmp expect actual
     + '
     + 
     ++check_log_shallow_history_advice () {
     ++	oid=$1 &&
     ++	grep '^hint:' err >actual &&
     ++	cat >expect <<-EOF &&
     ++	hint: 'git log' stopped at $oid because this repository is a shallow
     ++	hint: clone, and might have more history upstream that was never fetched.
     ++	hint: Disable this message with "git config set advice.shallowHistory false"
     ++	EOF
     ++	test_cmp expect actual
     ++}
     ++
     ++test_expect_success 'set up linear history for shallow log advice tests' '
     ++	git checkout --orphan loghist &&
     ++	test_commit loghist_1 &&
     ++	test_commit loghist_2 &&
     ++	test_commit loghist_3 &&
     ++	test_commit loghist_4 &&
     ++	test_commit loghist_5 &&
     ++	git checkout main
     ++'
     ++
     ++test_expect_success 'log -<n> advises when it runs out of history at a shallow boundary' '
     ++	git clone --no-local --depth=2 --branch loghist --single-branch \
     ++		.git shallow-log-advice &&
     ++	test_when_finished "rm -rf shallow-log-advice" &&
     ++	oid=$(git -C shallow-log-advice rev-parse --short origin/loghist~1) &&
     ++	git -C shallow-log-advice log -5 --oneline origin/loghist >out 2>err &&
     ++	check_log_shallow_history_advice "$oid"
     ++'
     ++
     ++test_expect_success 'log --since advises when it runs out of history at a shallow boundary' '
     ++	git clone --no-local --depth=2 --branch loghist --single-branch \
     ++		.git shallow-log-advice-since &&
     ++	test_when_finished "rm -rf shallow-log-advice-since" &&
     ++	oid=$(git -C shallow-log-advice-since rev-parse --short origin/loghist~1) &&
     ++	git -C shallow-log-advice-since log --since=2000-01-01 --oneline \
     ++		origin/loghist >out 2>err &&
     ++	check_log_shallow_history_advice "$oid"
     ++'
     ++
     ++test_expect_success 'log without -<n> or --since does not advise at a shallow boundary' '
     ++	git clone --no-local --depth=1 --branch loghist --single-branch \
     ++		.git shallow-log-plain &&
     ++	test_when_finished "rm -rf shallow-log-plain" &&
     ++	git -C shallow-log-plain log --oneline origin/loghist >out 2>err &&
     ++	test_grep ! "^hint:" err
     ++'
     ++
     ++test_expect_success 'log -<n> does not advise when satisfied within local history' '
     ++	git clone --no-local --depth=5 --branch loghist --single-branch \
     ++		.git shallow-log-satisfied &&
     ++	test_when_finished "rm -rf shallow-log-satisfied" &&
     ++	git -C shallow-log-satisfied log -2 --oneline origin/loghist >out 2>err &&
     ++	test_grep ! "^hint:" err
     ++'
     ++
     ++test_expect_success 'log -<n> shallow history advice can be disabled' '
     ++	git clone --no-local --depth=1 --branch loghist --single-branch \
     ++		.git shallow-log-off &&
     ++	test_when_finished "rm -rf shallow-log-off" &&
     ++	git -C shallow-log-off -c advice.shallowHistory=false \
     ++		log -5 --oneline origin/loghist >out 2>err &&
     ++	test_grep ! "^hint:" err
     ++'
     ++
     ++test_expect_success 'log -<n> does not advise in a non-shallow repository' '
     ++	test_when_finished "rm -rf non-shallow-log" &&
     ++	git init -q non-shallow-log &&
     ++	test_commit -C non-shallow-log only-commit &&
     ++	git -C non-shallow-log log -20 --oneline >out 2>err &&
     ++	test_grep ! "^hint:" err
     ++'
     ++
     + test_done


 Documentation/config/advice.adoc |   4 +
 advice.c                         |   1 +
 advice.h                         |   1 +
 builtin/log.c                    |  26 +++++++
 object-name.c                    | 125 +++++++++++++++++++++++++++++--
 shallow.c                        |   8 ++
 shallow.h                        |   8 ++
 t/t1500-rev-parse.sh             | 109 +++++++++++++++++++++++++++
 t/t4202-log.sh                   |  73 ++++++++++++++++++
 9 files changed, 350 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
index 81f80a9274..bb1e508bb6 100644
--- a/Documentation/config/advice.adoc
+++ b/Documentation/config/advice.adoc
@@ -128,6 +128,10 @@ all advice messages.
 		give directions on how to proceed from the current state.
 	sequencerInUse::
 		Shown when a sequencer command is already in progress.
+	shallowHistory::
+		Shown when `<rev>~<n>` cannot resolve enough ancestors of
+		`<rev>` because history stops at a shallow boundary, to
+		suggest fetching more history.
 	skippedCherryPicks::
 		Shown when linkgit:git-rebase[1] skips a commit that has already
 		been cherry-picked onto the upstream branch.
diff --git a/advice.c b/advice.c
index 63bf8b0c5f..e22706ead5 100644
--- a/advice.c
+++ b/advice.c
@@ -80,6 +80,7 @@ static struct {
 	[ADVICE_RM_HINTS]				= { "rmHints" },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
+	[ADVICE_SHALLOW_HISTORY]			= { "shallowHistory" },
 	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
 	[ADVICE_SPARSE_INDEX_EXPANDED]			= { "sparseIndexExpanded" },
 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
diff --git a/advice.h b/advice.h
index 66f6cd6a77..e80e3e82cc 100644
--- a/advice.h
+++ b/advice.h
@@ -47,6 +47,7 @@ enum advice_type {
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,
 	ADVICE_SET_UPSTREAM_FAILURE,
+	ADVICE_SHALLOW_HISTORY,
 	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_SPARSE_INDEX_EXPANDED,
 	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
diff --git a/builtin/log.c b/builtin/log.c
index 350b35c556..22a40c7d28 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -47,6 +47,7 @@
 #include "commit-reach.h"
 #include "promisor-remote.h"
 #include "range-diff.h"
+#include "shallow.h"
 #include "tmp-objdir.h"
 #include "tree.h"
 #include "userdiff.h"
@@ -396,9 +397,32 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	cmd_log_init_finish(argc, argv, prefix, rev, opt, cfg);
 }
 
+static void advise_if_log_stopped_at_shallow_boundary(struct rev_info *rev,
+						       struct commit *last_shown)
+{
+	if (!last_shown)
+		return;
+	/* a plain "git log" running out of history is expected */
+	if (rev->max_count < 0 && rev->max_age == (timestamp_t)-1)
+		return;
+	if (!is_repository_shallow(the_repository))
+		return;
+	if (!commit_is_shallow_boundary(the_repository, &last_shown->object.oid))
+		return;
+	wait_for_pager();
+	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
+			   _("'%s' stopped at %s because this repository is a shallow\n"
+			     "clone, and might have more history upstream that was never fetched."),
+			   "git log",
+			   repo_find_unique_abbrev(the_repository,
+						    &last_shown->object.oid,
+						    DEFAULT_ABBREV));
+}
+
 static int cmd_log_walk_no_free(struct rev_info *rev)
 {
 	struct commit *commit;
+	struct commit *last_shown = NULL;
 	int saved_nrl = 0;
 	int saved_dcctc = 0;
 	int result;
@@ -412,6 +436,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
 	 * retain that state information if replacing rev->diffopt in this loop
 	 */
 	while ((commit = get_revision(rev)) != NULL) {
+		last_shown = commit;
 		if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
 			/*
 			 * We decremented max_count in get_revision,
@@ -437,6 +462,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
 		if (rev->diffopt.degraded_cc_to_c)
 			saved_dcctc = 1;
 	}
+	advise_if_log_stopped_at_shallow_boundary(rev, last_shown);
 	rev->diffopt.degraded_cc_to_c = saved_dcctc;
 	rev->diffopt.needed_rename_limit = saved_nrl;
 
diff --git a/object-name.c b/object-name.c
index 4eda8c8eac..cb601e1532 100644
--- a/object-name.c
+++ b/object-name.c
@@ -22,6 +22,7 @@
 #include "repo-settings.h"
 #include "repository.h"
 #include "setup.h"
+#include "shallow.h"
 #include "midx.h"
 #include "commit-reach.h"
 #include "date.h"
@@ -824,9 +825,115 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 	return 0;
 }
 
+struct remote_tracking_search {
+	char *dst;
+	char *remote_name;
+	char *branch_name;
+};
+
+static int search_remote_tracking(struct remote *remote, void *cb_data)
+{
+	struct remote_tracking_search *search = cb_data;
+	struct refspec_item query;
+
+	memset(&query, 0, sizeof(query));
+	query.dst = search->dst;
+	if (remote_find_tracking(remote, &query))
+		return 0;
+	search->remote_name = xstrdup(remote->name);
+	search->branch_name = query.src;
+	return 1;
+}
+
+/*
+ * If "name" resolves to a remote-tracking ref, find which configured
+ * remote it actually belongs to and what branch of that remote's it is,
+ * by reverse mapping through each remote's real fetch refspec instead
+ * of assuming the common "refs/remotes/<remote>/<branch>" layout,
+ * since a remote's refspec need not put its branches there at all. On a
+ * match, fill "remote_out" and "branch_out" with newly allocated copies
+ * and return true. Otherwise leave them untouched and return false.
+ */
+static bool parse_remote_tracking_name(struct repository *r,
+				       const char *name, int namelen,
+				       char **remote_out, char **branch_out)
+{
+	struct object_id oid;
+	char *real_ref = NULL;
+	struct remote_tracking_search search = { 0 };
+	const char *branch_name;
+
+	if (!repo_dwim_ref(r, name, namelen, &oid, &real_ref, 0) || !real_ref)
+		return false;
+
+	search.dst = real_ref;
+	if (!for_each_remote(search_remote_tracking, &search)) {
+		free(real_ref);
+		return false;
+	}
+	free(real_ref);
+
+	branch_name = search.branch_name;
+	skip_prefix(branch_name, "refs/heads/", &branch_name);
+	*remote_out = search.remote_name;
+	*branch_out = xstrdup(branch_name);
+	free(search.branch_name);
+	return true;
+}
+
+/*
+ * When a "name~<n>" or "name^<n>" walk runs out of parents at "commit",
+ * and that is because "commit" is where this shallow repository's history
+ * was cut off (rather than commit being a real root commit), let the
+ * user know that fetching more history might be what they are after.
+ *
+ * "suggested_depth" is the --deepen value to recommend. For "name^<n>"
+ * this is always 1: deepening by one generation fetches "commit"'s real
+ * parent list in full, whatever it turns out to contain, regardless of
+ * which parent index <n> asked for.
+ */
+static void advise_if_shallow_cutoff(struct repository *r,
+				     const char *name, int namelen,
+				     struct commit *commit,
+				     unsigned lookup_flags,
+				     int suggested_depth)
+{
+	char *remote = NULL, *branch = NULL;
+	struct strbuf cmd = STRBUF_INIT;
+
+	if (lookup_flags & GET_OID_QUIETLY)
+		return;
+	if (!is_repository_shallow(r))
+		return;
+	if (!commit_is_shallow_boundary(r, &commit->object.oid))
+		return;
+
+	if (parse_remote_tracking_name(r, name, namelen, &remote, &branch))
+		strbuf_addf(&cmd, "git fetch --deepen=%d %s %s",
+			    suggested_depth, remote, branch);
+	else
+		strbuf_addf(&cmd, "git fetch --deepen=%d <remote> <branch>",
+			    suggested_depth);
+	free(remote);
+	free(branch);
+
+	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
+			   _("'%.*s' does not have that many ancestors locally.\n"
+			     "History stops at %s because this repository is a\n"
+			     "shallow clone, and might have more history upstream.\n"
+			     "To check, try:\n"
+			     "\n"
+			     "  %s"),
+			   namelen, name,
+			   repo_find_unique_abbrev(r, &commit->object.oid, DEFAULT_ABBREV),
+			   cmd.buf);
+	strbuf_release(&cmd);
+}
+
 static enum get_oid_result get_parent(struct repository *r,
 				      const char *name, int len,
-				      struct object_id *result, int idx)
+				      struct object_id *result, int idx,
+				      unsigned lookup_flags)
 {
 	struct object_id oid;
 	enum get_oid_result ret = get_oid_1(r, name, len, &oid,
@@ -851,13 +958,15 @@ static enum get_oid_result get_parent(struct repository *r,
 		}
 		p = p->next;
 	}
+	advise_if_shallow_cutoff(r, name, len, commit, lookup_flags, 1);
 	return MISSING_OBJECT;
 }
 
 static enum get_oid_result get_nth_ancestor(struct repository *r,
 					    const char *name, int len,
 					    struct object_id *result,
-					    int generation)
+					    int generation,
+					    unsigned lookup_flags)
 {
 	struct object_id oid;
 	struct commit *commit;
@@ -871,8 +980,14 @@ static enum get_oid_result get_nth_ancestor(struct repository *r,
 		return MISSING_OBJECT;
 
 	while (generation--) {
-		if (repo_parse_commit(r, commit) || !commit->parents)
+		if (repo_parse_commit(r, commit))
+			return MISSING_OBJECT;
+		if (!commit->parents) {
+			/* Remaining "generation" plus this failed step is the actual gap. */
+			advise_if_shallow_cutoff(r, name, len, commit,
+						 lookup_flags, generation + 1);
 			return MISSING_OBJECT;
+		}
 		commit = commit->parents->item;
 	}
 	oidcpy(result, &commit->object.oid);
@@ -1119,9 +1234,9 @@ static enum get_oid_result get_oid_1(struct repository *r,
 		else if (num > INT_MAX)
 			return MISSING_OBJECT;
 		if (has_suffix == '^')
-			return get_parent(r, name, len1, oid, num);
+			return get_parent(r, name, len1, oid, num, lookup_flags);
 		/* else if (has_suffix == '~') -- goes without saying */
-		return get_nth_ancestor(r, name, len1, oid, num);
+		return get_nth_ancestor(r, name, len1, oid, num, lookup_flags);
 	}
 
 	ret = peel_onion(r, name, len, oid, lookup_flags);
diff --git a/shallow.c b/shallow.c
index 8e244a5669..2e0cf09f32 100644
--- a/shallow.c
+++ b/shallow.c
@@ -92,6 +92,14 @@ int is_repository_shallow(struct repository *r)
 	return r->parsed_objects->is_shallow;
 }
 
+int commit_is_shallow_boundary(struct repository *r,
+			       const struct object_id *oid)
+{
+	struct commit_graft *graft = lookup_commit_graft(r, oid);
+
+	return graft && graft->nr_parent == -1;
+}
+
 static void reset_repository_shallow(struct repository *r)
 {
 	r->parsed_objects->is_shallow = -1;
diff --git a/shallow.h b/shallow.h
index 6a64db42c9..66c9e723f7 100644
--- a/shallow.h
+++ b/shallow.h
@@ -15,6 +15,14 @@ int register_shallow(struct repository *r, const struct object_id *oid);
 int unregister_shallow(const struct object_id *oid);
 int is_repository_shallow(struct repository *r);
 
+/*
+ * True if "oid" is where this shallow repository's history was cut off,
+ * i.e. a shallow-graft commit with no parents recorded locally, as
+ * opposed to a commit that is actually a root.
+ */
+int commit_is_shallow_boundary(struct repository *r,
+			       const struct object_id *oid);
+
 /*
  * Lock for updating the $GIT_DIR/shallow file.
  *
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 4174ca40c3..ecadeddcdb 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -193,6 +193,115 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '
 	test_cmp expect actual
 '
 
+check_shallow_history_advice () {
+	name=$1 oid=$2 cmd=$3 &&
+	grep '^hint:' err >actual &&
+	cat >expect <<-EOF &&
+	hint: '$name' does not have that many ancestors locally.
+	hint: History stops at $oid because this repository is a
+	hint: shallow clone, and might have more history upstream.
+	hint: To check, try:
+	hint:
+	hint:   $cmd
+	hint: Disable this message with "git config set advice.shallowHistory false"
+	EOF
+	test_cmp expect actual
+}
+
+test_expect_success 'shallowHistory advice on ~N beyond shallow boundary' '
+	test_commit shallow_advice_1 &&
+	test_commit shallow_advice_2 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice &&
+	test_when_finished "rm -rf shallow-advice" &&
+	oid=$(git -C shallow-advice rev-parse --short origin/main) &&
+	test_must_fail git -C shallow-advice rev-parse origin/main~1 2>err &&
+	check_shallow_history_advice origin/main "$oid" \
+		"git fetch --deepen=1 origin main"
+'
+
+test_expect_success 'shallowHistory advice accounts for depth already present' '
+	test_commit shallow_partial_1 &&
+	test_commit shallow_partial_2 &&
+	test_commit shallow_partial_3 &&
+	test_commit shallow_partial_4 &&
+	test_commit shallow_partial_5 &&
+	test_commit shallow_partial_6 &&
+	git clone --no-local --depth=3 --branch main --single-branch \
+		.git shallow-advice-partial &&
+	test_when_finished "rm -rf shallow-advice-partial" &&
+	(
+		cd shallow-advice-partial &&
+		oid=$(git rev-parse --short origin/main~2) &&
+		test_must_fail git rev-parse origin/main~5 2>err &&
+		check_shallow_history_advice origin/main "$oid" \
+			"git fetch --deepen=3 origin main" &&
+		git fetch --deepen=3 origin &&
+		git rev-parse origin/main~5 &&
+		test_must_fail git rev-parse origin/main~6
+	)
+'
+
+test_expect_success 'shallowHistory advice on ^N (first parent) beyond shallow boundary' '
+	test_commit shallow_caret_1 &&
+	test_commit shallow_caret_2 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-caret &&
+	test_when_finished "rm -rf shallow-advice-caret" &&
+	oid=$(git -C shallow-advice-caret rev-parse --short origin/main) &&
+	test_must_fail git -C shallow-advice-caret rev-parse origin/main^1 2>err &&
+	check_shallow_history_advice origin/main "$oid" \
+		"git fetch --deepen=1 origin main"
+'
+
+test_expect_success 'shallowHistory advice on ^N suggests deepen=1 even for a merge parent' '
+	test_commit shallow_merge_base &&
+	git checkout -q -b shallow-merge-side &&
+	test_commit shallow_merge_side1 &&
+	git checkout -q main &&
+	test_commit shallow_merge_main1 &&
+	git merge -q --no-ff shallow-merge-side -m "shallow merge commit" &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-merge &&
+	test_when_finished "rm -rf shallow-advice-merge" &&
+	(
+		cd shallow-advice-merge &&
+		oid=$(git rev-parse --short origin/main) &&
+		test_must_fail git rev-parse origin/main^2 2>err &&
+		check_shallow_history_advice origin/main "$oid" \
+			"git fetch --deepen=1 origin main" &&
+		git fetch -q --deepen=1 origin &&
+		git rev-parse origin/main^1 &&
+		git rev-parse origin/main^2
+	)
+'
+
+test_expect_success 'shallowHistory advice can be disabled' '
+	test_commit shallow_off_1 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-off &&
+	test_when_finished "rm -rf shallow-advice-off" &&
+	test_must_fail git -C shallow-advice-off \
+		-c advice.shallowHistory=false rev-parse origin/main~1 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'shallowHistory advice not shown for a non-shallow repository' '
+	test_must_fail git rev-parse HEAD~100000 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'shallowHistory advice not shown when resolution succeeds' '
+	test_commit shallow_ok_1 &&
+	test_commit shallow_ok_2 &&
+	test_commit shallow_ok_3 &&
+	git clone --no-local --depth=3 --branch main --single-branch \
+		.git shallow-advice-ok &&
+	test_when_finished "rm -rf shallow-advice-ok" &&
+	git -C shallow-advice-ok rev-parse origin/main~1 >actual 2>err &&
+	test_grep ! "^hint:" err
+'
+
 test_expect_success 'rev-parse --show-object-format in repo' '
 	test_oid algo >expect &&
 	git rev-parse --show-object-format >actual &&
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 06552a6271..ad22934c9d 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -2452,4 +2452,77 @@ test_expect_success 'log --invert-grep --grep --author' '
 	test_cmp expect actual
 '
 
+check_log_shallow_history_advice () {
+	oid=$1 &&
+	grep '^hint:' err >actual &&
+	cat >expect <<-EOF &&
+	hint: 'git log' stopped at $oid because this repository is a shallow
+	hint: clone, and might have more history upstream that was never fetched.
+	hint: Disable this message with "git config set advice.shallowHistory false"
+	EOF
+	test_cmp expect actual
+}
+
+test_expect_success 'set up linear history for shallow log advice tests' '
+	git checkout --orphan loghist &&
+	test_commit loghist_1 &&
+	test_commit loghist_2 &&
+	test_commit loghist_3 &&
+	test_commit loghist_4 &&
+	test_commit loghist_5 &&
+	git checkout main
+'
+
+test_expect_success 'log -<n> advises when it runs out of history at a shallow boundary' '
+	git clone --no-local --depth=2 --branch loghist --single-branch \
+		.git shallow-log-advice &&
+	test_when_finished "rm -rf shallow-log-advice" &&
+	oid=$(git -C shallow-log-advice rev-parse --short origin/loghist~1) &&
+	git -C shallow-log-advice log -5 --oneline origin/loghist >out 2>err &&
+	check_log_shallow_history_advice "$oid"
+'
+
+test_expect_success 'log --since advises when it runs out of history at a shallow boundary' '
+	git clone --no-local --depth=2 --branch loghist --single-branch \
+		.git shallow-log-advice-since &&
+	test_when_finished "rm -rf shallow-log-advice-since" &&
+	oid=$(git -C shallow-log-advice-since rev-parse --short origin/loghist~1) &&
+	git -C shallow-log-advice-since log --since=2000-01-01 --oneline \
+		origin/loghist >out 2>err &&
+	check_log_shallow_history_advice "$oid"
+'
+
+test_expect_success 'log without -<n> or --since does not advise at a shallow boundary' '
+	git clone --no-local --depth=1 --branch loghist --single-branch \
+		.git shallow-log-plain &&
+	test_when_finished "rm -rf shallow-log-plain" &&
+	git -C shallow-log-plain log --oneline origin/loghist >out 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'log -<n> does not advise when satisfied within local history' '
+	git clone --no-local --depth=5 --branch loghist --single-branch \
+		.git shallow-log-satisfied &&
+	test_when_finished "rm -rf shallow-log-satisfied" &&
+	git -C shallow-log-satisfied log -2 --oneline origin/loghist >out 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'log -<n> shallow history advice can be disabled' '
+	git clone --no-local --depth=1 --branch loghist --single-branch \
+		.git shallow-log-off &&
+	test_when_finished "rm -rf shallow-log-off" &&
+	git -C shallow-log-off -c advice.shallowHistory=false \
+		log -5 --oneline origin/loghist >out 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'log -<n> does not advise in a non-shallow repository' '
+	test_when_finished "rm -rf non-shallow-log" &&
+	git init -q non-shallow-log &&
+	test_commit -C non-shallow-log only-commit &&
+	git -C non-shallow-log log -20 --oneline >out 2>err &&
+	test_grep ! "^hint:" err
+'
+
 test_done

base-commit: d38352cd43ab9745686d697872408bc3249a153f
-- 
gitgitgadget

Re: [PATCH v2] shallow: advise when a walk stops at a shallow boundary

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index 350b35c556..22a40c7d28 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -47,6 +47,7 @@
>  #include "commit-reach.h"
>  #include "promisor-remote.h"
>  #include "range-diff.h"
> +#include "shallow.h"
>  #include "tmp-objdir.h"
>  #include "tree.h"
>  #include "userdiff.h"
> @@ -396,9 +397,32 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
>  	cmd_log_init_finish(argc, argv, prefix, rev, opt, cfg);
>  }
>  
> +static void advise_if_log_stopped_at_shallow_boundary(struct rev_info *rev,
> +						       struct commit *last_shown)
> +{
> +	if (!last_shown)
> +		return;
> +	/* a plain "git log" running out of history is expected */
> +	if (rev->max_count < 0 && rev->max_age == (timestamp_t)-1)
> +		return;

"git log -999" may run out of commits because the history genuinely
may only have 20 commits, or the clone was made shallowly and we
only happen to have 20 commits at hand.  The same is true for "git
log" that does not get any count.  So I do not quite see the reason
why we want to give an early return in this function.

> +	if (!is_repository_shallow(the_repository))
> +		return;
> +	if (!commit_is_shallow_boundary(the_repository, &last_shown->object.oid))
> +		return;
> +	wait_for_pager();
> +	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
> +			   _("'%s' stopped at %s because this repository is a shallow\n"
> +			     "clone, and might have more history upstream that was never fetched."),
> +			   "git log",
> +			   repo_find_unique_abbrev(the_repository,
> +						    &last_shown->object.oid,
> +						    DEFAULT_ABBREV));
> +}
> +

Anyway, sorry, I regret opening this can of worms X-<.  It is not
that your implementation and design is bad, it is the problem being
solved that is bad.  But ...

>  static int cmd_log_walk_no_free(struct rev_info *rev)
>  {
>  	struct commit *commit;
> +	struct commit *last_shown = NULL;
>  	int saved_nrl = 0;
>  	int saved_dcctc = 0;
>  	int result;
> @@ -412,6 +436,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
>  	 * retain that state information if replacing rev->diffopt in this loop
>  	 */
>  	while ((commit = get_revision(rev)) != NULL) {
> +		last_shown = commit;
>  		if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
>  			/*
>  			 * We decremented max_count in get_revision,
> @@ -437,6 +462,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
>  		if (rev->diffopt.degraded_cc_to_c)
>  			saved_dcctc = 1;
>  	}
> +	advise_if_log_stopped_at_shallow_boundary(rev, last_shown);

... the "last shown" commit may or may not be at shallow boundary.
It may be a normal root commit, yet there may be truncated side
history that we stopped traversing during the above loop.  If for
example we had a history like this (time flows from left to right):

     ()---b---d---e	(side branch)
                   \
                    \
  a---------c--------f------g	(trunk)

where a side branch is much denser than the trunk, and shallow clone
truncated the history, hiding the parents of 'b', we may see that
our traversal goes 'g', 'f', 'e', 'd', 'c', 'b', 'a' and the last
shown commit may be 'a', which is a genuine root commit.  But behind
'b' there may be hundreds of commits on the side branch that
eventually leads down to 'a'.  Doesn't the user want to be notified
that they are missing tons of history behind 'b' in such a case when
'b' is shown and we stop traversing its parents?

That was the original motivation behind the issue I raised during
the previous review, and that is why I say I regret opening this can
of worms.  If the commit 'a' in the history had parentes hidden
behind a shallow boundary (in other words, 'a' is not root), then
from the same traversal, we would see the "traversal stopped at
shallow boundary" advise, which means that we sometimes see it and
sometimes we don't, even though in either case we are showing 'b' as
if it were a root when it is not.

I do not think of a good way to solve this, and showing "your
traversal happened to have ended at the shallow boundary" only
sometimes in an unreliable way is probably counter-productive, I am
afraid.

So please forget what I said in the previous review.  Even though it
may be a good piece of information to have somehow for the user to
know which commit has its parents hidden beyond a shallow boundary,
a regular get_revision() traversal loop is probalby not a good place
to do so.

We might want to show the information by enriching "log --graph"
output but that is totally unrelated to what you are doing with this
<rev>~N topic.

> +test_expect_success 'shallowHistory advice accounts for depth already present' '
> +	test_commit shallow_partial_1 &&
> +	test_commit shallow_partial_2 &&
> +	test_commit shallow_partial_3 &&
> +	test_commit shallow_partial_4 &&
> +	test_commit shallow_partial_5 &&
> +	test_commit shallow_partial_6 &&
> +	git clone --no-local --depth=3 --branch main --single-branch \
> +		.git shallow-advice-partial &&
> +	test_when_finished "rm -rf shallow-advice-partial" &&
> +	(
> +		cd shallow-advice-partial &&
> +		oid=$(git rev-parse --short origin/main~2) &&
> +		test_must_fail git rev-parse origin/main~5 2>err &&
> +		check_shallow_history_advice origin/main "$oid" \
> +			"git fetch --deepen=3 origin main" &&

Would wew see the same output if we asked for "origin/main^^^^^"?
Just being curious.

> +		git fetch --deepen=3 origin &&
> +		git rev-parse origin/main~5 &&
> +		test_must_fail git rev-parse origin/main~6
> +	)
> +'

Thanks, and sorry about the ill-defined feature request.

[PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

Asking for a commit's ancestor with <rev>~N in a shallow clone that
does not have N commits of history locally fails with a bare "is not
a commit" error, with no indication that the repository being shallow
is the reason, or what to do about it.

Add a hint, shown when a walk runs out of parents exactly at a
recorded shallow boundary, not merely because the repository happens
to be shallow elsewhere. When the revision <rev> is given as a
remote-tracking branch, the remote and branch are exactly named in the
suggested command, found by reverse mapping through the remote's real
refspec instead of assuming a refs/remotes/<remote>/ layout, since a
refspec can put branches anywhere. For <rev>~N it suggests the exact
--deepen needed, accounting for history already present. For <rev>^N
it always suggests --deepen=1: a shallow boundary commit has no
parents recorded at all, so one more generation reveals its complete
real parent list regardless of which index was asked for.

The hint only suggests that older history might exist. A recorded
shallow boundary does not distinguish a depth cutoff from a commit
that is the true root: cloning with a depth that happens to exactly
match the available history still records that root commit as a
boundary, and only the remote knows for certain whether deepening
would fetch anything new.

The advice is threaded through GET_OID_QUIETLY so it is not shown
during the internal re-resolution some commands do while building a
better error message, which would otherwise print it twice for the
same failing argument. A chained <rev>^^^ resolves one parent at a
time, and each step re-resolves everything before it through a fresh,
always non-quiet lookup, so a chained caret landing past a shallow
boundary printed the hint twice instead of once. Thread the quiet bit
through that inner lookup too, so it stays quiet exactly when the
outer resolution asked it to.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
    object-name: explain why ~N fails in a shallow clone
    
    Asking for a commit's ancestor with <rev>~N in a shallow clone that
    doesn't have N commits of history locally fails with a "is not a commit"
    error, with no indication that the repository being shallow is the
    reason.
    
    Changes in v3:
    
     * Drop the git log -<n>/--since extension entirely, per feedback that
       last commit shown is an unreliable signal for a shallow boundary.
     * Fix a bug for chained <rev>^^^ where it would print the hint twice.
    
    Changes in v2:
    
     * <ref> renamed to <rev> throughout. Restrict the commit message and
       advice.shallowHistory documentation to <rev>~N, since <rev>^N doesn't
       get deeper with a higher index the way <rev>~N does.
     * Bare <remote>/<branch> names are now resolved by reverse mapping
       through the remote's configured refspec (remote_find_tracking),
       instead of assuming branches live at refs/remotes/<remote>/<branch>.
     * Extend the shallow history hint to git log -<n> and git log
       --since=<date>, which previously stopped early at a shallow boundary
       with no indication anything was missing.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2413%2FHaraldNordgren%2Fshallow-history-advice-hint-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2413/HaraldNordgren/shallow-history-advice-hint-v3
Pull-Request: https://github.com/git/git/pull/2413

Range-diff vs v2:

 1:  305d22d33e ! 1:  d5f2128258 shallow: advise when a walk stops at a shallow boundary
     @@ Metadata
      Author: Harald Nordgren <haraldnordgren@gmail.com>
      
       ## Commit message ##
     -    shallow: advise when a walk stops at a shallow boundary
     +    object-name: explain why <rev>~N fails in a shallow clone
      
          Asking for a commit's ancestor with <rev>~N in a shallow clone that
          does not have N commits of history locally fails with a bare "is not
          a commit" error, with no indication that the repository being shallow
          is the reason, or what to do about it.
      
     -    git log -<n> and git log --since=<date> have the opposite problem.
     -    Instead of failing, they silently stop once they reach the shallow
     -    boundary, with no error and nothing to suggest that more history
     -    exists upstream and simply was not fetched.
     -
          Add a hint, shown when a walk runs out of parents exactly at a
          recorded shallow boundary, not merely because the repository happens
          to be shallow elsewhere. When the revision <rev> is given as a
     @@ Commit message
          boundary, and only the remote knows for certain whether deepening
          would fetch anything new.
      
     -    git log -<n> and git log --since only show the hint when the last
     -    commit they printed is that same recorded boundary. A plain git log
     -    running out of history, or a git log -<n> or --since that is satisfied
     -    well before reaching the boundary, stays silent as before. When the
     -    output is paginated, the hint waits for the pager to close before
     -    printing, since a full screen pager like less otherwise wipes it from
     -    the terminal along with the rest of the output the moment it quits.
     -
          The advice is threaded through GET_OID_QUIETLY so it is not shown
          during the internal re-resolution some commands do while building a
          better error message, which would otherwise print it twice for the
     -    same failing argument.
     +    same failing argument. A chained <rev>^^^ resolves one parent at a
     +    time, and each step re-resolves everything before it through a fresh,
     +    always non-quiet lookup, so a chained caret landing past a shallow
     +    boundary printed the hint twice instead of once. Thread the quiet bit
     +    through that inner lookup too, so it stays quiet exactly when the
     +    outer resolution asked it to.
      
          Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
      
     @@ advice.h: enum advice_type {
       	ADVICE_SPARSE_INDEX_EXPANDED,
       	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
      
     - ## builtin/log.c ##
     -@@
     - #include "commit-reach.h"
     - #include "promisor-remote.h"
     - #include "range-diff.h"
     -+#include "shallow.h"
     - #include "tmp-objdir.h"
     - #include "tree.h"
     - #include "userdiff.h"
     -@@ builtin/log.c: static void cmd_log_init(int argc, const char **argv, const char *prefix,
     - 	cmd_log_init_finish(argc, argv, prefix, rev, opt, cfg);
     - }
     - 
     -+static void advise_if_log_stopped_at_shallow_boundary(struct rev_info *rev,
     -+						       struct commit *last_shown)
     -+{
     -+	if (!last_shown)
     -+		return;
     -+	/* a plain "git log" running out of history is expected */
     -+	if (rev->max_count < 0 && rev->max_age == (timestamp_t)-1)
     -+		return;
     -+	if (!is_repository_shallow(the_repository))
     -+		return;
     -+	if (!commit_is_shallow_boundary(the_repository, &last_shown->object.oid))
     -+		return;
     -+	wait_for_pager();
     -+	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
     -+			   _("'%s' stopped at %s because this repository is a shallow\n"
     -+			     "clone, and might have more history upstream that was never fetched."),
     -+			   "git log",
     -+			   repo_find_unique_abbrev(the_repository,
     -+						    &last_shown->object.oid,
     -+						    DEFAULT_ABBREV));
     -+}
     -+
     - static int cmd_log_walk_no_free(struct rev_info *rev)
     - {
     - 	struct commit *commit;
     -+	struct commit *last_shown = NULL;
     - 	int saved_nrl = 0;
     - 	int saved_dcctc = 0;
     - 	int result;
     -@@ builtin/log.c: static int cmd_log_walk_no_free(struct rev_info *rev)
     - 	 * retain that state information if replacing rev->diffopt in this loop
     - 	 */
     - 	while ((commit = get_revision(rev)) != NULL) {
     -+		last_shown = commit;
     - 		if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
     - 			/*
     - 			 * We decremented max_count in get_revision,
     -@@ builtin/log.c: static int cmd_log_walk_no_free(struct rev_info *rev)
     - 		if (rev->diffopt.degraded_cc_to_c)
     - 			saved_dcctc = 1;
     - 	}
     -+	advise_if_log_stopped_at_shallow_boundary(rev, last_shown);
     - 	rev->diffopt.degraded_cc_to_c = saved_dcctc;
     - 	rev->diffopt.needed_rename_limit = saved_nrl;
     - 
     -
       ## object-name.c ##
      @@
       #include "repo-settings.h"
     @@ object-name.c: static int get_oid_basic(struct repository *r, const char *str, i
       {
       	struct object_id oid;
       	enum get_oid_result ret = get_oid_1(r, name, len, &oid,
     +-					    GET_OID_COMMITTISH);
     ++					    GET_OID_COMMITTISH |
     ++					    (lookup_flags & GET_OID_QUIETLY));
     + 	struct commit *commit;
     + 	struct commit_list *p;
     + 
      @@ object-name.c: static enum get_oid_result get_parent(struct repository *r,
       		}
       		p = p->next;
     @@ object-name.c: static enum get_oid_result get_parent(struct repository *r,
       {
       	struct object_id oid;
       	struct commit *commit;
     + 	int ret;
     + 
     +-	ret = get_oid_1(r, name, len, &oid, GET_OID_COMMITTISH);
     ++	ret = get_oid_1(r, name, len, &oid,
     ++			GET_OID_COMMITTISH | (lookup_flags & GET_OID_QUIETLY));
     + 	if (ret)
     + 		return ret;
     + 	commit = lookup_commit_reference(r, &oid);
      @@ object-name.c: static enum get_oid_result get_nth_ancestor(struct repository *r,
       		return MISSING_OBJECT;
       
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
      +		"git fetch --deepen=1 origin main"
      +'
      +
     ++test_expect_success 'shallowHistory advice on chained ^ shows the hint only once' '
     ++	test_commit shallow_chain_1 &&
     ++	git clone --no-local --depth=1 --branch main --single-branch \
     ++		.git shallow-advice-chain &&
     ++	test_when_finished "rm -rf shallow-advice-chain" &&
     ++	oid=$(git -C shallow-advice-chain rev-parse --short origin/main) &&
     ++	test_must_fail git -C shallow-advice-chain rev-parse origin/main^^ 2>err &&
     ++	check_shallow_history_advice origin/main "$oid" \
     ++		"git fetch --deepen=1 origin main"
     ++'
     ++
      +test_expect_success 'shallowHistory advice on ^N suggests deepen=1 even for a merge parent' '
      +	test_commit shallow_merge_base &&
      +	git checkout -q -b shallow-merge-side &&
     @@ t/t1500-rev-parse.sh: test_expect_success 'rev-parse --is-shallow-repository in
       test_expect_success 'rev-parse --show-object-format in repo' '
       	test_oid algo >expect &&
       	git rev-parse --show-object-format >actual &&
     -
     - ## t/t4202-log.sh ##
     -@@ t/t4202-log.sh: test_expect_success 'log --invert-grep --grep --author' '
     - 	test_cmp expect actual
     - '
     - 
     -+check_log_shallow_history_advice () {
     -+	oid=$1 &&
     -+	grep '^hint:' err >actual &&
     -+	cat >expect <<-EOF &&
     -+	hint: 'git log' stopped at $oid because this repository is a shallow
     -+	hint: clone, and might have more history upstream that was never fetched.
     -+	hint: Disable this message with "git config set advice.shallowHistory false"
     -+	EOF
     -+	test_cmp expect actual
     -+}
     -+
     -+test_expect_success 'set up linear history for shallow log advice tests' '
     -+	git checkout --orphan loghist &&
     -+	test_commit loghist_1 &&
     -+	test_commit loghist_2 &&
     -+	test_commit loghist_3 &&
     -+	test_commit loghist_4 &&
     -+	test_commit loghist_5 &&
     -+	git checkout main
     -+'
     -+
     -+test_expect_success 'log -<n> advises when it runs out of history at a shallow boundary' '
     -+	git clone --no-local --depth=2 --branch loghist --single-branch \
     -+		.git shallow-log-advice &&
     -+	test_when_finished "rm -rf shallow-log-advice" &&
     -+	oid=$(git -C shallow-log-advice rev-parse --short origin/loghist~1) &&
     -+	git -C shallow-log-advice log -5 --oneline origin/loghist >out 2>err &&
     -+	check_log_shallow_history_advice "$oid"
     -+'
     -+
     -+test_expect_success 'log --since advises when it runs out of history at a shallow boundary' '
     -+	git clone --no-local --depth=2 --branch loghist --single-branch \
     -+		.git shallow-log-advice-since &&
     -+	test_when_finished "rm -rf shallow-log-advice-since" &&
     -+	oid=$(git -C shallow-log-advice-since rev-parse --short origin/loghist~1) &&
     -+	git -C shallow-log-advice-since log --since=2000-01-01 --oneline \
     -+		origin/loghist >out 2>err &&
     -+	check_log_shallow_history_advice "$oid"
     -+'
     -+
     -+test_expect_success 'log without -<n> or --since does not advise at a shallow boundary' '
     -+	git clone --no-local --depth=1 --branch loghist --single-branch \
     -+		.git shallow-log-plain &&
     -+	test_when_finished "rm -rf shallow-log-plain" &&
     -+	git -C shallow-log-plain log --oneline origin/loghist >out 2>err &&
     -+	test_grep ! "^hint:" err
     -+'
     -+
     -+test_expect_success 'log -<n> does not advise when satisfied within local history' '
     -+	git clone --no-local --depth=5 --branch loghist --single-branch \
     -+		.git shallow-log-satisfied &&
     -+	test_when_finished "rm -rf shallow-log-satisfied" &&
     -+	git -C shallow-log-satisfied log -2 --oneline origin/loghist >out 2>err &&
     -+	test_grep ! "^hint:" err
     -+'
     -+
     -+test_expect_success 'log -<n> shallow history advice can be disabled' '
     -+	git clone --no-local --depth=1 --branch loghist --single-branch \
     -+		.git shallow-log-off &&
     -+	test_when_finished "rm -rf shallow-log-off" &&
     -+	git -C shallow-log-off -c advice.shallowHistory=false \
     -+		log -5 --oneline origin/loghist >out 2>err &&
     -+	test_grep ! "^hint:" err
     -+'
     -+
     -+test_expect_success 'log -<n> does not advise in a non-shallow repository' '
     -+	test_when_finished "rm -rf non-shallow-log" &&
     -+	git init -q non-shallow-log &&
     -+	test_commit -C non-shallow-log only-commit &&
     -+	git -C non-shallow-log log -20 --oneline >out 2>err &&
     -+	test_grep ! "^hint:" err
     -+'
     -+
     - test_done


 Documentation/config/advice.adoc |   4 +
 advice.c                         |   1 +
 advice.h                         |   1 +
 object-name.c                    | 131 +++++++++++++++++++++++++++++--
 shallow.c                        |   8 ++
 shallow.h                        |   8 ++
 t/t1500-rev-parse.sh             | 120 ++++++++++++++++++++++++++++
 7 files changed, 266 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/advice.adoc b/Documentation/config/advice.adoc
index 81f80a9274..bb1e508bb6 100644
--- a/Documentation/config/advice.adoc
+++ b/Documentation/config/advice.adoc
@@ -128,6 +128,10 @@ all advice messages.
 		give directions on how to proceed from the current state.
 	sequencerInUse::
 		Shown when a sequencer command is already in progress.
+	shallowHistory::
+		Shown when `<rev>~<n>` cannot resolve enough ancestors of
+		`<rev>` because history stops at a shallow boundary, to
+		suggest fetching more history.
 	skippedCherryPicks::
 		Shown when linkgit:git-rebase[1] skips a commit that has already
 		been cherry-picked onto the upstream branch.
diff --git a/advice.c b/advice.c
index 63bf8b0c5f..e22706ead5 100644
--- a/advice.c
+++ b/advice.c
@@ -80,6 +80,7 @@ static struct {
 	[ADVICE_RM_HINTS]				= { "rmHints" },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
+	[ADVICE_SHALLOW_HISTORY]			= { "shallowHistory" },
 	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
 	[ADVICE_SPARSE_INDEX_EXPANDED]			= { "sparseIndexExpanded" },
 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
diff --git a/advice.h b/advice.h
index 66f6cd6a77..e80e3e82cc 100644
--- a/advice.h
+++ b/advice.h
@@ -47,6 +47,7 @@ enum advice_type {
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,
 	ADVICE_SET_UPSTREAM_FAILURE,
+	ADVICE_SHALLOW_HISTORY,
 	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_SPARSE_INDEX_EXPANDED,
 	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
diff --git a/object-name.c b/object-name.c
index 4eda8c8eac..7376c24601 100644
--- a/object-name.c
+++ b/object-name.c
@@ -22,6 +22,7 @@
 #include "repo-settings.h"
 #include "repository.h"
 #include "setup.h"
+#include "shallow.h"
 #include "midx.h"
 #include "commit-reach.h"
 #include "date.h"
@@ -824,13 +825,120 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 	return 0;
 }
 
+struct remote_tracking_search {
+	char *dst;
+	char *remote_name;
+	char *branch_name;
+};
+
+static int search_remote_tracking(struct remote *remote, void *cb_data)
+{
+	struct remote_tracking_search *search = cb_data;
+	struct refspec_item query;
+
+	memset(&query, 0, sizeof(query));
+	query.dst = search->dst;
+	if (remote_find_tracking(remote, &query))
+		return 0;
+	search->remote_name = xstrdup(remote->name);
+	search->branch_name = query.src;
+	return 1;
+}
+
+/*
+ * If "name" resolves to a remote-tracking ref, find which configured
+ * remote it actually belongs to and what branch of that remote's it is,
+ * by reverse mapping through each remote's real fetch refspec instead
+ * of assuming the common "refs/remotes/<remote>/<branch>" layout,
+ * since a remote's refspec need not put its branches there at all. On a
+ * match, fill "remote_out" and "branch_out" with newly allocated copies
+ * and return true. Otherwise leave them untouched and return false.
+ */
+static bool parse_remote_tracking_name(struct repository *r,
+				       const char *name, int namelen,
+				       char **remote_out, char **branch_out)
+{
+	struct object_id oid;
+	char *real_ref = NULL;
+	struct remote_tracking_search search = { 0 };
+	const char *branch_name;
+
+	if (!repo_dwim_ref(r, name, namelen, &oid, &real_ref, 0) || !real_ref)
+		return false;
+
+	search.dst = real_ref;
+	if (!for_each_remote(search_remote_tracking, &search)) {
+		free(real_ref);
+		return false;
+	}
+	free(real_ref);
+
+	branch_name = search.branch_name;
+	skip_prefix(branch_name, "refs/heads/", &branch_name);
+	*remote_out = search.remote_name;
+	*branch_out = xstrdup(branch_name);
+	free(search.branch_name);
+	return true;
+}
+
+/*
+ * When a "name~<n>" or "name^<n>" walk runs out of parents at "commit",
+ * and that is because "commit" is where this shallow repository's history
+ * was cut off (rather than commit being a real root commit), let the
+ * user know that fetching more history might be what they are after.
+ *
+ * "suggested_depth" is the --deepen value to recommend. For "name^<n>"
+ * this is always 1: deepening by one generation fetches "commit"'s real
+ * parent list in full, whatever it turns out to contain, regardless of
+ * which parent index <n> asked for.
+ */
+static void advise_if_shallow_cutoff(struct repository *r,
+				     const char *name, int namelen,
+				     struct commit *commit,
+				     unsigned lookup_flags,
+				     int suggested_depth)
+{
+	char *remote = NULL, *branch = NULL;
+	struct strbuf cmd = STRBUF_INIT;
+
+	if (lookup_flags & GET_OID_QUIETLY)
+		return;
+	if (!is_repository_shallow(r))
+		return;
+	if (!commit_is_shallow_boundary(r, &commit->object.oid))
+		return;
+
+	if (parse_remote_tracking_name(r, name, namelen, &remote, &branch))
+		strbuf_addf(&cmd, "git fetch --deepen=%d %s %s",
+			    suggested_depth, remote, branch);
+	else
+		strbuf_addf(&cmd, "git fetch --deepen=%d <remote> <branch>",
+			    suggested_depth);
+	free(remote);
+	free(branch);
+
+	advise_if_enabled(ADVICE_SHALLOW_HISTORY,
+			   _("'%.*s' does not have that many ancestors locally.\n"
+			     "History stops at %s because this repository is a\n"
+			     "shallow clone, and might have more history upstream.\n"
+			     "To check, try:\n"
+			     "\n"
+			     "  %s"),
+			   namelen, name,
+			   repo_find_unique_abbrev(r, &commit->object.oid, DEFAULT_ABBREV),
+			   cmd.buf);
+	strbuf_release(&cmd);
+}
+
 static enum get_oid_result get_parent(struct repository *r,
 				      const char *name, int len,
-				      struct object_id *result, int idx)
+				      struct object_id *result, int idx,
+				      unsigned lookup_flags)
 {
 	struct object_id oid;
 	enum get_oid_result ret = get_oid_1(r, name, len, &oid,
-					    GET_OID_COMMITTISH);
+					    GET_OID_COMMITTISH |
+					    (lookup_flags & GET_OID_QUIETLY));
 	struct commit *commit;
 	struct commit_list *p;
 
@@ -851,19 +959,22 @@ static enum get_oid_result get_parent(struct repository *r,
 		}
 		p = p->next;
 	}
+	advise_if_shallow_cutoff(r, name, len, commit, lookup_flags, 1);
 	return MISSING_OBJECT;
 }
 
 static enum get_oid_result get_nth_ancestor(struct repository *r,
 					    const char *name, int len,
 					    struct object_id *result,
-					    int generation)
+					    int generation,
+					    unsigned lookup_flags)
 {
 	struct object_id oid;
 	struct commit *commit;
 	int ret;
 
-	ret = get_oid_1(r, name, len, &oid, GET_OID_COMMITTISH);
+	ret = get_oid_1(r, name, len, &oid,
+			GET_OID_COMMITTISH | (lookup_flags & GET_OID_QUIETLY));
 	if (ret)
 		return ret;
 	commit = lookup_commit_reference(r, &oid);
@@ -871,8 +982,14 @@ static enum get_oid_result get_nth_ancestor(struct repository *r,
 		return MISSING_OBJECT;
 
 	while (generation--) {
-		if (repo_parse_commit(r, commit) || !commit->parents)
+		if (repo_parse_commit(r, commit))
+			return MISSING_OBJECT;
+		if (!commit->parents) {
+			/* Remaining "generation" plus this failed step is the actual gap. */
+			advise_if_shallow_cutoff(r, name, len, commit,
+						 lookup_flags, generation + 1);
 			return MISSING_OBJECT;
+		}
 		commit = commit->parents->item;
 	}
 	oidcpy(result, &commit->object.oid);
@@ -1119,9 +1236,9 @@ static enum get_oid_result get_oid_1(struct repository *r,
 		else if (num > INT_MAX)
 			return MISSING_OBJECT;
 		if (has_suffix == '^')
-			return get_parent(r, name, len1, oid, num);
+			return get_parent(r, name, len1, oid, num, lookup_flags);
 		/* else if (has_suffix == '~') -- goes without saying */
-		return get_nth_ancestor(r, name, len1, oid, num);
+		return get_nth_ancestor(r, name, len1, oid, num, lookup_flags);
 	}
 
 	ret = peel_onion(r, name, len, oid, lookup_flags);
diff --git a/shallow.c b/shallow.c
index 8e244a5669..2e0cf09f32 100644
--- a/shallow.c
+++ b/shallow.c
@@ -92,6 +92,14 @@ int is_repository_shallow(struct repository *r)
 	return r->parsed_objects->is_shallow;
 }
 
+int commit_is_shallow_boundary(struct repository *r,
+			       const struct object_id *oid)
+{
+	struct commit_graft *graft = lookup_commit_graft(r, oid);
+
+	return graft && graft->nr_parent == -1;
+}
+
 static void reset_repository_shallow(struct repository *r)
 {
 	r->parsed_objects->is_shallow = -1;
diff --git a/shallow.h b/shallow.h
index 6a64db42c9..66c9e723f7 100644
--- a/shallow.h
+++ b/shallow.h
@@ -15,6 +15,14 @@ int register_shallow(struct repository *r, const struct object_id *oid);
 int unregister_shallow(const struct object_id *oid);
 int is_repository_shallow(struct repository *r);
 
+/*
+ * True if "oid" is where this shallow repository's history was cut off,
+ * i.e. a shallow-graft commit with no parents recorded locally, as
+ * opposed to a commit that is actually a root.
+ */
+int commit_is_shallow_boundary(struct repository *r,
+			       const struct object_id *oid);
+
 /*
  * Lock for updating the $GIT_DIR/shallow file.
  *
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 4174ca40c3..fbb6e6e582 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -193,6 +193,126 @@ test_expect_success 'rev-parse --is-shallow-repository in non-shallow repo' '
 	test_cmp expect actual
 '
 
+check_shallow_history_advice () {
+	name=$1 oid=$2 cmd=$3 &&
+	grep '^hint:' err >actual &&
+	cat >expect <<-EOF &&
+	hint: '$name' does not have that many ancestors locally.
+	hint: History stops at $oid because this repository is a
+	hint: shallow clone, and might have more history upstream.
+	hint: To check, try:
+	hint:
+	hint:   $cmd
+	hint: Disable this message with "git config set advice.shallowHistory false"
+	EOF
+	test_cmp expect actual
+}
+
+test_expect_success 'shallowHistory advice on ~N beyond shallow boundary' '
+	test_commit shallow_advice_1 &&
+	test_commit shallow_advice_2 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice &&
+	test_when_finished "rm -rf shallow-advice" &&
+	oid=$(git -C shallow-advice rev-parse --short origin/main) &&
+	test_must_fail git -C shallow-advice rev-parse origin/main~1 2>err &&
+	check_shallow_history_advice origin/main "$oid" \
+		"git fetch --deepen=1 origin main"
+'
+
+test_expect_success 'shallowHistory advice accounts for depth already present' '
+	test_commit shallow_partial_1 &&
+	test_commit shallow_partial_2 &&
+	test_commit shallow_partial_3 &&
+	test_commit shallow_partial_4 &&
+	test_commit shallow_partial_5 &&
+	test_commit shallow_partial_6 &&
+	git clone --no-local --depth=3 --branch main --single-branch \
+		.git shallow-advice-partial &&
+	test_when_finished "rm -rf shallow-advice-partial" &&
+	(
+		cd shallow-advice-partial &&
+		oid=$(git rev-parse --short origin/main~2) &&
+		test_must_fail git rev-parse origin/main~5 2>err &&
+		check_shallow_history_advice origin/main "$oid" \
+			"git fetch --deepen=3 origin main" &&
+		git fetch --deepen=3 origin &&
+		git rev-parse origin/main~5 &&
+		test_must_fail git rev-parse origin/main~6
+	)
+'
+
+test_expect_success 'shallowHistory advice on ^N (first parent) beyond shallow boundary' '
+	test_commit shallow_caret_1 &&
+	test_commit shallow_caret_2 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-caret &&
+	test_when_finished "rm -rf shallow-advice-caret" &&
+	oid=$(git -C shallow-advice-caret rev-parse --short origin/main) &&
+	test_must_fail git -C shallow-advice-caret rev-parse origin/main^1 2>err &&
+	check_shallow_history_advice origin/main "$oid" \
+		"git fetch --deepen=1 origin main"
+'
+
+test_expect_success 'shallowHistory advice on chained ^ shows the hint only once' '
+	test_commit shallow_chain_1 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-chain &&
+	test_when_finished "rm -rf shallow-advice-chain" &&
+	oid=$(git -C shallow-advice-chain rev-parse --short origin/main) &&
+	test_must_fail git -C shallow-advice-chain rev-parse origin/main^^ 2>err &&
+	check_shallow_history_advice origin/main "$oid" \
+		"git fetch --deepen=1 origin main"
+'
+
+test_expect_success 'shallowHistory advice on ^N suggests deepen=1 even for a merge parent' '
+	test_commit shallow_merge_base &&
+	git checkout -q -b shallow-merge-side &&
+	test_commit shallow_merge_side1 &&
+	git checkout -q main &&
+	test_commit shallow_merge_main1 &&
+	git merge -q --no-ff shallow-merge-side -m "shallow merge commit" &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-merge &&
+	test_when_finished "rm -rf shallow-advice-merge" &&
+	(
+		cd shallow-advice-merge &&
+		oid=$(git rev-parse --short origin/main) &&
+		test_must_fail git rev-parse origin/main^2 2>err &&
+		check_shallow_history_advice origin/main "$oid" \
+			"git fetch --deepen=1 origin main" &&
+		git fetch -q --deepen=1 origin &&
+		git rev-parse origin/main^1 &&
+		git rev-parse origin/main^2
+	)
+'
+
+test_expect_success 'shallowHistory advice can be disabled' '
+	test_commit shallow_off_1 &&
+	git clone --no-local --depth=1 --branch main --single-branch \
+		.git shallow-advice-off &&
+	test_when_finished "rm -rf shallow-advice-off" &&
+	test_must_fail git -C shallow-advice-off \
+		-c advice.shallowHistory=false rev-parse origin/main~1 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'shallowHistory advice not shown for a non-shallow repository' '
+	test_must_fail git rev-parse HEAD~100000 2>err &&
+	test_grep ! "^hint:" err
+'
+
+test_expect_success 'shallowHistory advice not shown when resolution succeeds' '
+	test_commit shallow_ok_1 &&
+	test_commit shallow_ok_2 &&
+	test_commit shallow_ok_3 &&
+	git clone --no-local --depth=3 --branch main --single-branch \
+		.git shallow-advice-ok &&
+	test_when_finished "rm -rf shallow-advice-ok" &&
+	git -C shallow-advice-ok rev-parse origin/main~1 >actual 2>err &&
+	test_grep ! "^hint:" err
+'
+
 test_expect_success 'rev-parse --show-object-format in repo' '
 	test_oid algo >expect &&
 	git rev-parse --show-object-format >actual &&

base-commit: 3bc0341126508f78f5869cbfc0005e987efdf0c7
-- 
gitgitgadget

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Asking for a commit's ancestor with <rev>~N in a shallow clone that
> does not have N commits of history locally fails with a bare "is not
> a commit" error, with no indication that the repository being shallow
> is the reason, or what to do about it.
> ...
>     Changes in v3:
>     
>      * Drop the git log -<n>/--since extension entirely, per feedback that
>        last commit shown is an unreliable signal for a shallow boundary.
>      * Fix a bug for chained <rev>^^^ where it would print the hint twice.

We unfortunately haven't seen many folks interested in this change.
Is anybody interested in reviewing the patch to give it a push?

Unless we see some volunteers to give it a review in a few weeks, I
will discard the topic out of my tree, as 'seen' has been crowded
with topics that have been collecting dust without getting many
reviews.

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Harald Nordgren]

On Mon, Nov 2, 2026 at 10:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Harald Nordgren <haraldnordgren@gmail.com>
> >
> > Asking for a commit's ancestor with <rev>~N in a shallow clone that
> > does not have N commits of history locally fails with a bare "is not
> > a commit" error, with no indication that the repository being shallow
> > is the reason, or what to do about it.
> > ...
> >     Changes in v3:
> >
> >      * Drop the git log -<n>/--since extension entirely, per feedback that
> >        last commit shown is an unreliable signal for a shallow boundary.
> >      * Fix a bug for chained <rev>^^^ where it would print the hint twice.
>
> We unfortunately haven't seen many folks interested in this change.
> Is anybody interested in reviewing the patch to give it a push?
>
> Unless we see some volunteers to give it a review in a few weeks, I
> will discard the topic out of my tree, as 'seen' has been crowded
> with topics that have been collecting dust without getting many
> reviews.

Is it possible that maybe the review process needs to change if too
many things are getting stuck? Rather than discarding acceptable work.


Harald

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Kristoffer Haugsbakk]

On Mon, Nov 2, 2026, at 11:06, Harald Nordgren wrote:
> On Mon, Nov 2, 2026 at 10:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > From: Harald Nordgren <haraldnordgren@gmail.com>
>> >
>> > Asking for a commit's ancestor with <rev>~N in a shallow clone that
>> > does not have N commits of history locally fails with a bare "is not
>> > a commit" error, with no indication that the repository being shallow
>> > is the reason, or what to do about it.
>> >[snip]
>>
>> We unfortunately haven't seen many folks interested in this change.
>> Is anybody interested in reviewing the patch to give it a push?
>>
>> Unless we see some volunteers to give it a review in a few weeks, I
>> will discard the topic out of my tree, as 'seen' has been crowded
>> with topics that have been collecting dust without getting many
>> reviews.
>
> Is it possible that maybe the review process needs to change if too
> many things are getting stuck?

The regular reviewers have been talking about review work being scarce
compared to contributions (contributor work) for at least several years
now. How can this be changed if there simply aren’t enough people to
review everything? And why does it *need* to be changed? The only
contributions that I know of that git(1) *needs* is full support for
the hash transition before Git 3.0.

> Rather than discarding acceptable work.

My work which has been stuck is also acceptable to me. ;) But my
opinions are beside the point wrt. an independent review.

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Harald Nordgren]

> > Is it possible that maybe the review process needs to change if too
> > many things are getting stuck?
>
> The regular reviewers have been talking about review work being scarce
> compared to contributions (contributor work) for at least several years
> now. How can this be changed if there simply aren’t enough people to
> review everything?

We can loosen the requirements on perfect code, as long as it does
what it's supposed to and doesn't have security backdoors.

Many things like tests, leaks and linting are already handled
automatically by GitHub CI. It's a good time investment to automate
more, because it frees up resources long-term.

> > Rather than discarding acceptable work.
>
> My work which has been stuck is also acceptable to me. ;) But my
> opinions are beside the point wrt. an independent review.

Maybe we can trade reviews if you send me the link or links here? :)


Harald

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Patrick Steinhardt]

On Mon, Nov 02, 2026 at 12:07:29PM +0100, Harald Nordgren wrote:
> > > Is it possible that maybe the review process needs to change if too
> > > many things are getting stuck?
> >
> > The regular reviewers have been talking about review work being scarce
> > compared to contributions (contributor work) for at least several years
> > now. How can this be changed if there simply aren’t enough people to
> > review everything?
> 
> We can loosen the requirements on perfect code, as long as it does
> what it's supposed to and doesn't have security backdoors.
> 
> Many things like tests, leaks and linting are already handled
> automatically by GitHub CI. It's a good time investment to automate
> more, because it frees up resources long-term.

Even if we automate more of it, that would still not fix our bottleneck.
I do no trust automated reviews enough to just merge patches without a
human review, as part of the review is also the conceptual direction
that we are moving towards. And that can only really be reviewed by
someone who is familiar with the project overall.

Now automation could of course still help us. For example, if
contributors started to self-review their patch series before sending
them out, or if we had a tool like Sashiko [1]. If so, the human review
would happen on a patch series that is in a better shape already, and
that would likely require less of a time investment overall.

Granted, at times we err too much on the side of polishing the last tiny
nits. But quite often that's also us trying to train newcomers to write
proper commit histories in the first place.

What I would very much like to avoid is reducing our standards just
because "carving in to AI is inevitable". I've heard that argument way
too often by now, and I don't think that it's a reasonable stance to
have. It is very much not "inevitable" that a project must move faster.

Patrick

[1]: https://sashiko.dev/

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Kristoffer Haugsbakk]

On Mon, Nov 2, 2026, at 12:07, Harald Nordgren wrote:
>> > Is it possible that maybe the review process needs to change if too
>> > many things are getting stuck?
>>
>> >[snip]
>> > Rather than discarding acceptable work.
>>
>> My work which has been stuck is also acceptable to me. ;) But my
>> opinions are beside the point wrt. an independent review.
>
> Maybe we can trade reviews if you send me the link or links here? :)

Nothing I have right now is important.

Maybe I could look at the design of some of your proposals. I’m not much
of a C programmer.

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Kristoffer Haugsbakk]

On Mon, Nov 2, 2026, at 13:24, Patrick Steinhardt wrote:
> On Mon, Nov 02, 2026 at 12:07:29PM +0100, Harald Nordgren wrote:
>> > > Is it possible that maybe the review process needs to change if too
>> > > many things are getting stuck?
>> >
>> > The regular reviewers have been talking about review work being scarce
>> > compared to contributions (contributor work) for at least several years
>> > now. How can this be changed if there simply aren’t enough people to
>> > review everything?
>>
>> We can loosen the requirements on perfect code, as long as it does
>> what it's supposed to and doesn't have security backdoors.
>>
>> Many things like tests, leaks and linting are already handled
>> automatically by GitHub CI. It's a good time investment to automate
>> more, because it frees up resources long-term.
>
> Even if we automate more of it, that would still not fix our bottleneck.
> I do no trust automated reviews enough to just merge patches without a
> human review, as part of the review is also the conceptual direction
> that we are moving towards. And that can only really be reviewed by
> someone who is familiar with the project overall.

I’m not sure what was meant by automate-more, but if it is rubber
stamping by an AI then I am skeptical as well.

>
> Now automation could of course still help us. For example, if
> contributors started to self-review their patch series before sending
> them out, or if we had a tool like Sashiko [1]. If so, the human review
> would happen on a patch series that is in a better shape already, and
> that would likely require less of a time investment overall.

I have had good experiences with private review by bots.

But again the usual problem with an influx of contributions is that
“requiring” this kind of soft check by a bot is not something that
reviewers can check for. They can check if someone skipped the
whitespace error check. But they can’t check that someone skipped a
self-review. It’s like requiring “using your own voice” for commit
messages. People can just say that they “drafted” it with AI but they
rewrote it “in their own voice”. Uncheckable.

Contributors have gotten a hundred-fold more help to write
code. (And even commit messages for some reason.) Meanwhile, what have
reviewers received?

>
> Granted, at times we err too much on the side of polishing the last tiny
> nits. But quite often that's also us trying to train newcomers to write
> proper commit histories in the first place.
>
> What I would very much like to avoid is reducing our standards just
> because "carving in to AI is inevitable". I've heard that argument way
> too often by now, and I don't think that it's a reasonable stance to
> have. It is very much not "inevitable" that a project must move faster.

*caving

I also don’t see why this project should move faster in terms of features.

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Harald Nordgren]

On Mon, Nov 2, 2026 at 1:25 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Nov 02, 2026 at 12:07:29PM +0100, Harald Nordgren wrote:
> > > > Is it possible that maybe the review process needs to change if too
> > > > many things are getting stuck?
> > >
> > > The regular reviewers have been talking about review work being scarce
> > > compared to contributions (contributor work) for at least several years
> > > now. How can this be changed if there simply aren’t enough people to
> > > review everything?
> >
> > We can loosen the requirements on perfect code, as long as it does
> > what it's supposed to and doesn't have security backdoors.
> >
> > Many things like tests, leaks and linting are already handled
> > automatically by GitHub CI. It's a good time investment to automate
> > more, because it frees up resources long-term.
>
> Even if we automate more of it, that would still not fix our bottleneck.
> I do no trust automated reviews enough to just merge patches without a
> human review, as part of the review is also the conceptual direction
> that we are moving towards. And that can only really be reviewed by
> someone who is familiar with the project overall.

I'm not primarily talking about the review part, but about the
mechanical aspects around reviewing:

You might point out that while the tip a series builds and tests pass,
a commit has a test added before the actual logic was added in the
subsequent commit. This is a class of problems that we can find with
automatic tooling, and then the human reviewers can free up more of
their scarce time for actual reviewing.

> Now automation could of course still help us. For example, if
> contributors started to self-review their patch series before sending
> them out, or if we had a tool like Sashiko [1]. If so, the human review
> would happen on a patch series that is in a better shape already, and
> that would likely require less of a time investment overall.
>
> [1]: https://sashiko.dev/

Interesting if Linux is using that. Linux is our sister project right? ;)

> Granted, at times we err too much on the side of polishing the last tiny
> nits. But quite often that's also us trying to train newcomers to write
> proper commit histories in the first place.

Yes, this is a bigger issue than I think we understand.

The Pareto principle (80/20) generally holds for things like this, and
maybe the last X% of code improvement is not worth the (100-X)% time
investment? For sure it's not linearly distributed.

> What I would very much like to avoid is reducing our standards just
> because "carving in to AI is inevitable". I've heard that argument way
> too often by now, and I don't think that it's a reasonable stance to
> have. It is very much not "inevitable" that a project must move faster.

This is not my main point (although I am AI-positive).


Harald

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Harald Nordgren]

> I’m not much of a C programmer.

I'm quite taken aback by this.

Is the Git code so sacred that even the reviewers are not in a
position to review? :D


Harald

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Patrick Steinhardt]

On Mon, Nov 02, 2026 at 03:07:10PM +0100, Harald Nordgren wrote:
> On Mon, Nov 2, 2026 at 1:25 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Nov 02, 2026 at 12:07:29PM +0100, Harald Nordgren wrote:
> > > > > Is it possible that maybe the review process needs to change if too
> > > > > many things are getting stuck?
> > > >
> > > > The regular reviewers have been talking about review work being scarce
> > > > compared to contributions (contributor work) for at least several years
> > > > now. How can this be changed if there simply aren’t enough people to
> > > > review everything?
> > >
> > > We can loosen the requirements on perfect code, as long as it does
> > > what it's supposed to and doesn't have security backdoors.
> > >
> > > Many things like tests, leaks and linting are already handled
> > > automatically by GitHub CI. It's a good time investment to automate
> > > more, because it frees up resources long-term.
> >
> > Even if we automate more of it, that would still not fix our bottleneck.
> > I do no trust automated reviews enough to just merge patches without a
> > human review, as part of the review is also the conceptual direction
> > that we are moving towards. And that can only really be reviewed by
> > someone who is familiar with the project overall.
> 
> I'm not primarily talking about the review part, but about the
> mechanical aspects around reviewing:
> 
> You might point out that while the tip a series builds and tests pass,
> a commit has a test added before the actual logic was added in the
> subsequent commit. This is a class of problems that we can find with
> automatic tooling, and then the human reviewers can free up more of
> their scarce time for actual reviewing.

Yeah, I agree that providing tooling that allows us to automatically
verify some aspects that would typically be covered during code review
would be good. It would probably need to come in the form of scripts
that can be self-executed though as we don't have enough computing
resources to execute such scripts.

Ideally, we'd also commit to a cetrain machine-verifiable coding style
so that it can be auto-verified, as well. But the discussions around
clang-format never really led anywhere, unfortunately.

> > Now automation could of course still help us. For example, if
> > contributors started to self-review their patch series before sending
> > them out, or if we had a tool like Sashiko [1]. If so, the human review
> > would happen on a patch series that is in a better shape already, and
> > that would likely require less of a time investment overall.
> >
> > [1]: https://sashiko.dev/
> 
> Interesting if Linux is using that. Linux is our sister project right? ;)

Yup. I've been chatting with Roman briefly about this project and the
possibility of adapting it to cover Git, as well. He was quite positive
and mentioned that Google might even sponsor the infrastructure required
for it.

I haven't yet found the time to investigate more though, but I would be
happy to have such tooling in Git. It shouldn't be mandatory, and in the
beginning it might just be the web interface. But if it works well
enough it could even eventually be updated to send review responses to
the mailing list directly.

> > Granted, at times we err too much on the side of polishing the last tiny
> > nits. But quite often that's also us trying to train newcomers to write
> > proper commit histories in the first place.
> 
> Yes, this is a bigger issue than I think we understand.
> 
> The Pareto principle (80/20) generally holds for things like this, and
> maybe the last X% of code improvement is not worth the (100-X)% time
> investment? For sure it's not linearly distributed.

I guess that part of the problem is also that a shallow review that
focusses on stylistic issues is way easier to do compared to one that
does a deep dive into the underlying principles. It's simply a time
tradeoff that reviewers need to do.

That does not mean that those are meaningless though. As said, the
benefit is also in that it allows newcomers to learn, both on the
contributing and on the reviewing side.

Patrick

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Patrick Steinhardt]

On Mon, Nov 02, 2026 at 03:10:56PM +0100, Harald Nordgren wrote:
> > I’m not much of a C programmer.
> 
> I'm quite taken aback by this.
> 
> Is the Git code so sacred that even the reviewers are not in a
> position to review? :D

There are other ways in which people can contribute, e.g. by
contributing documentation. Not everyone contributing to Git is a
developer -- I'd argue we could in fact use way more non-developer
contributions.

Patrick

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Phillip Wood]

On 02/11/2026 12:24, Patrick Steinhardt wrote:
> On Mon, Nov 02, 2026 at 12:07:29PM +0100, Harald Nordgren wrote:
>> 
>> We can loosen the requirements on perfect code, as long as it does
>> what it's supposed to and doesn't have security backdoors.
>>
>> Many things like tests,

The CI checks that the tests pass. It does not tell is if there is a 
sensible amount of test coverage for the change that's being made. 
Stolee used to run a coverage report which was helpful to highlight any 
glaring holes in our coverage but I think he stopped running it because 
it was quite time consuming to maintain. Even having that doesn't tell 
us if someone has added a million pointless tests that slow the test 
suite down - that still requires someone to read and understand them.

> leaks and linting are already handled
>> automatically by GitHub CI. It's a good time investment to automate
>> more, because it frees up resources long-term.

Having automated checks for common errors is certainly a good idea

> Even if we automate more of it, that would still not fix our bottleneck.
> I do no trust automated reviews enough to just merge patches without a
> human review, as part of the review is also the conceptual direction
> that we are moving towards. And that can only really be reviewed by
> someone who is familiar with the project overall.

Agreed - it is no good having a "perfect" patch if it implements a bad idea.

> Now automation could of course still help us. For example, if
> contributors started to self-review their patch series before sending
> them out, 

That would be really helpful. Once someone has submitted a few patches 
they should know what issues reviewers are likely to bring up and be 
able to preempt that by at least trying to fix those issues before the 
submitting their patches. I don't mind spending time helping new 
contributors get used to the process, but after a few series they really 
should have got the hang of writing a cover letter and thinking about 
how their changes affect other users.

A while ago Junio floated the idea of requiring review contributions 
from contributors who submit patches. I wonder it would help to say 
something like "for every three patch series you submit you should 
provide an in-depth review of one series from another contributor". I'm 
not sure how well that would work (I seem to remember reading PostgreSQL 
tried something similar and ended up abandoning it) but it might be 
worth a try.

Thanks

Phillip
  > or if we had a tool like Sashiko [1]. If so, the human review
> would happen on a patch series that is in a better shape already, and
> that would likely require less of a time investment overall.
> 
> Granted, at times we err too much on the side of polishing the last tiny
> nits. But quite often that's also us trying to train newcomers to write
> proper commit histories in the first place.
> 
> What I would very much like to avoid is reducing our standards just
> because "carving in to AI is inevitable". I've heard that argument way
> too often by now, and I don't think that it's a reasonable stance to
> have. It is very much not "inevitable" that a project must move faster.
> 
> Patrick
> 
> [1]: https://sashiko.dev/

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Harald Nordgren]

> > > Granted, at times we err too much on the side of polishing the last tiny
> > > nits. But quite often that's also us trying to train newcomers to write
> > > proper commit histories in the first place.
> >
> > Yes, this is a bigger issue than I think we understand.
> >
> > The Pareto principle (80/20) generally holds for things like this, and
> > maybe the last X% of code improvement is not worth the (100-X)% time
> > investment? For sure it's not linearly distributed.
>
> I guess that part of the problem is also that a shallow review that
> focusses on stylistic issues is way easier to do compared to one that
> does a deep dive into the underlying principles. It's simply a time
> tradeoff that reviewers need to do.

It's a vicious dopamine cycle.

I would probably like to see a soft rule / principle in place of no
nitpicking until the idea has been settled. Nitpicking wastes precious
reviewer time, and will often generate a new version, which ends up
biting the reviewer again, and so on.


Harald

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Harald Nordgren]

> The CI checks that the tests pass. It does not tell is if there is a
> sensible amount of test coverage for the change that's being made.
> Stolee used to run a coverage report which was helpful to highlight any
> glaring holes in our coverage but I think he stopped running it because
> it was quite time consuming to maintain.

It certainly shouldn't rely on a single individual.

There are tools like Codecov, that can be hooked into GitHub pull requests.

> it was quite time consuming to maintain. Even having that doesn't tell
> us if someone has added a million pointless tests that slow the test
> suite down - that still requires someone to read and understand them.

Yes, but this is still a subset of all cases.


Harald

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

>> We unfortunately haven't seen many folks interested in this change.
>> Is anybody interested in reviewing the patch to give it a push?
>>
>> Unless we see some volunteers to give it a review in a few weeks, I
>> will discard the topic out of my tree, as 'seen' has been crowded
>> with topics that have been collecting dust without getting many
>> reviews.
>
> Is it possible that maybe the review process needs to change if too
> many things are getting stuck? Rather than discarding acceptable work.

If it were acceptable, it would have long been in 'next' and
'master', though.  One of the important criteria for being
acceptable is whether others find it a reasonable change to make to
the system, and I do not know of a way to show that other than by
positive reviews.

And that is why I pinged the thread again.

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> ... part of the review is also the conceptual direction
> that we are moving towards. And that can only really be reviewed by
> someone who is familiar with the project overall.

Well said.

> What I would very much like to avoid is reducing our standards just
> because "carving in to AI is inevitable". I've heard that argument way
> too often by now, and I don't think that it's a reasonable stance to
> have. It is very much not "inevitable" that a project must move faster.

Yes, I very much agree with this.  There was a release few cycles
ago where I blindly trusted AI generated reviews too much, allowed
topics without human reviews to be merged to 'next' and 'master',
that resulted in quite a few immediate reverts.  I learned from that
episode and am basing heavily on human reviews the decision to mark
the topic for 'next' these days.

Thanks.

Re: [PATCH v3] object-name: explain why <rev>~N fails in a shallow clone

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> something like "for every three patch series you submit you should 
> provide an in-depth review of one series from another contributor". I'm 
> not sure how well that would work (I seem to remember reading PostgreSQL 
> tried something similar and ended up abandoning it) but it might be 
> worth a try.

Hmph, I do not know if you want a "contributor" who throws too many
AI slop at us to contaminate discussion threads on others' patches
with their AI generated "reviews" in the first place, though.