Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

On Sun Feb 4, 2024 at 4:03 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > Therefore, make a new function user_meant_head() which takes the
> > revision string and compares it to 'HEAD' as well as '@'. However, in
> > builtin/checkout.c, there is a logic to convert all command line input
> > rev to the raw object name for underlying machinery (e.g., diff-index)
> > that does not recognize the <a>...<b> notation, but we'd need to leave
> > 'HEAD' intact.  Now we need to teach that '@' is a synonym to 'HEAD'
> >  to that code and leave '@' intact, too.
>
> I am not sure what that "However" wants to say.
>
>  - Now we have a helper function to see what the end-user said, and
>    tell if the end-user meant the state that is currently checked
>    out (aka "HEAD" but some folks like a synonym "@"[*]), or if the
>    end-user meant some other "concrete" branch.
>
>  - In builtin/checkout.c, there is a logic to convert unless what
>    the end-user meant is the state that is currently checked out.
>
> Isn't the natural conclusion that follows these two stentences
> "therefore, the latter is a very good place to use that helper
> function, too"?
Yeah, I did not use the helper function in builtin/checkout.c. Hence the
"However". But I agree on the point of exporting the function.
Therefore I have attached the patch with the updated message below.

> 	Side note: the "@" is already problematic not just because
> 	"git branch @" would not refuse to create "refs/heads/@",
> 	but there is no ref "@" (like $GIT_DIR/refs/@ or $GIT_DIR/@)
> 	when it is used as a synonym for "HEAD".  There is a check
> 	in builtin/checkout.c:update_refs_for_switch() that runs
> 	strcmp() on a token given by the end-user from the command
> 	line with "HEAD" to notice the no-op case "git checkout
> 	HEAD" but the code does not trigger when "@" is given, and
> 	it happens to work by accident.  I really wish we didn't add
> 	that oddball synonym, but that is water under the bridge by
> 	now.
well, I suppose it is maybe annoying from the development perspective,
but users seem to like the concept of it[1].

> In any case, I think we'd find more places that currently treats the
> token "HEAD" given directly by the end-user specially and may want
> to teach at least some of them to also accept "@" the same way, and
> the helper function you are introducing may become useful in the
> future, at which time we may move it to a more public header.  If it
> needs to be shared already between add-patch.c and builtin/checkout.c
> (I am guessing what you meant with "However" as an excuse for open
> coding it instead of sharing the code), perhaps we should do so without
> waiting for that future, though.  I dunno.

Yeah, that "However" was for not using the helper function.

> If we choose to do so, for now, a squashable patch may look like the
> attached, but we'd need to update the log message while squashing it
> in.
Thanks for the patch. Updated message is below.

[Footnote]
[1]: https://www.reddit.com/r/git/comments/k15cqm/do_you_know_is_a_shortcut_for_head/

-- >8 --
Subject: [PATCH] add-patch: classify '@' as a synonym for 'HEAD'

Currently, (checkout, reset, restore) commands correctly take '@' as a
synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and
'HEAD', different prompts/messages are given by the commands mentioned
above (because of applying reverse mode(-R) in case of '@'). This is due to
the literal and only string comparison with the word 'HEAD' in run_add_p().
Synonymity between '@' and 'HEAD' is obviously desired, especially since
'@' already resolves to 'HEAD'.

Therefore, make a new function the_user_meant_head() which takes the
revision string and compares it to 'HEAD' as well as '@'. Also in
builtin/checkout.c, there is a logic to convert all command line input
rev to the raw object name for underlying machinery (e.g., diff-index)
that does not recognize the <a>...<b> notation, but we'd need to leave
'HEAD' intact.  Now we need to teach that '@' is a synonym to 'HEAD'
to that code and leave '@' intact, too. Therefore, this function is
already useful in add-patch.c and builtin/checkout.c and may also
become helpful in other places in future. Therefore, it makes sense to
export it.

There is one unintended side-effect/behavior change of this, even if
there exists a branch named '@', when providing '@' as a rev-source to
(checkout, reset, restore) commands in patch mode, it will consider it
as HEAD. This is due to the behavior of diff-index. However, naming a
branch '@' is an obvious foot-gun and there are many existing commands
which already take '@' for 'HEAD' regardless of whether 'refs/heads/@'
exists or not (e.g., 'git log @', 'git push origin @' etc.). Therefore,
this should be fine.

Also, add tests for checking the above mentioned synonymity.

Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-interactive.h         | 14 ++++++++++++
 add-patch.c               |  6 ++---
 builtin/checkout.c        | 10 ++++-----
 t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------
 t/t2071-restore-patch.sh  | 18 +++++++++------
 t/t7105-reset-patch.sh    | 10 +++++++++
 6 files changed, 69 insertions(+), 35 deletions(-)

diff --git a/add-interactive.h b/add-interactive.h
index 693f125e8e..ca7326336d 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -38,4 +38,18 @@ enum add_p_mode {
 int run_add_p(struct repository *r, enum add_p_mode mode,
 	      const char *revision, const struct pathspec *ps);
 
+/*
+ * When the user gives these tokens from the command line, they mean
+ * the state that the currently checked out state came from.  This
+ * single bit of information affects the direction in which the patch
+ * is presented to the end-user: are we showing a patch to go back to
+ * the currently committed state, or are we showing a patch to move
+ * forward to the given commit that may be different from the
+ * committed state we started with?
+ */
+static inline int the_user_meant_head(const char *rev)
+{
+	return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
+}
+
 #endif
diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..5502acebb8 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,21 +1729,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 	if (mode == ADD_P_STASH)
 		s.mode = &patch_mode_stash;
 	else if (mode == ADD_P_RESET) {
-		if (!revision || !strcmp(revision, "HEAD"))
+		if (!revision || the_user_meant_head(revision))
 			s.mode = &patch_mode_reset_head;
 		else
 			s.mode = &patch_mode_reset_nothead;
 	} else if (mode == ADD_P_CHECKOUT) {
 		if (!revision)
 			s.mode = &patch_mode_checkout_index;
-		else if (!strcmp(revision, "HEAD"))
+		else if (the_user_meant_head(revision))
 			s.mode = &patch_mode_checkout_head;
 		else
 			s.mode = &patch_mode_checkout_nothead;
 	} else if (mode == ADD_P_WORKTREE) {
 		if (!revision)
 			s.mode = &patch_mode_checkout_index;
-		else if (!strcmp(revision, "HEAD"))
+		else if (the_user_meant_head(revision))
 			s.mode = &patch_mode_worktree_head;
 		else
 			s.mode = &patch_mode_worktree_nothead;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..63c669b157 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -539,12 +539,12 @@ static int checkout_paths(const struct checkout_opts *opts,
 		 * recognized by diff-index), we will always replace the name
 		 * with the hex of the commit (whether it's in `...` form or
 		 * not) for the run_add_interactive() machinery to work
-		 * properly. However, there is special logic for the HEAD case
-		 * so we mustn't replace that.  Also, when we were given a
-		 * tree-object, new_branch_info->commit would be NULL, but we
-		 * do not have to do any replacement, either.
+		 * properly. However, there is special logic for the 'HEAD' and
+		 * '@' case so we mustn't replace that.  Also, when we were
+		 * given a tree-object, new_branch_info->commit would be NULL,
+		 * but we do not have to do any replacement, either.
 		 */
-		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
+		if (rev && new_branch_info->commit && !the_user_meant_head(rev))
 			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
 
 		if (opts->checkout_index && opts->checkout_worktree)
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 747eb5563e..c4f9bf09aa 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '
 	verify_state dir/foo index index
 '
 
-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
-	set_and_save_state dir/foo work head &&
-	test_write_lines n y n | git checkout -p HEAD &&
-	verify_saved_state bar &&
-	verify_saved_state dir/foo
-'
-
-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
-	test_write_lines n y y | git checkout -p HEAD &&
-	verify_saved_state bar &&
-	verify_state dir/foo head head
-'
-
-test_expect_success 'git checkout -p HEAD with change already staged' '
-	set_state dir/foo index index &&
-	# the third n is to get out in case it mistakenly does not apply
-	test_write_lines n y n | git checkout -p HEAD &&
-	verify_saved_state bar &&
-	verify_state dir/foo head head
-'
+for opt in "HEAD" "@"
+do
+	test_expect_success "git checkout -p $opt with NO staged changes: abort" '
+		set_and_save_state dir/foo work head &&
+		test_write_lines n y n | git checkout -p $opt >output &&
+		verify_saved_state bar &&
+		verify_saved_state dir/foo &&
+		test_grep "Discard" output
+	'
+
+	test_expect_success "git checkout -p $opt with NO staged changes: apply" '
+		test_write_lines n y y | git checkout -p $opt >output &&
+		verify_saved_state bar &&
+		verify_state dir/foo head head &&
+		test_grep "Discard" output
+	'
+
+	test_expect_success "git checkout -p $opt with change already staged" '
+		set_state dir/foo index index &&
+		# the third n is to get out in case it mistakenly does not apply
+		test_write_lines n y n | git checkout -p $opt >output &&
+		verify_saved_state bar &&
+		verify_state dir/foo head head &&
+		test_grep "Discard" output
+	'
+done
 
 test_expect_success 'git checkout -p HEAD^...' '
 	# the third n is to get out in case it mistakenly does not apply
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index b5c5c0ff7e..3dc9184b4a 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '
 	verify_state dir/foo index index
 '
 
-test_expect_success PERL 'git restore -p --source=HEAD' '
-	set_state dir/foo work index &&
-	# the third n is to get out in case it mistakenly does not apply
-	test_write_lines n y n | git restore -p --source=HEAD &&
-	verify_saved_state bar &&
-	verify_state dir/foo head index
-'
+for opt in "HEAD" "@"
+do
+	test_expect_success PERL "git restore -p --source=$opt" '
+		set_state dir/foo work index &&
+		# the third n is to get out in case it mistakenly does not apply
+		test_write_lines n y n | git restore -p --source=$opt >output &&
+		verify_saved_state bar &&
+		verify_state dir/foo head index &&
+		test_grep "Discard" output
+	'
+done
 
 test_expect_success PERL 'git restore -p --source=HEAD^' '
 	set_state dir/foo work index &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 05079c7246..ec7f16dfb6 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' '
 	test_grep "Unstage" output
 '
 
+for opt in "HEAD" "@"
+do
+	test_expect_success PERL "git reset -p $opt" '
+		test_write_lines n y | git reset -p $opt >output &&
+		verify_state dir/foo work head &&
+		verify_saved_state bar &&
+		test_grep "Unstage" output
+	'
+done
+
 test_expect_success PERL 'git reset -p HEAD^' '
 	test_write_lines n y | git reset -p HEAD^ >output &&
 	verify_state dir/foo work parent &&
-- 
2.43.0