[GSOC][RFC PATCH 0/2] add-patch: compare object id

[GSOC][RFC PATCH 0/2] add-patch: compare object id

[Ghanshyam Thakkar]

This patch series is RFC as this contains a partial behavior change.
The first patch introduces a new function to compare object id instead
of revision strings. The second patch removes the special case for HEAD
in builtin/checkout.c.

This patch series enables for HEAD to be specified in any form and
removes need to be kept a literal string 'HEAD'. Consequently, this
removes the need to keep a special case for HEAD in callers of
run_add_p() (as in builtin/checkout.c).

Additional context:
https://lore.kernel.org/git/xmqqsgat7ttf.fsf@gitster.c.googlers.com/

Ghanshyam Thakkar (2):
  add-patch: compare object id instead of literal string
  checkout: remove HEAD special case

 add-patch.c               | 28 +++++++++++++++-------
 builtin/checkout.c        |  9 ++++---
 t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
 t/t2071-restore-patch.sh  | 21 ++++++++++------
 t/t7105-reset-patch.sh    | 14 +++++++++++
 5 files changed, 81 insertions(+), 41 deletions(-)

-- 
2.43.0

[RFC PATCH 1/2] add-patch: compare object id instead of literal string

[Ghanshyam Thakkar]

Add a new function reveq(), which takes repository struct and two revision
strings as arguments and returns 0 if the revisions point to the same
object. Passing a rev which does not point to an object is considered
undefined behavior as the underlying function memcmp() will be called
with NULL hash strings.

Subsequently, replace literal string comparison to HEAD in run_add_p()
with reveq() to handle more ways of saying HEAD (such as '@' or '$branch'
where $branch points to same commit as HEAD). This addresses the
NEEDSWORK comment in run_add_p().

However, in ADD_P_RESET mode keep string comparison in logical OR with
reveq() to handle unborn HEAD.

As for the behavior change, with this patch applied if the given
revision points to the same object as HEAD, the patch mode will be set to
patch_mode_(reset,checkout,worktree)_head instead of
patch_mode_(...)_nothead. That is equivalent of not setting -R flag in
diff-index, which would have been otherwise set before this patch.
However, when given same set of inputs, the actual outcome is same as
before this patch. Therefore, this does not affect any automated scripts.

Also, add testcases to check the similarity of result between different
ways of saying HEAD.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
Should the return values of repo_get_oid() be checked in reveq()? As
reveq() is not a global function and is only used in run_add_p(), the
validity of revisions is already checked beforehand by builtin/checkout.c
and builtin/reset.c before the call to run_add_p().

 add-patch.c               | 28 +++++++++++++++-------
 t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
 t/t2071-restore-patch.sh  | 21 ++++++++++------
 t/t7105-reset-patch.sh    | 14 +++++++++++
 4 files changed, 77 insertions(+), 36 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..01eb71d90e 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -14,6 +14,7 @@
 #include "color.h"
 #include "compat/terminal.h"
 #include "prompt.h"
+#include "hash.h"
 
 enum prompt_mode_type {
 	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
@@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,
 		     INDEX_ENVIRONMENT "=%s", s->s.r->index_file);
 }
 
+// Check if two revisions point to the same object. Passing a rev which does not
+// point to an object is undefined behavior.
+static inline int reveq(struct repository *r, const char *rev1,
+			const char *rev2)
+{
+	struct object_id oid_rev1, oid_rev2;
+	repo_get_oid(r, rev1, &oid_rev1);
+	repo_get_oid(r, rev2, &oid_rev2);
+
+	return !oideq(&oid_rev1, &oid_rev2);
+}
+
 static int parse_range(const char **p,
 		       unsigned long *offset, unsigned long *count)
 {
@@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		s.mode = &patch_mode_stash;
 	else if (mode == ADD_P_RESET) {
 		/*
-		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
-		 * compare the commit objects instead so that other ways of
-		 * saying the same thing (such as "@") are also handled
-		 * appropriately.
-		 *
-		 * This applies to the cases below too.
+		 * The literal string comparison to HEAD below is kept
+		 * to handle unborn HEAD.
 		 */
-		if (!revision || !strcmp(revision, "HEAD"))
+		if (!revision || !strcmp(revision, "HEAD") ||
+		    !reveq(r, revision, "HEAD"))
 			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 (!reveq(r, revision, "HEAD"))
 			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 (!reveq(r, revision, "HEAD"))
 			s.mode = &patch_mode_worktree_head;
 		else
 			s.mode = &patch_mode_worktree_nothead;
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 747eb5563e..431f34fa9c 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -12,6 +12,7 @@ test_expect_success 'setup' '
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
+	git branch newbranch &&
 	set_and_save_state bar bar_work bar_index &&
 	save_head
 '
@@ -38,26 +39,35 @@ 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
-'
+# Note: 'newbranch' points to the same commit as HEAD. And it is technically
+# allowed to name a branch '@' as of now, however in below test '@'
+# represents the shortcut for HEAD.
+for opt in "HEAD" "@" "newbranch"
+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..305b4a0c4f 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -12,6 +12,7 @@ test_expect_success PERL 'setup' '
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
+	git branch newbranch &&
 	set_and_save_state bar bar_work bar_index &&
 	save_head
 '
@@ -44,13 +45,19 @@ 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
-'
+# Note: 'newbranch' points to the same commit as HEAD. And '@' is a
+# shortcut for HEAD.
+for opt in "HEAD" "@" "newbranch"
+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..65a8802b29 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -13,6 +13,7 @@ test_expect_success PERL 'setup' '
 	git commit -m initial &&
 	test_tick &&
 	test_commit second dir/foo head &&
+	git branch newbranch &&
 	set_and_save_state bar bar_work bar_index &&
 	save_head
 '
@@ -33,6 +34,19 @@ test_expect_success PERL 'git reset -p' '
 	test_grep "Unstage" output
 '
 
+# Note: '@' can technically also be used as a branch name, but in below test
+# it represents the shortcut for HEAD. And 'newbranch' points to the same
+# commit as HEAD.
+for opt in "HEAD" "@" "newbranch"
+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

[RFC PATCH 2/2] checkout: remove HEAD special case

[Ghanshyam Thakkar]

run_add_p() is capable of handling HEAD in any form (e.g. hex, 'HEAD',
'@' etc.), not just string 'HEAD'. Therefore, special casing of HEAD
does not have any effect.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 builtin/checkout.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..6b74e5fa4e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -539,12 +539,11 @@ 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. Also, when we were given a tree-object,
+		 * new_branch_info->commit would be NULL, but we do not
+		 * have to do any replacement.
 		 */
-		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
+		if (rev && new_branch_info->commit)
 			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
 
 		if (opts->checkout_index && opts->checkout_worktree)
-- 
2.43.0

Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3886 bytes --]

On Sun, Jan 28, 2024 at 11:41:22PM +0530, Ghanshyam Thakkar wrote:

We typically start commit messages with an explanation of what the
actual problem is that the commit is trying to solve. This helps to set
the stage for any reviewers so that they know why you're doing changes
in the first place.

> Add a new function reveq(), which takes repository struct and two revision
> strings as arguments and returns 0 if the revisions point to the same
> object. Passing a rev which does not point to an object is considered
> undefined behavior as the underlying function memcmp() will be called
> with NULL hash strings.
> 
> Subsequently, replace literal string comparison to HEAD in run_add_p()
> with reveq() to handle more ways of saying HEAD (such as '@' or '$branch'
> where $branch points to same commit as HEAD). This addresses the
> NEEDSWORK comment in run_add_p().
> 
> However, in ADD_P_RESET mode keep string comparison in logical OR with
> reveq() to handle unborn HEAD.
> 
> As for the behavior change, with this patch applied if the given
> revision points to the same object as HEAD, the patch mode will be set to
> patch_mode_(reset,checkout,worktree)_head instead of
> patch_mode_(...)_nothead. That is equivalent of not setting -R flag in
> diff-index, which would have been otherwise set before this patch.
> However, when given same set of inputs, the actual outcome is same as
> before this patch. Therefore, this does not affect any automated scripts.

So this is the closest to an actual description of what your goal is.
But it doesn't say why that is a good idea, it only explains the change
in behaviour.

I think the best thing to do would be to give a sequence of Git commands
that demonstrate the problem that you are trying to solve. This would
help the reader gain a high-level understanding of what you propose to
change.

> Also, add testcases to check the similarity of result between different
> ways of saying HEAD.
> 
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> Should the return values of repo_get_oid() be checked in reveq()? As
> reveq() is not a global function and is only used in run_add_p(), the
> validity of revisions is already checked beforehand by builtin/checkout.c
> and builtin/reset.c before the call to run_add_p().
> 
>  add-patch.c               | 28 +++++++++++++++-------
>  t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
>  t/t2071-restore-patch.sh  | 21 ++++++++++------
>  t/t7105-reset-patch.sh    | 14 +++++++++++
>  4 files changed, 77 insertions(+), 36 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 79eda168eb..01eb71d90e 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -14,6 +14,7 @@
>  #include "color.h"
>  #include "compat/terminal.h"
>  #include "prompt.h"
> +#include "hash.h"
>  
>  enum prompt_mode_type {
>  	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
> @@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,
>  		     INDEX_ENVIRONMENT "=%s", s->s.r->index_file);
>  }
>  
> +// Check if two revisions point to the same object. Passing a rev which does not
> +// point to an object is undefined behavior.

We only use `/* */`-style comments in the Git codebase.

> +static inline int reveq(struct repository *r, const char *rev1,
> +			const char *rev2)
> +{
> +	struct object_id oid_rev1, oid_rev2;
> +	repo_get_oid(r, rev1, &oid_rev1);
> +	repo_get_oid(r, rev2, &oid_rev2);
> +
> +	return !oideq(&oid_rev1, &oid_rev2);
> +}

I don't think it's a good idea to allow for undefined behaviour here.
While more tedious for the caller, I think it's preferable to handle the
case correctly where revisions don't resolve, e.g. by returning `-1` in
case either of the revisions does not resolve.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 2/2] checkout: remove HEAD special case

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On Sun, Jan 28, 2024 at 11:41:23PM +0530, Ghanshyam Thakkar wrote:
> run_add_p() is capable of handling HEAD in any form (e.g. hex, 'HEAD',
> '@' etc.), not just string 'HEAD'. Therefore, special casing of HEAD
> does not have any effect.

Are there any tests that cover this behaviour? If so, it would be nice
to point them out in the commit message. Otherwise, we should add them.

Patrick

> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>  builtin/checkout.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a6e30931b5..6b74e5fa4e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -539,12 +539,11 @@ 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. Also, when we were given a tree-object,
> +		 * new_branch_info->commit would be NULL, but we do not
> +		 * have to do any replacement.
>  		 */
> -		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
> +		if (rev && new_branch_info->commit)
>  			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
>  
>  		if (opts->checkout_index && opts->checkout_worktree)
> -- 
> 2.43.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> Add a new function reveq(), which takes repository struct and two revision
> strings as arguments and returns 0 if the revisions point to the same
> object. Passing a rev which does not point to an object is considered
> undefined behavior as the underlying function memcmp() will be called
> with NULL hash strings.

I didn't dug into the patch or the codepath it touches, but
I wonder if it has something (possibly historical) to do with the
fact that these two behave quite differently:

    $ git checkout HEAD
    $ git checkout HEAD^0

With the former, you will stay on your current branch, while with
the latter you detach at the commit at the tip of your current
branch.  Granted that "git checkout -p" is not about moving HEAD but
checking out some files to the worktree from a given tree-ish, anytime
I see code that does strcmp() with a fixed "HEAD" string, that is
one consideration I'd look for.

> Should the return values of repo_get_oid() be checked in reveq()? As
> reveq() is not a global function and is only used in run_add_p(), the
> validity of revisions is already checked beforehand by builtin/checkout.c
> and builtin/reset.c before the call to run_add_p().

If this were to become a public function (even if it somehow turns
out that it is a bad idea to move away from an explicit comparison
with "HEAD", introducing such a function might be useful---I dunno),
it probably makes sense not to burden its potential callers with too
many assumption.  But doesn't the fact that the immediate callers
you are introducing already checked the validity of the revisions
tell us something?  Would it result in us not needing this new
helper function at all, if we rearranged the code that already
checks the validity so that the actual object names are collected?
Then it would become the matter of running oideq() directly on these
object names, instead of calling a new helper function that
(re)converts from strings to object names and compares them.

> +// Check if two revisions point to the same object. Passing a rev which does not
> +// point to an object is undefined behavior.

Style:

    /*
     * Our multi-line comments look
     * like this.
     */

> +static inline int reveq(struct repository *r, const char *rev1,
> +			const char *rev2)
> +{
> +	struct object_id oid_rev1, oid_rev2;
> +	repo_get_oid(r, rev1, &oid_rev1);
> +	repo_get_oid(r, rev2, &oid_rev2);
> +
> +	return !oideq(&oid_rev1, &oid_rev2);

Horribly confusing.  If oideq() says "yes, they are the same" by
returning 0, then any helper function derived from it to ansewr "are
X and Y the same?" should return 0 when it wants to say "yes, they
are the same" to help developers keep their sanity.

> +}
> +
>  static int parse_range(const char **p,
>  		       unsigned long *offset, unsigned long *count)
>  {
> @@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  		s.mode = &patch_mode_stash;
>  	else if (mode == ADD_P_RESET) {
>  		/*
> -		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
> -		 * compare the commit objects instead so that other ways of
> -		 * saying the same thing (such as "@") are also handled
> -		 * appropriately.
> -		 *
> -		 * This applies to the cases below too.
> +		 * The literal string comparison to HEAD below is kept
> +		 * to handle unborn HEAD.
>  		 */

So, does this change solve the NEEDSWORK comment?  On an unborn
HEAD, this would still not allow you to say "@".  Only "HEAD" is
supported.

Not that I necessarily agree with the original "NEEDSWORK" comment
(I think it is perfectly fine for this or any other codepaths not to
take "@" as "HEAD"), but if that desire still stands here, should
the resulting comment still mention it with a NEEDSWORK label?

Besides ...

> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 747eb5563e..431f34fa9c 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -12,6 +12,7 @@ test_expect_success 'setup' '
>  	git commit -m initial &&
>  	test_tick &&
>  	test_commit second dir/foo head &&
> +	git branch newbranch &&
>  	set_and_save_state bar bar_work bar_index &&
>  	save_head
>  '
> +# Note: 'newbranch' points to the same commit as HEAD. And it is technically
> +# allowed to name a branch '@' as of now, however in below test '@'
> +# represents the shortcut for HEAD.
> +for opt in "HEAD" "@" "newbranch"
> +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
> +	'

I think this change in behaviour, especially for "newbranch" that
used to use the "_nothead" variants of directions and messages, is
way too confusing.  Users may consider "HEAD" and "@" the same and
may want them to behave the same way, but the user, when explicitly
naming "newbranch", means they want to "check contents out of that
OTHER thing named 'newbranch', not the current branch"; it may or
may not happen to be pointing at the same commit as HEAD, but if
the user meant to say "check contents out of the current commit,
(partially) reverting the local changes I have", the user would have
said HEAD.  After all, the user may not even be immediately aware
that "newbranch" happens to point at the same commit as HEAD.

So, after thinking about it a bit more, I do not think I agree with
the NEEDSWORK comment.  I can buy "@", but not an arbitrary revision
name that happens to point at the same commit as HEAD.  In other
words, I may be persuaded to thinking into it is a good idea to add:

    static inline int user_means_HEAD(const char *a)
    {
	return !strcmp(a, "HEAD") || !strcmp(a, "@");
    }

and replace "!strcmp(rev, "HEAD")" with "user_means_HEAD(rev)", but
I would not go any further than that.

Thanks.

Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So, after thinking about it a bit more, I do not think I agree with
> the NEEDSWORK comment.  I can buy "@", but not an arbitrary revision
> name that happens to point at the same commit as HEAD.  

One more thing is it might make sense, if we were to allow more than
the literal string "HEAD", is to include the name of the current
branch (e.g., if "git symbolic-ref HEAD" says "refs/heads/main",
then "main") to the set of tokens that the user may use when they
mean to refer to "HEAD".  Unlike "newbranch" they are not currently
on, if they know what branch they are on and they know that is what
HEAD refers to, so the likelihood of them wanting to see the command
behave (i.e. the direction of the patch to be selected and the
messages) the same way may be much higher, I would suspect.

But still, the sudden reversal of the direction of the patches may
bring unexpected confusions to uses.  I dunno.

> In other
> words, I may be persuaded to thinking into it is a good idea to add:
>
>     static inline int user_means_HEAD(const char *a)
>     {
> 	return !strcmp(a, "HEAD") || !strcmp(a, "@");
>     }
>
> and replace "!strcmp(rev, "HEAD")" with "user_means_HEAD(rev)", but
> I would not go any further than that.
>
> Thanks.

Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string

[Ghanshyam Thakkar]

On Mon Jan 29, 2024 at 11:57 PM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > Add a new function reveq(), which takes repository struct and two revision
> > strings as arguments and returns 0 if the revisions point to the same
> > object. Passing a rev which does not point to an object is considered
> > undefined behavior as the underlying function memcmp() will be called
> > with NULL hash strings.
>
> I didn't dug into the patch or the codepath it touches, but
> I wonder if it has something (possibly historical) to do with the
> fact that these two behave quite differently:
>
>     $ git checkout HEAD
>     $ git checkout HEAD^0
>
> With the former, you will stay on your current branch, while with
> the latter you detach at the commit at the tip of your current
> branch.  Granted that "git checkout -p" is not about moving HEAD but
> checking out some files to the worktree from a given tree-ish, anytime
> I see code that does strcmp() with a fixed "HEAD" string, that is
> one consideration I'd look for.
>
> > Should the return values of repo_get_oid() be checked in reveq()? As
> > reveq() is not a global function and is only used in run_add_p(), the
> > validity of revisions is already checked beforehand by builtin/checkout.c
> > and builtin/reset.c before the call to run_add_p().
>
> If this were to become a public function (even if it somehow turns
> out that it is a bad idea to move away from an explicit comparison
> with "HEAD", introducing such a function might be useful---I dunno),
> it probably makes sense not to burden its potential callers with too
> many assumption.  But doesn't the fact that the immediate callers
> you are introducing already checked the validity of the revisions
> tell us something?  Would it result in us not needing this new
> helper function at all, if we rearranged the code that already
> checks the validity so that the actual object names are collected?
> Then it would become the matter of running oideq() directly on these
> object names, instead of calling a new helper function that
> (re)converts from strings to object names and compares them.
>
> > +// Check if two revisions point to the same object. Passing a rev which does not
> > +// point to an object is undefined behavior.
>
> Style:
>
>     /*
>      * Our multi-line comments look
>      * like this.
>      */
>
> > +static inline int reveq(struct repository *r, const char *rev1,
> > +			const char *rev2)
> > +{
> > +	struct object_id oid_rev1, oid_rev2;
> > +	repo_get_oid(r, rev1, &oid_rev1);
> > +	repo_get_oid(r, rev2, &oid_rev2);
> > +
> > +	return !oideq(&oid_rev1, &oid_rev2);
>
> Horribly confusing.  If oideq() says "yes, they are the same" by
> returning 0, then any helper function derived from it to ansewr "are
> X and Y the same?" should return 0 when it wants to say "yes, they
> are the same" to help developers keep their sanity.
>
> > +}
> > +
> >  static int parse_range(const char **p,
> >  		       unsigned long *offset, unsigned long *count)
> >  {
> > @@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> >  		s.mode = &patch_mode_stash;
> >  	else if (mode == ADD_P_RESET) {
> >  		/*
> > -		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
> > -		 * compare the commit objects instead so that other ways of
> > -		 * saying the same thing (such as "@") are also handled
> > -		 * appropriately.
> > -		 *
> > -		 * This applies to the cases below too.
> > +		 * The literal string comparison to HEAD below is kept
> > +		 * to handle unborn HEAD.
> >  		 */
>
> So, does this change solve the NEEDSWORK comment?  On an unborn
> HEAD, this would still not allow you to say "@".  Only "HEAD" is
> supported.

The reset command does not support naming anything on the unborn HEAD.
Meaning, on an unborn HEAD, using both 'git reset -p @' and 'git reset
-p HEAD' error out. However in case of 'git reset -p' on an unborn
HEAD, it works becuase it skips the validity checks for the revision
string in parse_args() in builtin/reset.c due to the absense of any
arguments. Afterwards the empty revision is replaced by 'HEAD'.
Therefore, the string comparison to HEAD is not for supporting
HEAD, but it is an indication to parse_diff() in add-patch.c, which
replaces that 'HEAD' with empty_tree_oid_hex().

In short, on unborn HEAD, 'git reset -p' passes 'HEAD' as revision
string to be replaced by empty_tree_oid_hex() in parse_diff().
relevant code lines from add-patch.c:

static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
{
    ...
	if (s->revision) {
		struct object_id oid;
		strvec_push(&args,
			    /* could be on an unborn branch */
			    !strcmp("HEAD", s->revision) &&
			    repo_get_oid(the_repository, "HEAD", &oid) ?
			    empty_tree_oid_hex() : s->revision);
	}
	...
}

Perhaps, I should have clarified that in the commit message or comment.

> Not that I necessarily agree with the original "NEEDSWORK" comment
> (I think it is perfectly fine for this or any other codepaths not to
> take "@" as "HEAD"), but if that desire still stands here, should
> the resulting comment still mention it with a NEEDSWORK label?
>
> Besides ...
>
> > diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> > index 747eb5563e..431f34fa9c 100755
> > --- a/t/t2016-checkout-patch.sh
> > +++ b/t/t2016-checkout-patch.sh
> > @@ -12,6 +12,7 @@ test_expect_success 'setup' '
> >  	git commit -m initial &&
> >  	test_tick &&
> >  	test_commit second dir/foo head &&
> > +	git branch newbranch &&
> >  	set_and_save_state bar bar_work bar_index &&
> >  	save_head
> >  '
> > +# Note: 'newbranch' points to the same commit as HEAD. And it is technically
> > +# allowed to name a branch '@' as of now, however in below test '@'
> > +# represents the shortcut for HEAD.
> > +for opt in "HEAD" "@" "newbranch"
> > +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
> > +	'
>
> I think this change in behaviour, especially for "newbranch" that
> used to use the "_nothead" variants of directions and messages, is
> way too confusing.  Users may consider "HEAD" and "@" the same and
> may want them to behave the same way, but the user, when explicitly
> naming "newbranch", means they want to "check contents out of that
> OTHER thing named 'newbranch', not the current branch"; it may or
> may not happen to be pointing at the same commit as HEAD, but if
> the user meant to say "check contents out of the current commit,
> (partially) reverting the local changes I have", the user would have
> said HEAD.  After all, the user may not even be immediately aware
> that "newbranch" happens to point at the same commit as HEAD.
>
> So, after thinking about it a bit more, I do not think I agree with
> the NEEDSWORK comment.  I can buy "@", but not an arbitrary revision
> name that happens to point at the same commit as HEAD.  In other
> words, I may be persuaded to thinking into it is a good idea to add:
>
>     static inline int user_means_HEAD(const char *a)
>     {
> 	return !strcmp(a, "HEAD") || !strcmp(a, "@");
>     }
>
> and replace "!strcmp(rev, "HEAD")" with "user_means_HEAD(rev)", but
> I would not go any further than that.

Yes, however, '@' can also be a branch name. And there is also the case
of '@' being a branch which points to same commit as HEAD, which would
again be confusing as you pointed out above, if "_head" variant is used
in that. For this to work, we would need to check if a branch named '@'
exists and if it does, then '@' should not be treated as a shortcut for
HEAD. (or should it still be treated as a shortcut for HEAD? As 'git
push origin @' pushes HEAD to remote inspite of a branch named '@' existing
locally at a different commit than HEAD).

Thanks.

Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string

[Ghanshyam Thakkar]

On Mon Jan 29, 2024 at 5:18 PM IST, Patrick Steinhardt wrote:
> On Sun, Jan 28, 2024 at 11:41:22PM +0530, Ghanshyam Thakkar wrote:
>
> We typically start commit messages with an explanation of what the
> actual problem is that the commit is trying to solve. This helps to set
> the stage for any reviewers so that they know why you're doing changes
> in the first place.

I will keep that in mind for future patches.

> > Add a new function reveq(), which takes repository struct and two revision
> > strings as arguments and returns 0 if the revisions point to the same
> > object. Passing a rev which does not point to an object is considered
> > undefined behavior as the underlying function memcmp() will be called
> > with NULL hash strings.
> > 
> > Subsequently, replace literal string comparison to HEAD in run_add_p()
> > with reveq() to handle more ways of saying HEAD (such as '@' or '$branch'
> > where $branch points to same commit as HEAD). This addresses the
> > NEEDSWORK comment in run_add_p().
> > 
> > However, in ADD_P_RESET mode keep string comparison in logical OR with
> > reveq() to handle unborn HEAD.
> > 
> > As for the behavior change, with this patch applied if the given
> > revision points to the same object as HEAD, the patch mode will be set to
> > patch_mode_(reset,checkout,worktree)_head instead of
> > patch_mode_(...)_nothead. That is equivalent of not setting -R flag in
> > diff-index, which would have been otherwise set before this patch.
> > However, when given same set of inputs, the actual outcome is same as
> > before this patch. Therefore, this does not affect any automated scripts.
>
> So this is the closest to an actual description of what your goal is.
> But it doesn't say why that is a good idea, it only explains the change
> in behaviour.
>
> I think the best thing to do would be to give a sequence of Git commands
> that demonstrate the problem that you are trying to solve. This would
> help the reader gain a high-level understanding of what you propose to
> change.

Yeah, my original motive was to support '@' as a shorthand for HEAD.
But, since '@' can also be used as branch name, I thought of comparing
object ids instead of string comparison in accordance with the
NEEDSWORK comment. However, as Junio pointed out, treating a branch
name revision that points to same commit as HEAD, as HEAD would just
cause confusion.

> > Also, add testcases to check the similarity of result between different
> > ways of saying HEAD.
> > 
> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> > ---
> > Should the return values of repo_get_oid() be checked in reveq()? As
> > reveq() is not a global function and is only used in run_add_p(), the
> > validity of revisions is already checked beforehand by builtin/checkout.c
> > and builtin/reset.c before the call to run_add_p().
> > 
> >  add-patch.c               | 28 +++++++++++++++-------
> >  t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
> >  t/t2071-restore-patch.sh  | 21 ++++++++++------
> >  t/t7105-reset-patch.sh    | 14 +++++++++++
> >  4 files changed, 77 insertions(+), 36 deletions(-)
> > 
> > diff --git a/add-patch.c b/add-patch.c
> > index 79eda168eb..01eb71d90e 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -14,6 +14,7 @@
> >  #include "color.h"
> >  #include "compat/terminal.h"
> >  #include "prompt.h"
> > +#include "hash.h"
> >  
> >  enum prompt_mode_type {
> >  	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
> > @@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,
> >  		     INDEX_ENVIRONMENT "=%s", s->s.r->index_file);
> >  }
> >  
> > +// Check if two revisions point to the same object. Passing a rev which does not
> > +// point to an object is undefined behavior.
>
> We only use `/* */`-style comments in the Git codebase.
>
> > +static inline int reveq(struct repository *r, const char *rev1,
> > +			const char *rev2)
> > +{
> > +	struct object_id oid_rev1, oid_rev2;
> > +	repo_get_oid(r, rev1, &oid_rev1);
> > +	repo_get_oid(r, rev2, &oid_rev2);
> > +
> > +	return !oideq(&oid_rev1, &oid_rev2);
> > +}
>
> I don't think it's a good idea to allow for undefined behaviour here.
> While more tedious for the caller, I think it's preferable to handle the
> case correctly where revisions don't resolve, e.g. by returning `-1` in
> case either of the revisions does not resolve.

Will update it. 

Thanks.

Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string

[Junio C Hamano]

"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:

> Yeah, my original motive was to support '@' as a shorthand for HEAD.
> But, since '@' can also be used as branch name, I thought of comparing
> object ids instead of string comparison in accordance with the
> NEEDSWORK comment. However, as Junio pointed out, treating a branch
> name revision that points to same commit as HEAD, as HEAD would just
> cause confusion.

FWIW, if we are not doing so in our documentation already, we may
want to discourage use of "refs/heads/@", given that "@" is used as
a synonym for "HEAD" in some[*] contexts, specifying the HEAD
(i.e. "work on the branch that is currently checked out, or in the
detached state") and specifying the concrete name of a branch
(i.e. "work on this branch") mean totally different things and may
result in (what may appear to the user as a) confusing behaviour.

Granted, the user who names their branch "@" is only hurting
themselves and it falls into the "Doctor, it hurts when I do
this. Then don't do that!" category.  

But the documentation is where we tell them "Then don't do that!"
and we should know better how it hurts when they do so than those
who learn from the documentation, so ...


[Footnote]

 * Even use of "@" as a synonym for "HEAD" may want to be
   discouraged, as there are still unnecessary differences between
   them that are not worth our engineering resource to fix.  Do
   people know what "git checkout HEAD" and "git checkout @" do, for
   example?

[PATCH v2 0/2] add-patch: Support '@' as a synonym for 'HEAD'

[Ghanshyam Thakkar]

This patch series removes a non-relevant NEEDSWORK comment and addresses
disparity between '@' and 'HEAD' in patch mode for (checkout, restore,
reset) commands.

The removal of the NEEDSWORK comment and the '@' support are split into
different patches because the former is still up for discussion. And if
it is decided against, the NEEDSWORK comment can still go as it would not
be the desired solution anyway (described in the commit message).

Ghanshyam Thakkar (2):
  add-patch: remove unnecessary NEEDSWORK comment
  add-patch: classify '@' as a synonym for 'HEAD'

 add-patch.c               | 19 +++++++---------
 builtin/checkout.c        | 11 +++++-----
 t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------
 t/t2071-restore-patch.sh  | 18 +++++++++------
 t/t7105-reset-patch.sh    | 10 +++++++++
 5 files changed, 61 insertions(+), 43 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] add-patch: remove non-relevant NEEDSWORK comment

[Ghanshyam Thakkar]

The comment suggested to compare commit objects instead of string
comparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.
'@'). However, this approach has unintended behavior of also counting a
non-checked out branch pointing to same commit as HEAD, as HEAD. This
would cause confusion to the user.

Junio described it best as[1]:

"Users may consider 'HEAD' and '@' the same and
may want them to behave the same way, but the user, when explicitly
naming '$branch', means they want to "check contents out of that
OTHER thing named '$branch', not the current branch"; it may or
may not happen to be pointing at the same commit as HEAD, but if
the user meant to say "check contents out of the current commit,
(partially) reverting the local changes I have", the user would have
said HEAD.  After all, the user may not even be immediately aware
that '$branch' happens to point at the same commit as HEAD."

[1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-patch.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..68f525b35c 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,14 +1729,6 @@ 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) {
-		/*
-		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
-		 * compare the commit objects instead so that other ways of
-		 * saying the same thing (such as "@") are also handled
-		 * appropriately.
-		 *
-		 * This applies to the cases below too.
-		 */
 		if (!revision || !strcmp(revision, "HEAD"))
 			s.mode = &patch_mode_reset_head;
 		else
-- 
2.43.0

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

[Ghanshyam Thakkar]

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. 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 user_meant_head() which takes the
revision string and compares it to 'HEAD' as well as '@'. However, in
builtin/checkout.c, there is some logic to convert all revs besides
'HEAD' to hex revs due to 'diff-index', which is used in patch mode
machinery, not being able to handle '<a>...<b>' revs. Therefore, in
addition to 'HEAD', do not convert '@' as well, so it can be later
used in assigning patch_mode_(...)_head.

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 take '@' for 'HEAD' even if 'refs/heads/@' exists (e.g., 'git log
@', 'git push origin @' etc.). Therefore, this should be fine.

Also, add tests to check the above mentioned synonymity between 'HEAD'
and '@'.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-patch.c               | 11 +++++++---
 builtin/checkout.c        | 11 +++++-----
 t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------
 t/t2071-restore-patch.sh  | 18 +++++++++------
 t/t7105-reset-patch.sh    | 10 +++++++++
 5 files changed, 61 insertions(+), 35 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..6c70a0240c 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	return 0;
 }
 
+static inline int is_rev_head(const char *rev)
+{
+	return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
+}
+
 static int is_octal(const char *p, size_t len)
 {
 	if (!len)
@@ -1729,21 +1734,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 || is_rev_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 (is_rev_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 (is_rev_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..79e208ee6d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -539,12 +539,13 @@ 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 && strcmp(rev, "HEAD") &&
+		    strcmp(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

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

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> 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. 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 user_meant_head() which takes the
> revision string and compares it to 'HEAD' as well as '@'. However, in
> builtin/checkout.c, there is some logic to convert all revs besides
> 'HEAD' to hex revs due to 'diff-index', which is used in patch mode
> machinery, not being able to handle '<a>...<b>' revs. Therefore, in
> addition to 'HEAD', do not convert '@' as well, so it can be later
> used in assigning patch_mode_(...)_head.

In this context <a>...<b> names a single rev (not two revs) that is
the merge base of <a> and <b>.  Perhaps

    ... 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, too.

which may be a bit shorter.

You decided to use is_rev_head() instead of user_meant_head(), so
you'd need to update the above description to match, I think.

> -		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
> +		if (rev && new_branch_info->commit && strcmp(rev, "HEAD") &&
> +		    strcmp(rev, "@"))

Shouldn't this be

		if (rev && new_branch_info->commit && !is_rev_head(rev))

instead of "HEAD" and "@" spelled out?

Other than the above, nicely done.

Re: [PATCH v2 0/2] add-patch: Support '@' as a synonym for 'HEAD'

[Ghanshyam Thakkar]

On Fri Feb 2, 2024 at 8:33 PM IST, Ghanshyam Thakkar wrote:
> This patch series removes a non-relevant NEEDSWORK comment and addresses
> disparity between '@' and 'HEAD' in patch mode for (checkout, restore,
> reset) commands.
>
> The removal of the NEEDSWORK comment and the '@' support are split into
> different patches because the former is still up for discussion. And if
s/former/latter/

> it is decided against, the NEEDSWORK comment can still go as it would not
> be the desired solution anyway (described in the commit message).

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> You decided to use is_rev_head() instead of user_meant_head(), so
> you'd need to update the above description to match, I think.

Having said this, I have a slight fear that normal users would
expect is_rev_head(X) to say "yes" for "master" when the current
branch is "master", though.  is_head(X) would have the same
downside.

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

[Ghanshyam Thakkar]

On Fri Feb 2, 2024 at 10:38 PM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > 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. 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 user_meant_head() which takes the
> > revision string and compares it to 'HEAD' as well as '@'. However, in
> > builtin/checkout.c, there is some logic to convert all revs besides
> > 'HEAD' to hex revs due to 'diff-index', which is used in patch mode
> > machinery, not being able to handle '<a>...<b>' revs. Therefore, in
> > addition to 'HEAD', do not convert '@' as well, so it can be later
> > used in assigning patch_mode_(...)_head.
>
> In this context <a>...<b> names a single rev (not two revs) that is
> the merge base of <a> and <b>.  Perhaps
I meant revs which are spelled out in the form of <a>...<b> and not the
<a> and <b> themselves. But I can see how it can be confusing. I will
change it.

>     ... 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, too.
>
> which may be a bit shorter.
Yeah, that is much better and clearer also.

> You decided to use is_rev_head() instead of user_meant_head(), so
> you'd need to update the above description to match, I think.
Will update.

> > -		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
> > +		if (rev && new_branch_info->commit && strcmp(rev, "HEAD") &&
> > +		    strcmp(rev, "@"))
>
> Shouldn't this be
>
> 		if (rev && new_branch_info->commit && !is_rev_head(rev))
>
> instead of "HEAD" and "@" spelled out?
is_rev_head() is in add-patch.c and the above is in
builtin/checkout.c and is_rev_head() is not exported. I can also define
it in builtin/checkout.c, but this would be the only instance in that
file which will use it. So, I think it is better to just add
strcmp(rev, "@") to the if condition.

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

[Ghanshyam Thakkar]

On Fri Feb 2, 2024 at 11:13 PM IST, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > You decided to use is_rev_head() instead of user_meant_head(), so
> > you'd need to update the above description to match, I think.
>
> Having said this, I have a slight fear that normal users would
> expect is_rev_head(X) to say "yes" for "master" when the current
> branch is "master", though.  is_head(X) would have the same
> downside.
Yeah, user_meant_head() looks like the better pick. I'll update it.

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

[Ghanshyam Thakkar]

The v3 of the series updates the commit message for patch [2/2] and
replaces the function name from is_rev_head() to user_meant_head().

Patch [1/2] remains unchanged.

Ghanshyam Thakkar (2):
  add-patch: remove unnecessary NEEDSWORK comment
  add-patch: classify '@' as a synonym for 'HEAD'

 add-patch.c               | 19 +++++++---------
 builtin/checkout.c        | 11 +++++-----
 t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------
 t/t2071-restore-patch.sh  | 18 +++++++++------
 t/t7105-reset-patch.sh    | 10 +++++++++
 5 files changed, 61 insertions(+), 43 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] add-patch: remove unnecessary NEEDSWORK comment

[Ghanshyam Thakkar]

The comment suggested to compare commit objects instead of string
comparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.
'@'). However, this approach would also count a non-checked out branch
pointing to same commit as HEAD, as HEAD. This would cause confusion to
the user.

Junio described it best as[1]:

"Users may consider 'HEAD' and '@' the same and may want them to behave
the same way, but the user, when explicitly naming '$branch', means they
want to "check contents out of that OTHER thing named '$branch', not the
current branch"; it may or may not happen to be pointing at the same
commit as HEAD, but if the user meant to say "check contents out of the
current commit, (partially) reverting the local changes I have", the user
would have said HEAD.  After all, the user may not even be immediately
aware that '$branch' happens to point at the same commit as HEAD."

[1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-patch.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..68f525b35c 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,14 +1729,6 @@ 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) {
-		/*
-		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
-		 * compare the commit objects instead so that other ways of
-		 * saying the same thing (such as "@") are also handled
-		 * appropriately.
-		 *
-		 * This applies to the cases below too.
-		 */
 		if (!revision || !strcmp(revision, "HEAD"))
 			s.mode = &patch_mode_reset_head;
 		else
-- 
2.43.0

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

[Ghanshyam Thakkar]

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

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 take '@' for 'HEAD' even if 'refs/heads/@' exists (e.g., 'git log
@', 'git push origin @' etc.). Therefore, this should be fine.

Also, add tests to check the above mentioned synonymity between 'HEAD'
and '@'.

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

diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..7d565dcb33 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	return 0;
 }
 
+static inline int user_meant_head(const char *rev)
+{
+	return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
+}
+
 static int is_octal(const char *p, size_t len)
 {
 	if (!len)
@@ -1729,21 +1734,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 || 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 (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 (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..79e208ee6d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -539,12 +539,13 @@ 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 && strcmp(rev, "HEAD") &&
+		    strcmp(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

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

[Junio C Hamano]

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

	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.

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.

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.

 add-interactive.h  | 14 ++++++++++++++
 add-patch.c        | 11 +++--------
 builtin/checkout.c |  3 +--
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git c/add-interactive.h w/add-interactive.h
index 693f125e8e..ca7326336d 100644
--- c/add-interactive.h
+++ w/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 c/add-patch.c w/add-patch.c
index 7d565dcb33..5502acebb8 100644
--- c/add-patch.c
+++ w/add-patch.c
@@ -378,11 +378,6 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
 	return 0;
 }
 
-static inline int user_meant_head(const char *rev)
-{
-	return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
-}
-
 static int is_octal(const char *p, size_t len)
 {
 	if (!len)
@@ -1734,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 || user_meant_head(revision))
+		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 (user_meant_head(revision))
+		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 (user_meant_head(revision))
+		else if (the_user_meant_head(revision))
 			s.mode = &patch_mode_worktree_head;
 		else
 			s.mode = &patch_mode_worktree_nothead;
diff --git c/builtin/checkout.c w/builtin/checkout.c
index 79e208ee6d..63c669b157 100644
--- c/builtin/checkout.c
+++ w/builtin/checkout.c
@@ -544,8 +544,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 		 * 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") &&
-		    strcmp(rev, "@"))
+		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)

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

[Ghanshyam Thakkar]

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

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

[Phillip Wood]

Hi Ghanshyam

I think this is a useful addition, I've left a couple of comments below

On 03/02/2024 11:25, Ghanshyam Thakkar wrote:
> diff --git a/add-patch.c b/add-patch.c
> index 68f525b35c..7d565dcb33 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
>   	return 0;
>   }
>   
> +static inline int user_meant_head(const char *rev)
> +{
> +	return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
> +}
> +

As well as the places you have converted we also have an explicit test 
for "HEAD" in parse_diff() which looks like

	if (s->revision) {
		struct object_id oid;
		strvec_push(&args,
			    /* could be on an unborn branch */
			    !strcmp("HEAD", s->revision) &&
			    repo_get_oid(the_repository, "HEAD", &oid) ?
			    empty_tree_oid_hex() : s->revision);
	}

I suspect we need to update that code as well to accept "@" as a synonym 
for "HEAD" on an unborn branch.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a6e30931b5..79e208ee6d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -539,12 +539,13 @@ 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 && strcmp(rev, "HEAD") &&
> +		    strcmp(rev, "@"))
>   			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);

I agree with Junio's suggestion to use the user_meant_head() here. 
Looking at this made me wonder why builtin/reset.c does not need 
updating. The answer seems to be that reset passes in the revision as 
given on the commandline after checking it refers to a valid tree 
whereas for checkout the revision for on the commandline might contain 
"..." which run_add_p() cannot handle.
> 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' '

It is a pre-existing problem but all these "PERL" prerequisites are 
no-longer required as we've removed the perl implementation of "add -p"

>   	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

It is good to see that we're now testing for a reversed patch here.

Best Wishes

Phillip

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

[Ghanshyam Thakkar]

On Mon Feb 5, 2024 at 10:07 PM IST, Phillip Wood wrote:
> On 03/02/2024 11:25, Ghanshyam Thakkar wrote:
> > diff --git a/add-patch.c b/add-patch.c
> > index 68f525b35c..7d565dcb33 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)
> >   	return 0;
> >   }
> >   
> > +static inline int user_meant_head(const char *rev)
> > +{
> > +	return !strcmp(rev, "HEAD") || !strcmp(rev, "@");
> > +}
> > +
>
> As well as the places you have converted we also have an explicit test 
> for "HEAD" in parse_diff() which looks like
>
> 	if (s->revision) {
> 		struct object_id oid;
> 		strvec_push(&args,
> 			    /* could be on an unborn branch */
> 			    !strcmp("HEAD", s->revision) &&
> 			    repo_get_oid(the_repository, "HEAD", &oid) ?
> 			    empty_tree_oid_hex() : s->revision);
> 	}
>
> I suspect we need to update that code as well to accept "@" as a synonym 
> for "HEAD" on an unborn branch.
I had already considered that. Updating here will not have any effect,
because on unborn branch, we do not allow naming HEAD or @. This case is
for when we run without naming any revision (i.e. git reset -p) on
unborn branch. In such cases, we pass 'HEAD' as a default value.
>
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index a6e30931b5..79e208ee6d 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -539,12 +539,13 @@ 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 && strcmp(rev, "HEAD") &&
> > +		    strcmp(rev, "@"))
> >   			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
>
> I agree with Junio's suggestion to use the user_meant_head() here. 
> Looking at this made me wonder why builtin/reset.c does not need 
> updating. The answer seems to be that reset passes in the revision as 
> given on the commandline after checking it refers to a valid tree 
> whereas for checkout the revision for on the commandline might contain 
> "..." which run_add_p() cannot handle.
I was not able to run reset with '...'. I ran,
'git reset main...$ANOTHERBRANCH'
but it gave me "fatal: ambiguous argument 'main...$ANOTHERBRANCH'"
error, with or without -p. While,
'git restore --source=main...$ANOTHERBRANCH .' and 
'git checkout main...$ANOTHERBRANCH' works fine, with or without -p.

> > 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' '
>
> It is a pre-existing problem but all these "PERL" prerequisites are 
> no-longer required as we've removed the perl implementation of "add -p"
I can send a separate patch to clean up this script, removing PERL
pre-req from all tests.

> >   	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
>
> It is good to see that we're now testing for a reversed patch here.
>
> Best Wishes
>
> Phillip

Thanks for the review.

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> As well as the places you have converted we also have an explicit test
> for "HEAD" in parse_diff() which looks like
>
> 	if (s->revision) {
> 		struct object_id oid;
> 		strvec_push(&args,
> 			    /* could be on an unborn branch */
> 			    !strcmp("HEAD", s->revision) &&
> 			    repo_get_oid(the_repository, "HEAD", &oid) ?
> 			    empty_tree_oid_hex() : s->revision);
> 	}
>
> I suspect we need to update that code as well to accept "@" as a
> synonym for "HEAD" on an unborn branch.

I actually think "@" in the input should be normalized to "HEAD" as
soon as possible.  After the if/elseif/ cascade in run_add_p() the
patch in this thread touches, there is an assignment

	s.revision = revision;

and even though we rewrite !strcmp(revision, "HEAD") to "user means
HEAD" to additionally accept "@" in that if/elseif/ cascade, here we
will stuff different values to s.revision here.  We could normalize
the end-user supplied "@" to "HEAD" before making this assignment,
then you do not have to worry about the code in parse_diff() above,
and more importantly, we do not have to rely on what the current set
of callers happen to do and do not happen to do (e.g., "git reset
-p" happens to use hardcoded "HEAD" for unborn case without using
anything obtained from the user, so the above code in parse_diff()
might be safe in that case, but we do not want to rely on such
subtlety to make sure our code is correct)

Come to think of it, we could even do the "normalizing" even before,
and that might greatly simplify things.  For example, if we did so
at the very beginning of run_add_p(), before we come to that if/else
if/ cascade, we may not even have to worry about the "user meant HEAD"
helper.  After the normalization, we can just keep using !strcmp()
with "HEAD" alone.  Simple and clean, no?

Thanks.

[PATCH v4 0/3] '@' as a synonym for 'HEAD' in patch mode

[Ghanshyam Thakkar]

Since v3, I have taken Junio's suggestion to replace '@' with 'HEAD'
at the beginning of run_add_p(). Due to this, I no longer think we
need to have/export the_user_meant_head() function just for the single
instance in builtin/checkout.c. Therefore, I have hardcoded the '@'
condition in builtin/checkout.c.

Also, Phillip mentioned that the PERL prerequisites in the test files
that are touched by this series are unnecessary and therefore I have
attached a patch to remove them.

Ghanshyam Thakkar (3):
  add-patch: remove unnecessary NEEDSWORK comment
  add-patch: classify '@' as a synonym for 'HEAD'
  add -p tests: remove Perl prerequisite

 add-patch.c               | 12 ++++------
 builtin/checkout.c        | 11 +++++-----
 t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------
 t/t2071-restore-patch.sh  | 46 +++++++++++++++++++++------------------
 t/t7105-reset-patch.sh    | 32 +++++++++++++++++----------
 5 files changed, 82 insertions(+), 65 deletions(-)

-- 
2.43.0

[PATCH v4 1/3] add-patch: remove unnecessary NEEDSWORK comment

[Ghanshyam Thakkar]

The comment suggested to compare commit objects instead of string
comparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.
'@'). However, this approach would also count a non-checked out branch
pointing to same commit as HEAD, as HEAD. This would cause confusion to
the user.

Junio described it best as[1]:

"Users may consider 'HEAD' and '@' the same and
may want them to behave the same way, but the user, when explicitly
naming '$branch', means they want to "check contents out of that
OTHER thing named '$branch', not the current branch"; it may or
may not happen to be pointing at the same commit as HEAD, but if
the user meant to say "check contents out of the current commit,
(partially) reverting the local changes I have", the user would have
said HEAD.  After all, the user may not even be immediately aware
that '$branch' happens to point at the same commit as HEAD."

[1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-patch.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..68f525b35c 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,14 +1729,6 @@ 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) {
-		/*
-		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
-		 * compare the commit objects instead so that other ways of
-		 * saying the same thing (such as "@") are also handled
-		 * appropriately.
-		 *
-		 * This applies to the cases below too.
-		 */
 		if (!revision || !strcmp(revision, "HEAD"))
 			s.mode = &patch_mode_reset_head;
 		else
-- 
2.43.0

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

[Ghanshyam Thakkar]

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, replace '@' to 'HEAD' at the beginning of
add-patch.c:run_add_p(). There is also logic in builtin/checkout.c 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.

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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-patch.c               |  4 ++++
 builtin/checkout.c        | 11 +++++-----
 t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------
 t/t2071-restore-patch.sh  | 18 +++++++++------
 t/t7105-reset-patch.sh    | 10 +++++++++
 5 files changed, 57 insertions(+), 32 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 68f525b35c..6f4ca8f4e4 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1726,6 +1726,10 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 
 	init_add_i_state(&s.s, r);
 
+	/* helpful in deciding the patch mode ahead */
+	if(revision && !strcmp(revision, "@"))
+		revision = "HEAD";
+
 	if (mode == ADD_P_STASH)
 		s.mode = &patch_mode_stash;
 	else if (mode == ADD_P_RESET) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..79e208ee6d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -539,12 +539,13 @@ 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 && strcmp(rev, "HEAD") &&
+		    strcmp(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..dbbefc188d 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 "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..7147148138 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 "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

[PATCH v4 3/3] add -p tests: remove Perl prerequisite

[Ghanshyam Thakkar]

The Perl version of the add -i/-p commands has been removed since
20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
add--interactive", 2023-02-07)

Therefore, Perl prerequisite in t2071-restore-patch and
t7105-reset-patch is not necessary.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t2071-restore-patch.sh | 26 +++++++++++++-------------
 t/t7105-reset-patch.sh   | 22 +++++++++++-----------
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index dbbefc188d..27e85be40a 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -4,7 +4,7 @@ test_description='git restore --patch'
 
 . ./lib-patch-mode.sh
 
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent >dir/foo &&
 	echo dummy >bar &&
@@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '
 	save_head
 '
 
-test_expect_success PERL 'restore -p without pathspec is fine' '
+test_expect_success 'restore -p without pathspec is fine' '
 	echo q >cmd &&
 	git restore -p <cmd
 '
 
 # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
 
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n n | git restore -p &&
 	verify_saved_state bar &&
 	verify_saved_state dir/foo
 '
 
-test_expect_success PERL 'git restore -p' '
+test_expect_success 'git restore -p' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n y | git restore -p &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'git restore -p with staged changes' '
+test_expect_success 'git restore -p with staged changes' '
 	set_state dir/foo work index &&
 	test_write_lines n y | git restore -p &&
 	verify_saved_state bar &&
@@ -56,7 +56,7 @@ do
 	'
 done
 
-test_expect_success PERL 'git restore -p --source=HEAD^' '
+test_expect_success '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^ &&
@@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '
 	verify_state dir/foo parent index
 '
 
-test_expect_success PERL 'git restore -p --source=HEAD^...' '
+test_expect_success '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^... &&
@@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '
 	verify_state dir/foo parent index
 '
 
-test_expect_success PERL 'git restore -p handles deletion' '
+test_expect_success 'git restore -p handles deletion' '
 	set_state dir/foo work index &&
 	rm dir/foo &&
 	test_write_lines n y | git restore -p &&
@@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'path limiting works: dir' '
+test_expect_success 'path limiting works: dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git restore -p dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: -- dir' '
+test_expect_success 'path limiting works: -- dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git restore -p -- dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
+test_expect_success 'path limiting works: HEAD^ -- dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | git restore -p --source=HEAD^ -- dir &&
@@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
 	verify_state dir/foo parent head
 '
 
-test_expect_success PERL 'path limiting works: foo inside dir' '
+test_expect_success 'path limiting works: foo inside dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | (cd dir && git restore -p foo) &&
@@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 7147148138..3691b94d1b 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -5,7 +5,7 @@ test_description='git reset --patch'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
 	echo dummy > bar &&
@@ -19,14 +19,14 @@ test_expect_success PERL 'setup' '
 
 # note: bar sorts before foo, so the first 'n' is always to skip 'bar'
 
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
 	set_and_save_state dir/foo work work &&
 	test_write_lines n n | git reset -p &&
 	verify_saved_state dir/foo &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p' '
+test_expect_success 'git reset -p' '
 	test_write_lines n y | git reset -p >output &&
 	verify_state dir/foo work head &&
 	verify_saved_state bar &&
@@ -43,28 +43,28 @@ do
 	'
 done
 
-test_expect_success PERL 'git reset -p HEAD^' '
+test_expect_success 'git reset -p HEAD^' '
 	test_write_lines n y | git reset -p HEAD^ >output &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar &&
 	test_grep "Apply" output
 '
 
-test_expect_success PERL 'git reset -p HEAD^^{tree}' '
+test_expect_success 'git reset -p HEAD^^{tree}' '
 	test_write_lines n y | git reset -p HEAD^^{tree} >output &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar &&
 	test_grep "Apply" output
 '
 
-test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
+test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '
 	set_and_save_state dir/foo work work &&
 	test_must_fail git reset -p HEAD^:dir/foo &&
 	verify_saved_state dir/foo &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
+test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '
 	set_and_save_state dir/foo work work &&
 	test_must_fail git reset -p aaaaaaaa &&
 	verify_saved_state dir/foo &&
@@ -76,27 +76,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'git reset -p dir' '
+test_expect_success 'git reset -p dir' '
 	set_state dir/foo work work &&
 	test_write_lines y n | git reset -p dir &&
 	verify_state dir/foo work head &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p -- foo (inside dir)' '
+test_expect_success 'git reset -p -- foo (inside dir)' '
 	set_state dir/foo work work &&
 	test_write_lines y n | (cd dir && git reset -p -- foo) &&
 	verify_state dir/foo work head &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p HEAD^ -- dir' '
+test_expect_success 'git reset -p HEAD^ -- dir' '
 	test_write_lines y n | git reset -p HEAD^ -- dir &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
-- 
2.43.0

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

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> 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, replace '@' to 'HEAD' at the beginning of
> add-patch.c:run_add_p().

Of course there is only one possible downside for this approach, in
that if we are using "revision" in an error message, users who asked
for "@" may complain when an error message says "HEAD" in it.  I think
the simplicity of the implementation far outweighs this downside.

> There is also logic in builtin/checkout.c 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.

Makes me wonder why we cannot use the same "normalize @ to HEAD
upfront" approach here, though?

It would involve translating "@" given to new_branch_info->name to
"HEAD" early, possibly in setup_new_branch_info_and_source_tree(),
and that probably will fix the other strcmp() with "HEAD" that
appears in builtin/checkout.c:update_refs_for_switch() as well, no?

> +	/* helpful in deciding the patch mode ahead */
> +	if(revision && !strcmp(revision, "@"))
> +		revision = "HEAD";

Style.  "if (revision ...)"

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

[Phillip Wood]

On 07/02/2024 01:05, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> 
>> 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, replace '@' to 'HEAD' at the beginning of
>> add-patch.c:run_add_p().
> 
> Of course there is only one possible downside for this approach, in
> that if we are using "revision" in an error message, users who asked
> for "@" may complain when an error message says "HEAD" in it.  I think
> the simplicity of the implementation far outweighs this downside.

I agree, if we were replacing the revision the user gave us with a hex 
object id that would be confusing but as "@" is just a shortcut for 
"HEAD" I think replacing it in the error message is fine. It was a good 
idea just to replace "@" with "HEAD", this version is much simpler.

Best Wishes

Phillip

>> There is also logic in builtin/checkout.c 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.
> 
> Makes me wonder why we cannot use the same "normalize @ to HEAD
> upfront" approach here, though?
> 
> It would involve translating "@" given to new_branch_info->name to
> "HEAD" early, possibly in setup_new_branch_info_and_source_tree(),
> and that probably will fix the other strcmp() with "HEAD" that
> appears in builtin/checkout.c:update_refs_for_switch() as well, no?
> 
>> +	/* helpful in deciding the patch mode ahead */
>> +	if(revision && !strcmp(revision, "@"))
>> +		revision = "HEAD";
> 
> Style.  "if (revision ...)"
> 

Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite

[Phillip Wood]

Hi Ghanshyam

On 06/02/2024 22:50, Ghanshyam Thakkar wrote:
 > The Perl version of the add -i/-p commands has been removed since
 > 20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
 > add--interactive", 2023-02-07)
 >
 > Therefore, Perl prerequisite in t2071-restore-patch and
 > t7105-reset-patch is not necessary.

Thanks for adding this patch. If you do re-roll I've just noticed that 
one of the tests in t7106-reset-unborn-branch.sh and another in 
t2024-checkout-dwim.sh still have PERL prerequisites as well. I don't 
think it is worth re-rolling just for that as we can clean them up 
separately if needed.

Best Wishes

Phillip

> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>   t/t2071-restore-patch.sh | 26 +++++++++++++-------------
>   t/t7105-reset-patch.sh   | 22 +++++++++++-----------
>   2 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
> index dbbefc188d..27e85be40a 100755
> --- a/t/t2071-restore-patch.sh
> +++ b/t/t2071-restore-patch.sh
> @@ -4,7 +4,7 @@ test_description='git restore --patch'
>   
>   . ./lib-patch-mode.sh
>   
> -test_expect_success PERL 'setup' '
> +test_expect_success 'setup' '
>   	mkdir dir &&
>   	echo parent >dir/foo &&
>   	echo dummy >bar &&
> @@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '
>   	save_head
>   '
>   
> -test_expect_success PERL 'restore -p without pathspec is fine' '
> +test_expect_success 'restore -p without pathspec is fine' '
>   	echo q >cmd &&
>   	git restore -p <cmd
>   '
>   
>   # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
>   
> -test_expect_success PERL 'saying "n" does nothing' '
> +test_expect_success 'saying "n" does nothing' '
>   	set_and_save_state dir/foo work head &&
>   	test_write_lines n n | git restore -p &&
>   	verify_saved_state bar &&
>   	verify_saved_state dir/foo
>   '
>   
> -test_expect_success PERL 'git restore -p' '
> +test_expect_success 'git restore -p' '
>   	set_and_save_state dir/foo work head &&
>   	test_write_lines n y | git restore -p &&
>   	verify_saved_state bar &&
>   	verify_state dir/foo head head
>   '
>   
> -test_expect_success PERL 'git restore -p with staged changes' '
> +test_expect_success 'git restore -p with staged changes' '
>   	set_state dir/foo work index &&
>   	test_write_lines n y | git restore -p &&
>   	verify_saved_state bar &&
> @@ -56,7 +56,7 @@ do
>   	'
>   done
>   
> -test_expect_success PERL 'git restore -p --source=HEAD^' '
> +test_expect_success '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^ &&
> @@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '
>   	verify_state dir/foo parent index
>   '
>   
> -test_expect_success PERL 'git restore -p --source=HEAD^...' '
> +test_expect_success '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^... &&
> @@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '
>   	verify_state dir/foo parent index
>   '
>   
> -test_expect_success PERL 'git restore -p handles deletion' '
> +test_expect_success 'git restore -p handles deletion' '
>   	set_state dir/foo work index &&
>   	rm dir/foo &&
>   	test_write_lines n y | git restore -p &&
> @@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '
>   # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
>   # the failure case (and thus get out of the loop).
>   
> -test_expect_success PERL 'path limiting works: dir' '
> +test_expect_success 'path limiting works: dir' '
>   	set_state dir/foo work head &&
>   	test_write_lines y n | git restore -p dir &&
>   	verify_saved_state bar &&
>   	verify_state dir/foo head head
>   '
>   
> -test_expect_success PERL 'path limiting works: -- dir' '
> +test_expect_success 'path limiting works: -- dir' '
>   	set_state dir/foo work head &&
>   	test_write_lines y n | git restore -p -- dir &&
>   	verify_saved_state bar &&
>   	verify_state dir/foo head head
>   '
>   
> -test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
> +test_expect_success 'path limiting works: HEAD^ -- dir' '
>   	set_state dir/foo work head &&
>   	# the third n is to get out in case it mistakenly does not apply
>   	test_write_lines y n n | git restore -p --source=HEAD^ -- dir &&
> @@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
>   	verify_state dir/foo parent head
>   '
>   
> -test_expect_success PERL 'path limiting works: foo inside dir' '
> +test_expect_success 'path limiting works: foo inside dir' '
>   	set_state dir/foo work head &&
>   	# the third n is to get out in case it mistakenly does not apply
>   	test_write_lines y n n | (cd dir && git restore -p foo) &&
> @@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
>   	verify_state dir/foo head head
>   '
>   
> -test_expect_success PERL 'none of this moved HEAD' '
> +test_expect_success 'none of this moved HEAD' '
>   	verify_saved_head
>   '
>   
> diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
> index 7147148138..3691b94d1b 100755
> --- a/t/t7105-reset-patch.sh
> +++ b/t/t7105-reset-patch.sh
> @@ -5,7 +5,7 @@ test_description='git reset --patch'
>   TEST_PASSES_SANITIZE_LEAK=true
>   . ./lib-patch-mode.sh
>   
> -test_expect_success PERL 'setup' '
> +test_expect_success 'setup' '
>   	mkdir dir &&
>   	echo parent > dir/foo &&
>   	echo dummy > bar &&
> @@ -19,14 +19,14 @@ test_expect_success PERL 'setup' '
>   
>   # note: bar sorts before foo, so the first 'n' is always to skip 'bar'
>   
> -test_expect_success PERL 'saying "n" does nothing' '
> +test_expect_success 'saying "n" does nothing' '
>   	set_and_save_state dir/foo work work &&
>   	test_write_lines n n | git reset -p &&
>   	verify_saved_state dir/foo &&
>   	verify_saved_state bar
>   '
>   
> -test_expect_success PERL 'git reset -p' '
> +test_expect_success 'git reset -p' '
>   	test_write_lines n y | git reset -p >output &&
>   	verify_state dir/foo work head &&
>   	verify_saved_state bar &&
> @@ -43,28 +43,28 @@ do
>   	'
>   done
>   
> -test_expect_success PERL 'git reset -p HEAD^' '
> +test_expect_success 'git reset -p HEAD^' '
>   	test_write_lines n y | git reset -p HEAD^ >output &&
>   	verify_state dir/foo work parent &&
>   	verify_saved_state bar &&
>   	test_grep "Apply" output
>   '
>   
> -test_expect_success PERL 'git reset -p HEAD^^{tree}' '
> +test_expect_success 'git reset -p HEAD^^{tree}' '
>   	test_write_lines n y | git reset -p HEAD^^{tree} >output &&
>   	verify_state dir/foo work parent &&
>   	verify_saved_state bar &&
>   	test_grep "Apply" output
>   '
>   
> -test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
> +test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '
>   	set_and_save_state dir/foo work work &&
>   	test_must_fail git reset -p HEAD^:dir/foo &&
>   	verify_saved_state dir/foo &&
>   	verify_saved_state bar
>   '
>   
> -test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
> +test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '
>   	set_and_save_state dir/foo work work &&
>   	test_must_fail git reset -p aaaaaaaa &&
>   	verify_saved_state dir/foo &&
> @@ -76,27 +76,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
>   # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
>   # the failure case (and thus get out of the loop).
>   
> -test_expect_success PERL 'git reset -p dir' '
> +test_expect_success 'git reset -p dir' '
>   	set_state dir/foo work work &&
>   	test_write_lines y n | git reset -p dir &&
>   	verify_state dir/foo work head &&
>   	verify_saved_state bar
>   '
>   
> -test_expect_success PERL 'git reset -p -- foo (inside dir)' '
> +test_expect_success 'git reset -p -- foo (inside dir)' '
>   	set_state dir/foo work work &&
>   	test_write_lines y n | (cd dir && git reset -p -- foo) &&
>   	verify_state dir/foo work head &&
>   	verify_saved_state bar
>   '
>   
> -test_expect_success PERL 'git reset -p HEAD^ -- dir' '
> +test_expect_success 'git reset -p HEAD^ -- dir' '
>   	test_write_lines y n | git reset -p HEAD^ -- dir &&
>   	verify_state dir/foo work parent &&
>   	verify_saved_state bar
>   '
>   
> -test_expect_success PERL 'none of this moved HEAD' '
> +test_expect_success 'none of this moved HEAD' '
>   	verify_saved_head
>   '
>   

Re: [PATCH v4 1/3] add-patch: remove unnecessary NEEDSWORK comment

[Phillip Wood]

Hi Ghanshyam

On 06/02/2024 22:50, Ghanshyam Thakkar wrote:
> The comment suggested to compare commit objects instead of string
> comparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.
> '@'). However, this approach would also count a non-checked out branch
> pointing to same commit as HEAD, as HEAD. This would cause confusion to
> the user.

I forgot to say before, but I think this could be squashed into the next 
patch so we remove the comment at the same time as fixing the issue it 
describes.

Best Wishes

Phillip

> Junio described it best as[1]:
> 
> "Users may consider 'HEAD' and '@' the same and
> may want them to behave the same way, but the user, when explicitly
> naming '$branch', means they want to "check contents out of that
> OTHER thing named '$branch', not the current branch"; it may or
> may not happen to be pointing at the same commit as HEAD, but if
> the user meant to say "check contents out of the current commit,
> (partially) reverting the local changes I have", the user would have
> said HEAD.  After all, the user may not even be immediately aware
> that '$branch' happens to point at the same commit as HEAD."
> 
> [1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/
> 
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>   add-patch.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 79eda168eb..68f525b35c 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1729,14 +1729,6 @@ 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) {
> -		/*
> -		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
> -		 * compare the commit objects instead so that other ways of
> -		 * saying the same thing (such as "@") are also handled
> -		 * appropriately.
> -		 *
> -		 * This applies to the cases below too.
> -		 */
>   		if (!revision || !strcmp(revision, "HEAD"))
>   			s.mode = &patch_mode_reset_head;
>   		else

Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite

[Phillip Wood]

On 07/02/2024 10:50, Phillip Wood wrote:
> Hi Ghanshyam
> 
> On 06/02/2024 22:50, Ghanshyam Thakkar wrote:
>  > The Perl version of the add -i/-p commands has been removed since
>  > 20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
>  > add--interactive", 2023-02-07)
>  >
>  > Therefore, Perl prerequisite in t2071-restore-patch and
>  > t7105-reset-patch is not necessary.
> 
> Thanks for adding this patch. If you do re-roll I've just noticed that 
> one of the tests in t7106-reset-unborn-branch.sh and another in 
> t2024-checkout-dwim.sh still have PERL prerequisites as well. I don't 
> think it is worth re-rolling just for that as we can clean them up 
> separately if needed.

I didn't cast my net wide enough when I was grepping earlier, 
t7514-commit-patch.sh and t3904-stash-patch.sh also have unnecessary 
PERL prerequisites

Best Wishes

Phillip

Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 07/02/2024 10:50, Phillip Wood wrote:
>> Hi Ghanshyam
>> On 06/02/2024 22:50, Ghanshyam Thakkar wrote:
>>  > The Perl version of the add -i/-p commands has been removed since
>>  > 20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
>>  > add--interactive", 2023-02-07)
>>  >
>>  > Therefore, Perl prerequisite in t2071-restore-patch and
>>  > t7105-reset-patch is not necessary.
>> Thanks for adding this patch. If you do re-roll I've just noticed
>> that one of the tests in t7106-reset-unborn-branch.sh and another in
>> t2024-checkout-dwim.sh still have PERL prerequisites as well. I
>> don't think it is worth re-rolling just for that as we can clean
>> them up separately if needed.
>
> I didn't cast my net wide enough when I was grepping earlier,
> t7514-commit-patch.sh and t3904-stash-patch.sh also have unnecessary
> PERL prerequisites

Thanks for helping usher this topic forward.  Very much appreciated.

Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite

[Eric Sunshine]

On Wed, Feb 7, 2024 at 8:51 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 07/02/2024 10:50, Phillip Wood wrote:
> > On 06/02/2024 22:50, Ghanshyam Thakkar wrote:
> >  > The Perl version of the add -i/-p commands has been removed since
> >  > 20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
> >  > add--interactive", 2023-02-07)
> >  >
> >  > Therefore, Perl prerequisite in t2071-restore-patch and
> >  > t7105-reset-patch is not necessary.
> >
> > Thanks for adding this patch. If you do re-roll I've just noticed that
> > one of the tests in t7106-reset-unborn-branch.sh and another in
> > t2024-checkout-dwim.sh still have PERL prerequisites as well. I don't
> > think it is worth re-rolling just for that as we can clean them up
> > separately if needed.
>
> I didn't cast my net wide enough when I was grepping earlier,
> t7514-commit-patch.sh and t3904-stash-patch.sh also have unnecessary
> PERL prerequisites

Additionally, patch [2/3] drops a PERL prerequisite when it moves an
existing test into a loop, but the removal of the prerequisite is not
mentioned in the commit message. Presumably, the relocation-into-loop
and prerequisite-removal should have been done separately (in patches
[2/3] and [3/3], respectively), and that's how I'd suggest doing it.

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

[Ghanshyam Thakkar]

On Wed Feb 7, 2024 at 6:35 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> > There is also logic in builtin/checkout.c 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.
>
> Makes me wonder why we cannot use the same "normalize @ to HEAD
> upfront" approach here, though?
>
> It would involve translating "@" given to new_branch_info->name to
> "HEAD" early, possibly in setup_new_branch_info_and_source_tree(),
> and that probably will fix the other strcmp() with "HEAD" that
> appears in builtin/checkout.c:update_refs_for_switch() as well, no?

I was wondering about "git checkout -b @". If we were to make the change
in setup_new_branch_info_and_source_tree(), then '@' would not be
treated as 'HEAD' in the above mentioned case and there would also be
some disparity on error messages showing '@' when running the above
command, and after hitting setup_new_branch_info_and_source_tree() would
show 'HEAD'. However, making the change in builtin/checkout.c:checkout_main()
would disallow creating a '@' branch alltogether. Hence, I would like some
feedback on whether we should change the behavior of 'git checkout -b @'
or not.

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

[Ghanshyam Thakkar]

Since v4, I have taken Junio's suggestion to change '@' to 'HEAD' as
early as reasonably possible. And also added new test to check the
no-op case of 'checkout HEAD/@'. 

And the second patch removes all the perl prerequisites from the tests
which use patch mode functionality, as pointed out by Phillip. And have
also taken into account Eric's suggestion to not remove perl
prerequisites while amending them in patch (1/2) and do it all
together in patch (2/2).

Ghanshyam Thakkar (2):
  add-patch: classify '@' as a synonym for 'HEAD'
  add -p tests: remove PERL prerequisites

 add-patch.c                    |  8 ------
 builtin/checkout.c             |  4 ++-
 builtin/reset.c                |  4 ++-
 t/t2016-checkout-patch.sh      | 46 +++++++++++++++++++---------------
 t/t2020-checkout-detach.sh     | 12 +++++++++
 t/t2024-checkout-dwim.sh       |  2 +-
 t/t2071-restore-patch.sh       | 46 ++++++++++++++++++----------------
 t/t3904-stash-patch.sh         |  6 -----
 t/t7105-reset-patch.sh         | 37 ++++++++++++++-------------
 t/t7106-reset-unborn-branch.sh |  2 +-
 t/t7514-commit-patch.sh        |  6 -----
 11 files changed, 91 insertions(+), 82 deletions(-)

-- 
2.43.0

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

[Ghanshyam Thakkar]

Currently, (restore, checkout, reset) commands correctly take '@' as a
synonym for 'HEAD'. However, in patch mode different prompts/messages
are given on command line due to patch mode machinery not considering
'@' to be a synonym for 'HEAD' due to literal string comparison with
the word 'HEAD', and therefore assigning patch_mode_($command)_nothead
and triggering reverse mode (-R in diff-index). The NEEDSWORK comment
suggested comparing commit objects to get around this. However, doing
so would also take a non-checked out branch pointing to the same commit
as HEAD, as HEAD. This would cause confusion to the user.

Therefore, after parsing '@', replace it with 'HEAD' as reasonably
early as possible. This also solves another problem of disparity
between 'git checkout HEAD' and 'git checkout @' (latter detaches at
the HEAD commit and the former does not).

Trade-offs:
- Some of the errors would show the revision argument as 'HEAD' when
  given '@'. This should be fine, as most users who probably use '@'
  would be aware that it is a shortcut for 'HEAD' and most probably
  used to use 'HEAD'. There is also relevant documentation in
  'gitrevisions' manpage about '@' being the shortcut for 'HEAD'. Also,
  the simplicity of the solution far outweighs this cost.

- Consider '@' as a shortcut for 'HEAD' even if 'refs/heads/@' exists
  at a different commit. Naming a branch '@' is an obvious foot-gun and
  many existing commands already take '@' for 'HEAD' even if
  'refs/heads/@' exists at a different commit or does not exist at all
  (e.g. 'git log @', 'git push origin @' etc.). Therefore this is an
  existing assumption and should not be a problem.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-patch.c                |  8 -------
 builtin/checkout.c         |  4 +++-
 builtin/reset.c            |  4 +++-
 t/t2016-checkout-patch.sh  | 46 +++++++++++++++++++++-----------------
 t/t2020-checkout-detach.sh | 12 ++++++++++
 t/t2071-restore-patch.sh   | 18 +++++++++------
 t/t7105-reset-patch.sh     | 15 ++++++++-----
 7 files changed, 64 insertions(+), 43 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..68f525b35c 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,14 +1729,6 @@ 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) {
-		/*
-		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
-		 * compare the commit objects instead so that other ways of
-		 * saying the same thing (such as "@") are also handled
-		 * appropriately.
-		 *
-		 * This applies to the cases below too.
-		 */
 		if (!revision || !strcmp(revision, "HEAD"))
 			s.mode = &patch_mode_reset_head;
 		else
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..067c251933 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(
 	struct tree **source_tree = &opts->source_tree;
 	struct object_id branch_rev;
 
-	new_branch_info->name = xstrdup(arg);
+	/* treat '@' as a shortcut for 'HEAD' */
+	new_branch_info->name = !strcmp(arg, "@") ? xstrdup("HEAD") :
+						    xstrdup(arg);
 	setup_branch_path(new_branch_info);
 
 	if (!check_refname_format(new_branch_info->path, 0) &&
diff --git a/builtin/reset.c b/builtin/reset.c
index 8390bfe4c4..f0bf29a478 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,
 			verify_filename(prefix, argv[0], 1);
 		}
 	}
-	*rev_ret = rev;
+
+	/* treat '@' as a shortcut for 'HEAD' */
+	*rev_ret = !strcmp("@", rev) ? "HEAD" : rev;
 
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
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/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 8202ef8c74..bce284c297 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -45,6 +45,18 @@ test_expect_success 'checkout branch does not detach' '
 	check_not_detached
 '
 
+for opt in "HEAD" "@"
+do
+	test_expect_success "checkout $opt no-op/don't detach" '
+		reset &&
+		cat .git/HEAD >expect &&
+		git checkout $opt &&
+		cat .git/HEAD >actual &&
+		check_not_detached &&
+		test_cmp expect actual
+	'
+done
+
 test_expect_success 'checkout tag detaches' '
 	reset &&
 	git checkout tag &&
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..0f597416d8 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -26,12 +26,15 @@ test_expect_success PERL 'saying "n" does nothing' '
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p' '
-	test_write_lines n y | git reset -p >output &&
-	verify_state dir/foo work head &&
-	verify_saved_state bar &&
-	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 &&
-- 
2.43.0

[PATCH v5 2/2] add -p tests: remove PERL prerequisites

[Ghanshyam Thakkar]

The Perl version of the add -i/-p commands has been removed since
20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
add--interactive", 2023-02-07)

Therefore, Perl prerequisite in the test scripts which use the patch
mode functionality is not neccessary.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t2024-checkout-dwim.sh       |  2 +-
 t/t2071-restore-patch.sh       | 28 ++++++++++++++--------------
 t/t3904-stash-patch.sh         |  6 ------
 t/t7105-reset-patch.sh         | 22 +++++++++++-----------
 t/t7106-reset-unborn-branch.sh |  2 +-
 t/t7514-commit-patch.sh        |  6 ------
 6 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index a97416ce65..a3b1449ef1 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -113,7 +113,7 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'
 	test_grep ! "^hint: " stderr
 '
 
-test_expect_success PERL 'checkout -p with multiple remotes does not print advice' '
+test_expect_success 'checkout -p with multiple remotes does not print advice' '
 	git checkout -B main &&
 	test_might_fail git branch -D foo &&
 
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index 3dc9184b4a..27e85be40a 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -4,7 +4,7 @@ test_description='git restore --patch'
 
 . ./lib-patch-mode.sh
 
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent >dir/foo &&
 	echo dummy >bar &&
@@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '
 	save_head
 '
 
-test_expect_success PERL 'restore -p without pathspec is fine' '
+test_expect_success 'restore -p without pathspec is fine' '
 	echo q >cmd &&
 	git restore -p <cmd
 '
 
 # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
 
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n n | git restore -p &&
 	verify_saved_state bar &&
 	verify_saved_state dir/foo
 '
 
-test_expect_success PERL 'git restore -p' '
+test_expect_success 'git restore -p' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n y | git restore -p &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'git restore -p with staged changes' '
+test_expect_success 'git restore -p with staged changes' '
 	set_state dir/foo work index &&
 	test_write_lines n y | git restore -p &&
 	verify_saved_state bar &&
@@ -46,7 +46,7 @@ test_expect_success PERL 'git restore -p with staged changes' '
 
 for opt in "HEAD" "@"
 do
-	test_expect_success PERL "git restore -p --source=$opt" '
+	test_expect_success "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 &&
@@ -56,7 +56,7 @@ do
 	'
 done
 
-test_expect_success PERL 'git restore -p --source=HEAD^' '
+test_expect_success '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^ &&
@@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '
 	verify_state dir/foo parent index
 '
 
-test_expect_success PERL 'git restore -p --source=HEAD^...' '
+test_expect_success '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^... &&
@@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '
 	verify_state dir/foo parent index
 '
 
-test_expect_success PERL 'git restore -p handles deletion' '
+test_expect_success 'git restore -p handles deletion' '
 	set_state dir/foo work index &&
 	rm dir/foo &&
 	test_write_lines n y | git restore -p &&
@@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'path limiting works: dir' '
+test_expect_success 'path limiting works: dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git restore -p dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: -- dir' '
+test_expect_success 'path limiting works: -- dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git restore -p -- dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
+test_expect_success 'path limiting works: HEAD^ -- dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | git restore -p --source=HEAD^ -- dir &&
@@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
 	verify_state dir/foo parent head
 '
 
-test_expect_success PERL 'path limiting works: foo inside dir' '
+test_expect_success 'path limiting works: foo inside dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | (cd dir && git restore -p foo) &&
@@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index accfe3845c..368fc2a6cc 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -3,12 +3,6 @@
 test_description='stash -p'
 . ./lib-patch-mode.sh
 
-if ! test_have_prereq PERL
-then
-	skip_all='skipping stash -p tests, perl not available'
-	test_done
-fi
-
 test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 0f597416d8..4e7ad2b0f3 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -5,7 +5,7 @@ test_description='git reset --patch'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
 	echo dummy > bar &&
@@ -19,7 +19,7 @@ test_expect_success PERL 'setup' '
 
 # note: bar sorts before foo, so the first 'n' is always to skip 'bar'
 
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
 	set_and_save_state dir/foo work work &&
 	test_write_lines n n | git reset -p &&
 	verify_saved_state dir/foo &&
@@ -28,7 +28,7 @@ test_expect_success PERL 'saying "n" does nothing' '
 
 for opt in "HEAD" "@" ""
 do
-	test_expect_success PERL "git reset -p $opt" '
+	test_expect_success "git reset -p $opt" '
 		test_write_lines n y | git reset -p $opt >output &&
 		verify_state dir/foo work head &&
 		verify_saved_state bar &&
@@ -36,28 +36,28 @@ do
 	'
 done
 
-test_expect_success PERL 'git reset -p HEAD^' '
+test_expect_success 'git reset -p HEAD^' '
 	test_write_lines n y | git reset -p HEAD^ >output &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar &&
 	test_grep "Apply" output
 '
 
-test_expect_success PERL 'git reset -p HEAD^^{tree}' '
+test_expect_success 'git reset -p HEAD^^{tree}' '
 	test_write_lines n y | git reset -p HEAD^^{tree} >output &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar &&
 	test_grep "Apply" output
 '
 
-test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
+test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '
 	set_and_save_state dir/foo work work &&
 	test_must_fail git reset -p HEAD^:dir/foo &&
 	verify_saved_state dir/foo &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
+test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '
 	set_and_save_state dir/foo work work &&
 	test_must_fail git reset -p aaaaaaaa &&
 	verify_saved_state dir/foo &&
@@ -69,27 +69,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'git reset -p dir' '
+test_expect_success 'git reset -p dir' '
 	set_state dir/foo work work &&
 	test_write_lines y n | git reset -p dir &&
 	verify_state dir/foo work head &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p -- foo (inside dir)' '
+test_expect_success 'git reset -p -- foo (inside dir)' '
 	set_state dir/foo work work &&
 	test_write_lines y n | (cd dir && git reset -p -- foo) &&
 	verify_state dir/foo work head &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p HEAD^ -- dir' '
+test_expect_success 'git reset -p HEAD^ -- dir' '
 	test_write_lines y n | git reset -p HEAD^ -- dir &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
index d20e5709f9..88d1c8adf4 100755
--- a/t/t7106-reset-unborn-branch.sh
+++ b/t/t7106-reset-unborn-branch.sh
@@ -34,7 +34,7 @@ test_expect_success 'reset $file' '
 	test_cmp expect actual
 '
 
-test_expect_success PERL 'reset -p' '
+test_expect_success 'reset -p' '
 	rm .git/index &&
 	git add a &&
 	echo y >yes &&
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 998a2103c7..b4de10a5dd 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -3,12 +3,6 @@
 test_description='hunk edit with "commit -p -m"'
 . ./test-lib.sh
 
-if ! test_have_prereq PERL
-then
-	skip_all="skipping '$test_description' tests, perl not available"
-	test_done
-fi
-
 test_expect_success 'setup (initial)' '
 	echo line1 >file &&
 	git add file &&
-- 
2.43.0

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

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

>  add-patch.c                |  8 -------
>  builtin/checkout.c         |  4 +++-
>  builtin/reset.c            |  4 +++-
>  t/t2016-checkout-patch.sh  | 46 +++++++++++++++++++++-----------------
>  t/t2020-checkout-detach.sh | 12 ++++++++++
>  t/t2071-restore-patch.sh   | 18 +++++++++------
>  t/t7105-reset-patch.sh     | 15 ++++++++-----
>  7 files changed, 64 insertions(+), 43 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 79eda168eb..68f525b35c 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1729,14 +1729,6 @@ 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) {
> -		/*
> -		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
> -		 * compare the commit objects instead so that other ways of
> -		 * saying the same thing (such as "@") are also handled
> -		 * appropriately.
> -		 *
> -		 * This applies to the cases below too.
> -		 */
>  		if (!revision || !strcmp(revision, "HEAD"))
>  			s.mode = &patch_mode_reset_head;
>  		else
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a6e30931b5..067c251933 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(
>  	struct tree **source_tree = &opts->source_tree;
>  	struct object_id branch_rev;
>  
> -	new_branch_info->name = xstrdup(arg);
> +	/* treat '@' as a shortcut for 'HEAD' */
> +	new_branch_info->name = !strcmp(arg, "@") ? xstrdup("HEAD") :
> +						    xstrdup(arg);
>  	setup_branch_path(new_branch_info);
>  
>  	if (!check_refname_format(new_branch_info->path, 0) &&
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 8390bfe4c4..f0bf29a478 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,
>  			verify_filename(prefix, argv[0], 1);
>  		}
>  	}
> -	*rev_ret = rev;
> +
> +	/* treat '@' as a shortcut for 'HEAD' */
> +	*rev_ret = !strcmp("@", rev) ? "HEAD" : rev;
>  
>  	parse_pathspec(pathspec, 0,
>  		       PATHSPEC_PREFER_FULL |

Nice.  Things have become so much simpler, without having to touch
millions of strcmp() with "HEAD" everywhere in the code paths.

Will queue.  Thanks.

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

[Ghanshyam Thakkar]

I just noticed after sending v5 that, in one of the tests, while
moving it inside the loop for also testing '@', set_and_save_state was
not used therefore the subsequent tests after the first $opt will not have
the changes for which we prove 'y' thereby making the tests useless.
(Although it would not fail regardless of this). This got unnoticed by
the previous reviews since v1 and me as well.

Apologies for the oversight and noise.

Ghanshyam Thakkar (2):
  add-patch: classify '@' as a synonym for 'HEAD'
  add -p tests: remove PERL prerequisites

 add-patch.c                    |  8 ------
 builtin/checkout.c             |  4 ++-
 builtin/reset.c                |  4 ++-
 t/t2016-checkout-patch.sh      | 46 +++++++++++++++++++---------------
 t/t2020-checkout-detach.sh     | 12 +++++++++
 t/t2024-checkout-dwim.sh       |  2 +-
 t/t2071-restore-patch.sh       | 46 ++++++++++++++++++----------------
 t/t3904-stash-patch.sh         |  6 -----
 t/t7105-reset-patch.sh         | 38 +++++++++++++++-------------
 t/t7106-reset-unborn-branch.sh |  2 +-
 t/t7514-commit-patch.sh        |  6 -----
 11 files changed, 92 insertions(+), 82 deletions(-)

change since v5:
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 0f597416d8..453872c8ba 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -29,6 +29,7 @@ test_expect_success PERL 'saying "n" does nothing' '
 for opt in "HEAD" "@" ""
 do
        test_expect_success PERL "git reset -p $opt" '
+               set_and_save_state dir/foo work work &&
                test_write_lines n y | git reset -p $opt >output &&
                verify_state dir/foo work head &&
                verify_saved_state bar &&

-- 
2.43.0

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

[Ghanshyam Thakkar]

Currently, (restore, checkout, reset) commands correctly take '@' as a
synonym for 'HEAD'. However, in patch mode different prompts/messages
are given on command line due to patch mode machinery not considering
'@' to be a synonym for 'HEAD' due to literal string comparison with
the word 'HEAD', and therefore assigning patch_mode_($command)_nothead
and triggering reverse mode (-R in diff-index). The NEEDSWORK comment
suggested comparing commit objects to get around this. However, doing
so would also take a non-checked out branch pointing to the same commit
as HEAD, as HEAD. This would cause confusion to the user.

Therefore, after parsing '@', replace it with 'HEAD' as reasonably
early as possible. This also solves another problem of disparity
between 'git checkout HEAD' and 'git checkout @' (latter detaches at
the HEAD commit and the former does not).

Trade-offs:
- Some of the errors would show the revision argument as 'HEAD' when
  given '@'. This should be fine, as most users who probably use '@'
  would be aware that it is a shortcut for 'HEAD' and most probably
  used to use 'HEAD'. There is also relevant documentation in
  'gitrevisions' manpage about '@' being the shortcut for 'HEAD'. Also,
  the simplicity of the solution far outweighs this cost.

- Consider '@' as a shortcut for 'HEAD' even if 'refs/heads/@' exists
  at a different commit. Naming a branch '@' is an obvious foot-gun and
  many existing commands already take '@' for 'HEAD' even if
  'refs/heads/@' exists at a different commit or does not exist at all
  (e.g. 'git log @', 'git push origin @' etc.). Therefore this is an
  existing assumption and should not be a problem.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-patch.c                |  8 -------
 builtin/checkout.c         |  4 +++-
 builtin/reset.c            |  4 +++-
 t/t2016-checkout-patch.sh  | 46 +++++++++++++++++++++-----------------
 t/t2020-checkout-detach.sh | 12 ++++++++++
 t/t2071-restore-patch.sh   | 18 +++++++++------
 t/t7105-reset-patch.sh     | 16 ++++++++-----
 7 files changed, 65 insertions(+), 43 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..68f525b35c 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1729,14 +1729,6 @@ 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) {
-		/*
-		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
-		 * compare the commit objects instead so that other ways of
-		 * saying the same thing (such as "@") are also handled
-		 * appropriately.
-		 *
-		 * This applies to the cases below too.
-		 */
 		if (!revision || !strcmp(revision, "HEAD"))
 			s.mode = &patch_mode_reset_head;
 		else
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..067c251933 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(
 	struct tree **source_tree = &opts->source_tree;
 	struct object_id branch_rev;
 
-	new_branch_info->name = xstrdup(arg);
+	/* treat '@' as a shortcut for 'HEAD' */
+	new_branch_info->name = !strcmp(arg, "@") ? xstrdup("HEAD") :
+						    xstrdup(arg);
 	setup_branch_path(new_branch_info);
 
 	if (!check_refname_format(new_branch_info->path, 0) &&
diff --git a/builtin/reset.c b/builtin/reset.c
index 8390bfe4c4..f0bf29a478 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,
 			verify_filename(prefix, argv[0], 1);
 		}
 	}
-	*rev_ret = rev;
+
+	/* treat '@' as a shortcut for 'HEAD' */
+	*rev_ret = !strcmp("@", rev) ? "HEAD" : rev;
 
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
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/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 8202ef8c74..bce284c297 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -45,6 +45,18 @@ test_expect_success 'checkout branch does not detach' '
 	check_not_detached
 '
 
+for opt in "HEAD" "@"
+do
+	test_expect_success "checkout $opt no-op/don't detach" '
+		reset &&
+		cat .git/HEAD >expect &&
+		git checkout $opt &&
+		cat .git/HEAD >actual &&
+		check_not_detached &&
+		test_cmp expect actual
+	'
+done
+
 test_expect_success 'checkout tag detaches' '
 	reset &&
 	git checkout tag &&
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..453872c8ba 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -26,12 +26,16 @@ test_expect_success PERL 'saying "n" does nothing' '
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p' '
-	test_write_lines n y | git reset -p >output &&
-	verify_state dir/foo work head &&
-	verify_saved_state bar &&
-	test_grep "Unstage" output
-'
+for opt in "HEAD" "@" ""
+do
+	test_expect_success PERL "git reset -p $opt" '
+		set_and_save_state dir/foo work work &&
+		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 &&
-- 
2.43.0

[PATCH v6 2/2] add -p tests: remove PERL prerequisites

[Ghanshyam Thakkar]

The Perl version of the add -i/-p commands has been removed since
20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
add--interactive", 2023-02-07)

Therefore, Perl prerequisite in the test scripts which use the patch
mode functionality is not neccessary.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t2024-checkout-dwim.sh       |  2 +-
 t/t2071-restore-patch.sh       | 28 ++++++++++++++--------------
 t/t3904-stash-patch.sh         |  6 ------
 t/t7105-reset-patch.sh         | 22 +++++++++++-----------
 t/t7106-reset-unborn-branch.sh |  2 +-
 t/t7514-commit-patch.sh        |  6 ------
 6 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index a97416ce65..a3b1449ef1 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -113,7 +113,7 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'
 	test_grep ! "^hint: " stderr
 '
 
-test_expect_success PERL 'checkout -p with multiple remotes does not print advice' '
+test_expect_success 'checkout -p with multiple remotes does not print advice' '
 	git checkout -B main &&
 	test_might_fail git branch -D foo &&
 
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index 3dc9184b4a..27e85be40a 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -4,7 +4,7 @@ test_description='git restore --patch'
 
 . ./lib-patch-mode.sh
 
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent >dir/foo &&
 	echo dummy >bar &&
@@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '
 	save_head
 '
 
-test_expect_success PERL 'restore -p without pathspec is fine' '
+test_expect_success 'restore -p without pathspec is fine' '
 	echo q >cmd &&
 	git restore -p <cmd
 '
 
 # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
 
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n n | git restore -p &&
 	verify_saved_state bar &&
 	verify_saved_state dir/foo
 '
 
-test_expect_success PERL 'git restore -p' '
+test_expect_success 'git restore -p' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n y | git restore -p &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'git restore -p with staged changes' '
+test_expect_success 'git restore -p with staged changes' '
 	set_state dir/foo work index &&
 	test_write_lines n y | git restore -p &&
 	verify_saved_state bar &&
@@ -46,7 +46,7 @@ test_expect_success PERL 'git restore -p with staged changes' '
 
 for opt in "HEAD" "@"
 do
-	test_expect_success PERL "git restore -p --source=$opt" '
+	test_expect_success "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 &&
@@ -56,7 +56,7 @@ do
 	'
 done
 
-test_expect_success PERL 'git restore -p --source=HEAD^' '
+test_expect_success '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^ &&
@@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '
 	verify_state dir/foo parent index
 '
 
-test_expect_success PERL 'git restore -p --source=HEAD^...' '
+test_expect_success '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^... &&
@@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '
 	verify_state dir/foo parent index
 '
 
-test_expect_success PERL 'git restore -p handles deletion' '
+test_expect_success 'git restore -p handles deletion' '
 	set_state dir/foo work index &&
 	rm dir/foo &&
 	test_write_lines n y | git restore -p &&
@@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'path limiting works: dir' '
+test_expect_success 'path limiting works: dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git restore -p dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: -- dir' '
+test_expect_success 'path limiting works: -- dir' '
 	set_state dir/foo work head &&
 	test_write_lines y n | git restore -p -- dir &&
 	verify_saved_state bar &&
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
+test_expect_success 'path limiting works: HEAD^ -- dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | git restore -p --source=HEAD^ -- dir &&
@@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '
 	verify_state dir/foo parent head
 '
 
-test_expect_success PERL 'path limiting works: foo inside dir' '
+test_expect_success 'path limiting works: foo inside dir' '
 	set_state dir/foo work head &&
 	# the third n is to get out in case it mistakenly does not apply
 	test_write_lines y n n | (cd dir && git restore -p foo) &&
@@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
 	verify_state dir/foo head head
 '
 
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index accfe3845c..368fc2a6cc 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -3,12 +3,6 @@
 test_description='stash -p'
 . ./lib-patch-mode.sh
 
-if ! test_have_prereq PERL
-then
-	skip_all='skipping stash -p tests, perl not available'
-	test_done
-fi
-
 test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 453872c8ba..f4f3b7a677 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -5,7 +5,7 @@ test_description='git reset --patch'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
 	mkdir dir &&
 	echo parent > dir/foo &&
 	echo dummy > bar &&
@@ -19,7 +19,7 @@ test_expect_success PERL 'setup' '
 
 # note: bar sorts before foo, so the first 'n' is always to skip 'bar'
 
-test_expect_success PERL 'saying "n" does nothing' '
+test_expect_success 'saying "n" does nothing' '
 	set_and_save_state dir/foo work work &&
 	test_write_lines n n | git reset -p &&
 	verify_saved_state dir/foo &&
@@ -28,7 +28,7 @@ test_expect_success PERL 'saying "n" does nothing' '
 
 for opt in "HEAD" "@" ""
 do
-	test_expect_success PERL "git reset -p $opt" '
+	test_expect_success "git reset -p $opt" '
 		set_and_save_state dir/foo work work &&
 		test_write_lines n y | git reset -p $opt >output &&
 		verify_state dir/foo work head &&
@@ -37,28 +37,28 @@ do
 	'
 done
 
-test_expect_success PERL 'git reset -p HEAD^' '
+test_expect_success 'git reset -p HEAD^' '
 	test_write_lines n y | git reset -p HEAD^ >output &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar &&
 	test_grep "Apply" output
 '
 
-test_expect_success PERL 'git reset -p HEAD^^{tree}' '
+test_expect_success 'git reset -p HEAD^^{tree}' '
 	test_write_lines n y | git reset -p HEAD^^{tree} >output &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar &&
 	test_grep "Apply" output
 '
 
-test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '
+test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '
 	set_and_save_state dir/foo work work &&
 	test_must_fail git reset -p HEAD^:dir/foo &&
 	verify_saved_state dir/foo &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
+test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '
 	set_and_save_state dir/foo work work &&
 	test_must_fail git reset -p aaaaaaaa &&
 	verify_saved_state dir/foo &&
@@ -70,27 +70,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '
 # dir/foo.  There's always an extra 'n' to reject edits to dir/foo in
 # the failure case (and thus get out of the loop).
 
-test_expect_success PERL 'git reset -p dir' '
+test_expect_success 'git reset -p dir' '
 	set_state dir/foo work work &&
 	test_write_lines y n | git reset -p dir &&
 	verify_state dir/foo work head &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p -- foo (inside dir)' '
+test_expect_success 'git reset -p -- foo (inside dir)' '
 	set_state dir/foo work work &&
 	test_write_lines y n | (cd dir && git reset -p -- foo) &&
 	verify_state dir/foo work head &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'git reset -p HEAD^ -- dir' '
+test_expect_success 'git reset -p HEAD^ -- dir' '
 	test_write_lines y n | git reset -p HEAD^ -- dir &&
 	verify_state dir/foo work parent &&
 	verify_saved_state bar
 '
 
-test_expect_success PERL 'none of this moved HEAD' '
+test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
index d20e5709f9..88d1c8adf4 100755
--- a/t/t7106-reset-unborn-branch.sh
+++ b/t/t7106-reset-unborn-branch.sh
@@ -34,7 +34,7 @@ test_expect_success 'reset $file' '
 	test_cmp expect actual
 '
 
-test_expect_success PERL 'reset -p' '
+test_expect_success 'reset -p' '
 	rm .git/index &&
 	git add a &&
 	echo y >yes &&
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index 998a2103c7..b4de10a5dd 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -3,12 +3,6 @@
 test_description='hunk edit with "commit -p -m"'
 . ./test-lib.sh
 
-if ! test_have_prereq PERL
-then
-	skip_all="skipping '$test_description' tests, perl not available"
-	test_done
-fi
-
 test_expect_success 'setup (initial)' '
 	echo line1 >file &&
 	git add file &&
-- 
2.43.0

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

[Phillip Wood]

Hi Ghanshyam

On 13/02/2024 00:05, Ghanshyam Thakkar wrote:
> I just noticed after sending v5 that, in one of the tests, while
> moving it inside the loop for also testing '@', set_and_save_state was
> not used therefore the subsequent tests after the first $opt will not have
> the changes for which we prove 'y' thereby making the tests useless.
> (Although it would not fail regardless of this). This got unnoticed by
> the previous reviews since v1 and me as well.

This version all looks fine to me, thanks for working on it. Thanks for 
removing the PERL prerequisite from the remaining tests. I think the 
change to "git checkout @" is a good idea as it makes "@" act like 
"HEAD", hopefully there aren't too many users relying on "git checkout 
@" to detach HEAD.

Best Wishes

Phillip

> Apologies for the oversight and noise.
> 
> Ghanshyam Thakkar (2):
>    add-patch: classify '@' as a synonym for 'HEAD'
>    add -p tests: remove PERL prerequisites
> 
>   add-patch.c                    |  8 ------
>   builtin/checkout.c             |  4 ++-
>   builtin/reset.c                |  4 ++-
>   t/t2016-checkout-patch.sh      | 46 +++++++++++++++++++---------------
>   t/t2020-checkout-detach.sh     | 12 +++++++++
>   t/t2024-checkout-dwim.sh       |  2 +-
>   t/t2071-restore-patch.sh       | 46 ++++++++++++++++++----------------
>   t/t3904-stash-patch.sh         |  6 -----
>   t/t7105-reset-patch.sh         | 38 +++++++++++++++-------------
>   t/t7106-reset-unborn-branch.sh |  2 +-
>   t/t7514-commit-patch.sh        |  6 -----
>   11 files changed, 92 insertions(+), 82 deletions(-)
> 
> change since v5:
> diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
> index 0f597416d8..453872c8ba 100755
> --- a/t/t7105-reset-patch.sh
> +++ b/t/t7105-reset-patch.sh
> @@ -29,6 +29,7 @@ test_expect_success PERL 'saying "n" does nothing' '
>   for opt in "HEAD" "@" ""
>   do
>          test_expect_success PERL "git reset -p $opt" '
> +               set_and_save_state dir/foo work work &&
>                  test_write_lines n y | git reset -p $opt >output &&
>                  verify_state dir/foo work head &&
>                  verify_saved_state bar &&
>