diff mbox series

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

Message ID 20240211202035.7196-4-shyamthakkar001@gmail.com (mailing list archive)
State Accepted
Commit 8c95d8219b07ee70d03ad226651134d5409ecfc6
Headers show
Series '@' as a synonym for 'HEAD' in patch mode | expand

Commit Message

Ghanshyam Thakkar Feb. 11, 2024, 8:20 p.m. UTC
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(-)

Comments

Junio C Hamano Feb. 12, 2024, 9:45 p.m. UTC | #1
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.
diff mbox series

Patch

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