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

[Junio C Hamano <gitster@pobox.com>]

"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.