Re: [PATCH] pull: avoid crash of invalid merge head

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

"Jiri Kuncar via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jiri Kuncar <jiri.kuncar@gmail.com>
>
> Adds NULL guards for lookup_commit_reference() to avoid segfaults.
>
> Those invalid references are possibly caused by parallel fetches or
> gc racing on the same repository.
>
> This effectively treats failed lookup as "not up to date" so caller
> falls to a normal merge, which reports the broken object instead of
> crashing.
>
> Signed-off-by: Jiri Kuncar <jiri.kuncar@gmail.com>
> ---
>     pull: avoid crash of invalid merge head

The log message sounds a bit unusual from our norm (see
Documentation/SubmittingPatches).

It is of course good to deal with a corrupt state more gracefully
rather than crashing.  From a cursory look, the particular solution
chosen, to drive the caller to perform a merge and have it fail, may
smell a bit like cheating, in that we could diagnose the breakage
better by reporting what was broken at each place, but it probably
is a good choice.

If we really want to improve the situation for 'orig_head', for
example, we would probably want to turn it into a commit object
instance a lot earlier and pass the commit object instance around in
the call chain.  Passing around many struct object_id instances
instead of object instances is an unnatural consequence of how this
program evolved.  It was originally written as a shell script, and
of course passing hexadecimal object names was the only way the
script could drive 'git merge-base' and other programs to see if the
commit recorded as the current 'HEAD' will fast-forward to the
commit that is fetched from the remote to be merged in, for example.
Once we go that route to resolve object names early to object
instances, we will not have multiple lookup_commit_reference() calls
on the same object name (which require us to watch out for failures)
to begin with.

The above is a long-winded way to say that it is a good improvement
that does not do more than it needs to do and we will not have to
spend too much effort to undo when we revamp the internals to do
"the right thing" later.


> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 27f38ab3c8..7a3eadddd3 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -888,4 +888,30 @@ test_expect_success 'git pull --rebase against local branch' '
>  	test_cmp expect file2
>  '
>  
> +test_expect_success 'pull does not crash when a merge head does not resolve' '
> +	test_when_finished "rm -rf up dn" &&
> +	git init up &&
> +	(
> +		cd up &&
> +		test_commit base &&
> +		git switch -c sideA &&
> +		test_commit a &&
> +		git switch -c sideB base &&
> +		test_commit b
> +	) &&
> +	git clone up dn &&
> +	(
> +		cd dn &&
> +		git -c fetch.unpackLimit=1000 fetch origin \
> +			"+refs/heads/*:refs/remotes/origin/*" &&
> +		git commit-graph write --reachable &&
> +		oid=$(git rev-parse refs/remotes/origin/sideA) &&
> +		obj=.git/objects/$(test_oid_to_path "$oid") &&
> +		test -f "$obj" &&
> +		chmod u+w "$obj" &&
> +		>"$obj" &&
> +		test_must_fail git pull --no-rebase origin sideA sideB
> +	)
> +'

The "test -f" there smells more like a debugging aid for this test
than making sure the fixed program works as expected.  I wonder if
it is simpler (and more portable to non-POSIX environments) if we
replace the "corrupt $obj" step with 'rm -f "$obj"'.

Thanks.