object-name: explain why <rev>~N fails in a shallow clone - #2413

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

On Sun, Sep 20, 2026 at 5:55 AM Harald Nordgren via GitGitGadget
<[email protected]> wrote:
>
> From: Harald Nordgren <[email protected]>
>
> 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

"Harald Nordgren via GitGitGadget" <[email protected]> writes:

> From: Harald Nordgren <[email protected]>
>
> 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 <[email protected]>
> ---
>     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.

> 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

Harald Nordgren <[email protected]> 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.

"Harald Nordgren via GitGitGadget" <[email protected]> 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.