Re: [PATCH v12 4/8] remote set-head: better output for --auto

On Fri Nov 15, 2024 at 06:50, Junio C Hamano [off-list ref] wrote:

Bence Ferdinandy [off-list ref] writes:

quoted

Currently, set-head --auto will print a message saying "remote/HEAD set
to branch", which implies something was changed.

Change the output of --auto, so the output actually reflects what was
done: a) set a previously unset HEAD, b) change HEAD because remote
changed or c) no updates. As a fourth output, if HEAD is changed from
a previous value that was not a remote branch, explicitly call attention
to this fact.

OK.  That's sensible.

There is a slight variant of the fourth case.  HEAD may have been a
symbolic ref that pointed at an unexpected place (which you
addressed), or HEAD may have been a non-symbolic ref (which the new
code would mistakenly say "HEAD is now created", if I am reading the
patch correctly).

Good point, and yes, that is what happens. (Although I'm not quite sure how
valid that state is where a remote's HEAD is not a branch).

quoted

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 9b50276646..0ea86d51a4 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh

@@ -432,12 +432,51 @@ test_expect_success 'set-head --auto' '
 	)
 '
 
+test_expect_success 'set-head --auto detects creation' '
+	(
+		cd test &&
+		git symbolic-ref -d refs/remotes/origin/HEAD &&

Are we sure refs/remotes/origin/HEAD exists at this point in the
test, regardless of which earlier tests were skipped or failed?  If
not, perhaps

		git update-ref --no-deref -d refs/remotes/origin/HEAD &&

is a better alternative.

Ack.

quoted

+		git remote set-head --auto origin >output &&
+		echo "${SQ}origin/HEAD${SQ} is now created and points to ${SQ}main${SQ}" >expect &&
+		test_cmp expect output
+	)
+'

Here, we could insert another one:

test_expect_success 'set-head --auto to update a non symbolic ref' '
	(
		cd test &&
		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
		git update-ref refs/remotes/origin/HEAD HEAD &&
		git remote set-head --auto origin >output &&

I'd imagine "output" should at least say that we are setting up a
symref origin/HEAD to point at some ref the --auto option figured
out, and if we wanted to report its previous state, it was a non
symbolic ref that pointed at some commit.  In any case,

		echo "${SQ}origin/HEAD${SQ} is now created and points to ${SQ}main${SQ}" >expect &&

is not what we want to see here, I suspect.

Can we detect the case where we overwrite a non symref with a symref
without going back to step 2/8 and doing a major surgery?

		test_cmp expect output
	)
'

I agree, adding this makes sense. And this also takes us back to the question
of what we should do in 2/8 when refs_read_symbolic_ref exits with 1. I now
tested the behaviour and if origin/HEAD is gibberish, git already dies before
with 

error: cannot lock ref 'refs/remotes/origin/HEAD': unable to resolve reference 'refs/remotes/origin/HEAD': reference broken

so refs_read_symbolic_ref -> 1 only happens if there's a valid non-symbolic ref
in origin/HEAD. So maybe if we put "Not a symbolic reference." in the referent
(which should be an invalid symref), the caller could check for that and then
should be able to distinguish this special case?

Thanks,
Bence

-- 
bence.ferdinandy.com