{"thread":{"id":"60801","subject":"[GSOC][RFC PATCH 0/2] add-patch: compare object id","startedAt":"2024-01-28T18:19:05Z","lastAt":"2024-02-14T11:06:14Z","messageCount":46,"participants":["Ghanshyam Thakkar","Patrick Steinhardt","Junio C Hamano","Phillip Wood","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487467","messageId":"20240128181202.986753-2-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":null,"subject":"[GSOC][RFC PATCH 0/2] add-patch: compare object id","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-28T18:11:21Z","receivedAt":"2024-01-28T18:19:05Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This patch series is RFC as this contains a partial behavior change.\nThe first patch introduces a new function to compare object id instead\nof revision strings. The second patch removes the special case for HEAD\nin builtin/checkout.c.\n\nThis patch series enables for HEAD to be specified in any form and\nremoves need to be kept a literal string 'HEAD'. Consequently, this\nremoves the need to keep a special case for HEAD in callers of\nrun_add_p() (as in builtin/checkout.c).\n\nAdditional context:\nhttps://lore.kernel.org/git/xmqqsgat7ttf.fsf@gitster.c.googlers.com/\n\nGhanshyam Thakkar (2):\n add-patch: compare object id instead of literal string\n checkout: remove HEAD special case\n\n add-patch.c | 28 +++++++++++++++-------\n builtin/checkout.c | 9 ++++---\n t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------\n t/t2071-restore-patch.sh | 21 ++++++++++------\n t/t7105-reset-patch.sh | 14 +++++++++++\n 5 files changed, 81 insertions(+), 41 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"487468","messageId":"20240128181202.986753-3-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240128181202.986753-2-shyamthakkar001@gmail.com","subject":"[RFC PATCH 1/2] add-patch: compare object id instead of literal string","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-28T18:11:22Z","receivedAt":"2024-01-28T18:19:13Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add a new function reveq(), which takes repository struct and two revision\nstrings as arguments and returns 0 if the revisions point to the same\nobject. Passing a rev which does not point to an object is considered\nundefined behavior as the underlying function memcmp() will be called\nwith NULL hash strings.\n\nSubsequently, replace literal string comparison to HEAD in run_add_p()\nwith reveq() to handle more ways of saying HEAD (such as '@' or '$branch'\nwhere $branch points to same commit as HEAD). This addresses the\nNEEDSWORK comment in run_add_p().\n\nHowever, in ADD_P_RESET mode keep string comparison in logical OR with\nreveq() to handle unborn HEAD.\n\nAs for the behavior change, with this patch applied if the given\nrevision points to the same object as HEAD, the patch mode will be set to\npatch_mode_(reset,checkout,worktree)_head instead of\npatch_mode_(...)_nothead. That is equivalent of not setting -R flag in\ndiff-index, which would have been otherwise set before this patch.\nHowever, when given same set of inputs, the actual outcome is same as\nbefore this patch. Therefore, this does not affect any automated scripts.\n\nAlso, add testcases to check the similarity of result between different\nways of saying HEAD.\n\nSigned-off-by: Ghanshyam Thakkar \n---\nShould the return values of repo_get_oid() be checked in reveq()? As\nreveq() is not a global function and is only used in run_add_p(), the\nvalidity of revisions is already checked beforehand by builtin/checkout.c\nand builtin/reset.c before the call to run_add_p().\n\n add-patch.c | 28 +++++++++++++++-------\n t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------\n t/t2071-restore-patch.sh | 21 ++++++++++------\n t/t7105-reset-patch.sh | 14 +++++++++++\n 4 files changed, 77 insertions(+), 36 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..01eb71d90e 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -14,6 +14,7 @@\n #include \"color.h\"\n #include \"compat/terminal.h\"\n #include \"prompt.h\"\n+#include \"hash.h\"\n \n enum prompt_mode_type {\n \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n@@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,\n \t\t INDEX_ENVIRONMENT \"=%s\", s->s.r->index_file);\n }\n \n+// Check if two revisions point to the same object. Passing a rev which does not\n+// point to an object is undefined behavior.\n+static inline int reveq(struct repository *r, const char *rev1,\n+\t\t\tconst char *rev2)\n+{\n+\tstruct object_id oid_rev1, oid_rev2;\n+\trepo_get_oid(r, rev1, &oid_rev1);\n+\trepo_get_oid(r, rev2, &oid_rev2);\n+\n+\treturn !oideq(&oid_rev1, &oid_rev2);\n+}\n+\n static int parse_range(const char **p,\n \t\t unsigned long *offset, unsigned long *count)\n {\n@@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n \t\t/*\n-\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n-\t\t * compare the commit objects instead so that other ways of\n-\t\t * saying the same thing (such as \"@\") are also handled\n-\t\t * appropriately.\n-\t\t *\n-\t\t * This applies to the cases below too.\n+\t\t * The literal string comparison to HEAD below is kept\n+\t\t * to handle unborn HEAD.\n \t\t */\n-\t\tif (!revision || !strcmp(revision, \"HEAD\"))\n+\t\tif (!revision || !strcmp(revision, \"HEAD\") ||\n+\t\t !reveq(r, revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_reset_nothead;\n \t} else if (mode == ADD_P_CHECKOUT) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (!reveq(r, revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_checkout_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_checkout_nothead;\n \t} else if (mode == ADD_P_WORKTREE) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (!reveq(r, revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_worktree_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_worktree_nothead;\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 747eb5563e..431f34fa9c 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -12,6 +12,7 @@ test_expect_success 'setup' '\n \tgit commit -m initial &&\n \ttest_tick &&\n \ttest_commit second dir/foo head &&\n+\tgit branch newbranch &&\n \tset_and_save_state bar bar_work bar_index &&\n \tsave_head\n '\n@@ -38,26 +39,35 @@ test_expect_success 'git checkout -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '\n-\tset_and_save_state dir/foo work head &&\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_saved_state dir/foo\n-'\n-\n-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '\n-\ttest_write_lines n y y | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n-\n-test_expect_success 'git checkout -p HEAD with change already staged' '\n-\tset_state dir/foo index index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n+# Note: 'newbranch' points to the same commit as HEAD. And it is technically\n+# allowed to name a branch '@' as of now, however in below test '@'\n+# represents the shortcut for HEAD.\n+for opt in \"HEAD\" \"@\" \"newbranch\"\n+do\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n+\t\tset_and_save_state dir/foo work head &&\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_saved_state dir/foo &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: apply\" '\n+\t\ttest_write_lines n y y | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with change already staged\" '\n+\t\tset_state dir/foo index index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success 'git checkout -p HEAD^...' '\n \t# the third n is to get out in case it mistakenly does not apply\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex b5c5c0ff7e..305b4a0c4f 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -12,6 +12,7 @@ test_expect_success PERL 'setup' '\n \tgit commit -m initial &&\n \ttest_tick &&\n \ttest_commit second dir/foo head &&\n+\tgit branch newbranch &&\n \tset_and_save_state bar bar_work bar_index &&\n \tsave_head\n '\n@@ -44,13 +45,19 @@ test_expect_success PERL 'git restore -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD' '\n-\tset_state dir/foo work index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git restore -p --source=HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head index\n-'\n+# Note: 'newbranch' points to the same commit as HEAD. And '@' is a\n+# shortcut for HEAD.\n+for opt in \"HEAD\" \"@\" \"newbranch\"\n+do\n+\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n+\t\tset_state dir/foo work index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head index &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 05079c7246..65a8802b29 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -13,6 +13,7 @@ test_expect_success PERL 'setup' '\n \tgit commit -m initial &&\n \ttest_tick &&\n \ttest_commit second dir/foo head &&\n+\tgit branch newbranch &&\n \tset_and_save_state bar bar_work bar_index &&\n \tsave_head\n '\n@@ -33,6 +34,19 @@ test_expect_success PERL 'git reset -p' '\n \ttest_grep \"Unstage\" output\n '\n \n+# Note: '@' can technically also be used as a branch name, but in below test\n+# it represents the shortcut for HEAD. And 'newbranch' points to the same\n+# commit as HEAD.\n+for opt in \"HEAD\" \"@\" \"newbranch\"\n+do\n+\ttest_expect_success PERL \"git reset -p $opt\" '\n+\t\ttest_write_lines n y | git reset -p $opt >output &&\n+\t\tverify_state dir/foo work head &&\n+\t\tverify_saved_state bar &&\n+\t\ttest_grep \"Unstage\" output\n+\t'\n+done\n+\n test_expect_success PERL 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n-- \n2.43.0\n\n"},{"id":"487469","messageId":"20240128181202.986753-4-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240128181202.986753-2-shyamthakkar001@gmail.com","subject":"[RFC PATCH 2/2] checkout: remove HEAD special case","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-28T18:11:23Z","receivedAt":"2024-01-28T18:19:16Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"run_add_p() is capable of handling HEAD in any form (e.g. hex, 'HEAD',\n'@' etc.), not just string 'HEAD'. Therefore, special casing of HEAD\ndoes not have any effect.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/checkout.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6e30931b5..6b74e5fa4e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -539,12 +539,11 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * recognized by diff-index), we will always replace the name\n \t\t * with the hex of the commit (whether it's in `...` form or\n \t\t * not) for the run_add_interactive() machinery to work\n-\t\t * properly. However, there is special logic for the HEAD case\n-\t\t * so we mustn't replace that. Also, when we were given a\n-\t\t * tree-object, new_branch_info->commit would be NULL, but we\n-\t\t * do not have to do any replacement, either.\n+\t\t * properly. Also, when we were given a tree-object,\n+\t\t * new_branch_info->commit would be NULL, but we do not\n+\t\t * have to do any replacement.\n \t\t */\n-\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n+\t\tif (rev && new_branch_info->commit)\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n \n \t\tif (opts->checkout_index && opts->checkout_worktree)\n-- \n2.43.0\n\n"},{"id":"487514","messageId":"ZbeQmv_KcChtrPqJ@tanuki","threadId":"60801","inReplyTo":"20240128181202.986753-3-shyamthakkar001@gmail.com","subject":"Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:48:42Z","receivedAt":"2024-01-29T11:48:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 28, 2024 at 11:41:22PM +0530, Ghanshyam Thakkar wrote:\n\nWe typically start commit messages with an explanation of what the\nactual problem is that the commit is trying to solve. This helps to set\nthe stage for any reviewers so that they know why you're doing changes\nin the first place.\n\n> Add a new function reveq(), which takes repository struct and two revision\n> strings as arguments and returns 0 if the revisions point to the same\n> object. Passing a rev which does not point to an object is considered\n> undefined behavior as the underlying function memcmp() will be called\n> with NULL hash strings.\n> \n> Subsequently, replace literal string comparison to HEAD in run_add_p()\n> with reveq() to handle more ways of saying HEAD (such as '@' or '$branch'\n> where $branch points to same commit as HEAD). This addresses the\n> NEEDSWORK comment in run_add_p().\n> \n> However, in ADD_P_RESET mode keep string comparison in logical OR with\n> reveq() to handle unborn HEAD.\n> \n> As for the behavior change, with this patch applied if the given\n> revision points to the same object as HEAD, the patch mode will be set to\n> patch_mode_(reset,checkout,worktree)_head instead of\n> patch_mode_(...)_nothead. That is equivalent of not setting -R flag in\n> diff-index, which would have been otherwise set before this patch.\n> However, when given same set of inputs, the actual outcome is same as\n> before this patch. Therefore, this does not affect any automated scripts.\n\nSo this is the closest to an actual description of what your goal is.\nBut it doesn't say why that is a good idea, it only explains the change\nin behaviour.\n\nI think the best thing to do would be to give a sequence of Git commands\nthat demonstrate the problem that you are trying to solve. This would\nhelp the reader gain a high-level understanding of what you propose to\nchange.\n\n> Also, add testcases to check the similarity of result between different\n> ways of saying HEAD.\n> \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> Should the return values of repo_get_oid() be checked in reveq()? As\n> reveq() is not a global function and is only used in run_add_p(), the\n> validity of revisions is already checked beforehand by builtin/checkout.c\n> and builtin/reset.c before the call to run_add_p().\n> \n> add-patch.c | 28 +++++++++++++++-------\n> t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------\n> t/t2071-restore-patch.sh | 21 ++++++++++------\n> t/t7105-reset-patch.sh | 14 +++++++++++\n> 4 files changed, 77 insertions(+), 36 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 79eda168eb..01eb71d90e 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -14,6 +14,7 @@\n> #include \"color.h\"\n> #include \"compat/terminal.h\"\n> #include \"prompt.h\"\n> +#include \"hash.h\"\n> \n> enum prompt_mode_type {\n> \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n> @@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,\n> \t\t INDEX_ENVIRONMENT \"=%s\", s->s.r->index_file);\n> }\n> \n> +// Check if two revisions point to the same object. Passing a rev which does not\n> +// point to an object is undefined behavior.\n\nWe only use `/* */`-style comments in the Git codebase.\n\n> +static inline int reveq(struct repository *r, const char *rev1,\n> +\t\t\tconst char *rev2)\n> +{\n> +\tstruct object_id oid_rev1, oid_rev2;\n> +\trepo_get_oid(r, rev1, &oid_rev1);\n> +\trepo_get_oid(r, rev2, &oid_rev2);\n> +\n> +\treturn !oideq(&oid_rev1, &oid_rev2);\n> +}\n\nI don't think it's a good idea to allow for undefined behaviour here.\nWhile more tedious for the caller, I think it's preferable to handle the\ncase correctly where revisions don't resolve, e.g. by returning `-1` in\ncase either of the revisions does not resolve.\n\nPatrick\n"},{"id":"487515","messageId":"ZbeQoDh_MPDDufk3@tanuki","threadId":"60801","inReplyTo":"20240128181202.986753-4-shyamthakkar001@gmail.com","subject":"Re: [RFC PATCH 2/2] checkout: remove HEAD special case","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:48:48Z","receivedAt":"2024-01-29T11:48:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 28, 2024 at 11:41:23PM +0530, Ghanshyam Thakkar wrote:\n> run_add_p() is capable of handling HEAD in any form (e.g. hex, 'HEAD',\n> '@' etc.), not just string 'HEAD'. Therefore, special casing of HEAD\n> does not have any effect.\n\nAre there any tests that cover this behaviour? If so, it would be nice\nto point them out in the commit message. Otherwise, we should add them.\n\nPatrick\n\n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> builtin/checkout.c | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index a6e30931b5..6b74e5fa4e 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -539,12 +539,11 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \t\t * recognized by diff-index), we will always replace the name\n> \t\t * with the hex of the commit (whether it's in `...` form or\n> \t\t * not) for the run_add_interactive() machinery to work\n> -\t\t * properly. However, there is special logic for the HEAD case\n> -\t\t * so we mustn't replace that. Also, when we were given a\n> -\t\t * tree-object, new_branch_info->commit would be NULL, but we\n> -\t\t * do not have to do any replacement, either.\n> +\t\t * properly. Also, when we were given a tree-object,\n> +\t\t * new_branch_info->commit would be NULL, but we do not\n> +\t\t * have to do any replacement.\n> \t\t */\n> -\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n> +\t\tif (rev && new_branch_info->commit)\n> \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n> \n> \t\tif (opts->checkout_index && opts->checkout_worktree)\n> -- \n> 2.43.0\n> \n> \n"},{"id":"487537","messageId":"xmqqmssohu69.fsf@gitster.g","threadId":"60801","inReplyTo":"20240128181202.986753-3-shyamthakkar001@gmail.com","subject":"Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T18:27:10Z","receivedAt":"2024-01-29T18:27:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Add a new function reveq(), which takes repository struct and two revision\n> strings as arguments and returns 0 if the revisions point to the same\n> object. Passing a rev which does not point to an object is considered\n> undefined behavior as the underlying function memcmp() will be called\n> with NULL hash strings.\n\nI didn't dug into the patch or the codepath it touches, but\nI wonder if it has something (possibly historical) to do with the\nfact that these two behave quite differently:\n\n $ git checkout HEAD\n $ git checkout HEAD^0\n\nWith the former, you will stay on your current branch, while with\nthe latter you detach at the commit at the tip of your current\nbranch. Granted that \"git checkout -p\" is not about moving HEAD but\nchecking out some files to the worktree from a given tree-ish, anytime\nI see code that does strcmp() with a fixed \"HEAD\" string, that is\none consideration I'd look for.\n\n> Should the return values of repo_get_oid() be checked in reveq()? As\n> reveq() is not a global function and is only used in run_add_p(), the\n> validity of revisions is already checked beforehand by builtin/checkout.c\n> and builtin/reset.c before the call to run_add_p().\n\nIf this were to become a public function (even if it somehow turns\nout that it is a bad idea to move away from an explicit comparison\nwith \"HEAD\", introducing such a function might be useful---I dunno),\nit probably makes sense not to burden its potential callers with too\nmany assumption. But doesn't the fact that the immediate callers\nyou are introducing already checked the validity of the revisions\ntell us something? Would it result in us not needing this new\nhelper function at all, if we rearranged the code that already\nchecks the validity so that the actual object names are collected?\nThen it would become the matter of running oideq() directly on these\nobject names, instead of calling a new helper function that\n(re)converts from strings to object names and compares them.\n\n> +// Check if two revisions point to the same object. Passing a rev which does not\n> +// point to an object is undefined behavior.\n\nStyle:\n\n /*\n * Our multi-line comments look\n * like this.\n */\n\n> +static inline int reveq(struct repository *r, const char *rev1,\n> +\t\t\tconst char *rev2)\n> +{\n> +\tstruct object_id oid_rev1, oid_rev2;\n> +\trepo_get_oid(r, rev1, &oid_rev1);\n> +\trepo_get_oid(r, rev2, &oid_rev2);\n> +\n> +\treturn !oideq(&oid_rev1, &oid_rev2);\n\nHorribly confusing. If oideq() says \"yes, they are the same\" by\nreturning 0, then any helper function derived from it to ansewr \"are\nX and Y the same?\" should return 0 when it wants to say \"yes, they\nare the same\" to help developers keep their sanity.\n\n> +}\n> +\n> static int parse_range(const char **p,\n> \t\t unsigned long *offset, unsigned long *count)\n> {\n> @@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> \t\ts.mode = &patch_mode_stash;\n> \telse if (mode == ADD_P_RESET) {\n> \t\t/*\n> -\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n> -\t\t * compare the commit objects instead so that other ways of\n> -\t\t * saying the same thing (such as \"@\") are also handled\n> -\t\t * appropriately.\n> -\t\t *\n> -\t\t * This applies to the cases below too.\n> +\t\t * The literal string comparison to HEAD below is kept\n> +\t\t * to handle unborn HEAD.\n> \t\t */\n\nSo, does this change solve the NEEDSWORK comment? On an unborn\nHEAD, this would still not allow you to say \"@\". Only \"HEAD\" is\nsupported.\n\nNot that I necessarily agree with the original \"NEEDSWORK\" comment\n(I think it is perfectly fine for this or any other codepaths not to\ntake \"@\" as \"HEAD\"), but if that desire still stands here, should\nthe resulting comment still mention it with a NEEDSWORK label?\n\nBesides ...\n\n> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\n> index 747eb5563e..431f34fa9c 100755\n> --- a/t/t2016-checkout-patch.sh\n> +++ b/t/t2016-checkout-patch.sh\n> @@ -12,6 +12,7 @@ test_expect_success 'setup' '\n> \tgit commit -m initial &&\n> \ttest_tick &&\n> \ttest_commit second dir/foo head &&\n> +\tgit branch newbranch &&\n> \tset_and_save_state bar bar_work bar_index &&\n> \tsave_head\n> '\n> +# Note: 'newbranch' points to the same commit as HEAD. And it is technically\n> +# allowed to name a branch '@' as of now, however in below test '@'\n> +# represents the shortcut for HEAD.\n> +for opt in \"HEAD\" \"@\" \"newbranch\"\n> +do\n> +\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n> +\t\tset_and_save_state dir/foo work head &&\n> +\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n> +\t\tverify_saved_state bar &&\n> +\t\tverify_saved_state dir/foo &&\n> +\t\ttest_grep \"Discard\" output\n> +\t'\n\nI think this change in behaviour, especially for \"newbranch\" that\nused to use the \"_nothead\" variants of directions and messages, is\nway too confusing. Users may consider \"HEAD\" and \"@\" the same and\nmay want them to behave the same way, but the user, when explicitly\nnaming \"newbranch\", means they want to \"check contents out of that\nOTHER thing named 'newbranch', not the current branch\"; it may or\nmay not happen to be pointing at the same commit as HEAD, but if\nthe user meant to say \"check contents out of the current commit,\n(partially) reverting the local changes I have\", the user would have\nsaid HEAD. After all, the user may not even be immediately aware\nthat \"newbranch\" happens to point at the same commit as HEAD.\n\nSo, after thinking about it a bit more, I do not think I agree with\nthe NEEDSWORK comment. I can buy \"@\", but not an arbitrary revision\nname that happens to point at the same commit as HEAD. In other\nwords, I may be persuaded to thinking into it is a good idea to add:\n\n static inline int user_means_HEAD(const char *a)\n {\n\treturn !strcmp(a, \"HEAD\") || !strcmp(a, \"@\");\n }\n\nand replace \"!strcmp(rev, \"HEAD\")\" with \"user_means_HEAD(rev)\", but\nI would not go any further than that.\n\nThanks.\n\n\n\n"},{"id":"487540","messageId":"xmqq8r48hspd.fsf@gitster.g","threadId":"60801","inReplyTo":"xmqqmssohu69.fsf@gitster.g","subject":"Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T18:58:54Z","receivedAt":"2024-01-29T18:58:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So, after thinking about it a bit more, I do not think I agree with\n> the NEEDSWORK comment. I can buy \"@\", but not an arbitrary revision\n> name that happens to point at the same commit as HEAD. \n\nOne more thing is it might make sense, if we were to allow more than\nthe literal string \"HEAD\", is to include the name of the current\nbranch (e.g., if \"git symbolic-ref HEAD\" says \"refs/heads/main\",\nthen \"main\") to the set of tokens that the user may use when they\nmean to refer to \"HEAD\". Unlike \"newbranch\" they are not currently\non, if they know what branch they are on and they know that is what\nHEAD refers to, so the likelihood of them wanting to see the command\nbehave (i.e. the direction of the patch to be selected and the\nmessages) the same way may be much higher, I would suspect.\n\nBut still, the sudden reversal of the direction of the patches may\nbring unexpected confusions to uses. I dunno.\n\n> In other\n> words, I may be persuaded to thinking into it is a good idea to add:\n>\n> static inline int user_means_HEAD(const char *a)\n> {\n> \treturn !strcmp(a, \"HEAD\") || !strcmp(a, \"@\");\n> }\n>\n> and replace \"!strcmp(rev, \"HEAD\")\" with \"user_means_HEAD(rev)\", but\n> I would not go any further than that.\n>\n> Thanks.\n"},{"id":"487578","messageId":"CYRSP93JDLV9.2U9KAZI4XDCFV@gmail.com","threadId":"60801","inReplyTo":"xmqqmssohu69.fsf@gitster.g","subject":"Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-30T05:35:21Z","receivedAt":"2024-01-30T05:35:26Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon Jan 29, 2024 at 11:57 PM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n>\n> > Add a new function reveq(), which takes repository struct and two revision\n> > strings as arguments and returns 0 if the revisions point to the same\n> > object. Passing a rev which does not point to an object is considered\n> > undefined behavior as the underlying function memcmp() will be called\n> > with NULL hash strings.\n>\n> I didn't dug into the patch or the codepath it touches, but\n> I wonder if it has something (possibly historical) to do with the\n> fact that these two behave quite differently:\n>\n> $ git checkout HEAD\n> $ git checkout HEAD^0\n>\n> With the former, you will stay on your current branch, while with\n> the latter you detach at the commit at the tip of your current\n> branch. Granted that \"git checkout -p\" is not about moving HEAD but\n> checking out some files to the worktree from a given tree-ish, anytime\n> I see code that does strcmp() with a fixed \"HEAD\" string, that is\n> one consideration I'd look for.\n>\n> > Should the return values of repo_get_oid() be checked in reveq()? As\n> > reveq() is not a global function and is only used in run_add_p(), the\n> > validity of revisions is already checked beforehand by builtin/checkout.c\n> > and builtin/reset.c before the call to run_add_p().\n>\n> If this were to become a public function (even if it somehow turns\n> out that it is a bad idea to move away from an explicit comparison\n> with \"HEAD\", introducing such a function might be useful---I dunno),\n> it probably makes sense not to burden its potential callers with too\n> many assumption. But doesn't the fact that the immediate callers\n> you are introducing already checked the validity of the revisions\n> tell us something? Would it result in us not needing this new\n> helper function at all, if we rearranged the code that already\n> checks the validity so that the actual object names are collected?\n> Then it would become the matter of running oideq() directly on these\n> object names, instead of calling a new helper function that\n> (re)converts from strings to object names and compares them.\n>\n> > +// Check if two revisions point to the same object. Passing a rev which does not\n> > +// point to an object is undefined behavior.\n>\n> Style:\n>\n> /*\n> * Our multi-line comments look\n> * like this.\n> */\n>\n> > +static inline int reveq(struct repository *r, const char *rev1,\n> > +\t\t\tconst char *rev2)\n> > +{\n> > +\tstruct object_id oid_rev1, oid_rev2;\n> > +\trepo_get_oid(r, rev1, &oid_rev1);\n> > +\trepo_get_oid(r, rev2, &oid_rev2);\n> > +\n> > +\treturn !oideq(&oid_rev1, &oid_rev2);\n>\n> Horribly confusing. If oideq() says \"yes, they are the same\" by\n> returning 0, then any helper function derived from it to ansewr \"are\n> X and Y the same?\" should return 0 when it wants to say \"yes, they\n> are the same\" to help developers keep their sanity.\n>\n> > +}\n> > +\n> > static int parse_range(const char **p,\n> > \t\t unsigned long *offset, unsigned long *count)\n> > {\n> > @@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> > \t\ts.mode = &patch_mode_stash;\n> > \telse if (mode == ADD_P_RESET) {\n> > \t\t/*\n> > -\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n> > -\t\t * compare the commit objects instead so that other ways of\n> > -\t\t * saying the same thing (such as \"@\") are also handled\n> > -\t\t * appropriately.\n> > -\t\t *\n> > -\t\t * This applies to the cases below too.\n> > +\t\t * The literal string comparison to HEAD below is kept\n> > +\t\t * to handle unborn HEAD.\n> > \t\t */\n>\n> So, does this change solve the NEEDSWORK comment? On an unborn\n> HEAD, this would still not allow you to say \"@\". Only \"HEAD\" is\n> supported.\n\nThe reset command does not support naming anything on the unborn HEAD.\nMeaning, on an unborn HEAD, using both 'git reset -p @' and 'git reset\n-p HEAD' error out. However in case of 'git reset -p' on an unborn\nHEAD, it works becuase it skips the validity checks for the revision\nstring in parse_args() in builtin/reset.c due to the absense of any\narguments. Afterwards the empty revision is replaced by 'HEAD'.\nTherefore, the string comparison to HEAD is not for supporting\nHEAD, but it is an indication to parse_diff() in add-patch.c, which\nreplaces that 'HEAD' with empty_tree_oid_hex().\n\nIn short, on unborn HEAD, 'git reset -p' passes 'HEAD' as revision\nstring to be replaced by empty_tree_oid_hex() in parse_diff().\nrelevant code lines from add-patch.c:\n\nstatic int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n{\n ...\n\tif (s->revision) {\n\t\tstruct object_id oid;\n\t\tstrvec_push(&args,\n\t\t\t /* could be on an unborn branch */\n\t\t\t !strcmp(\"HEAD\", s->revision) &&\n\t\t\t repo_get_oid(the_repository, \"HEAD\", &oid) ?\n\t\t\t empty_tree_oid_hex() : s->revision);\n\t}\n\t...\n}\n\nPerhaps, I should have clarified that in the commit message or comment.\n\n> Not that I necessarily agree with the original \"NEEDSWORK\" comment\n> (I think it is perfectly fine for this or any other codepaths not to\n> take \"@\" as \"HEAD\"), but if that desire still stands here, should\n> the resulting comment still mention it with a NEEDSWORK label?\n>\n> Besides ...\n>\n> > diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\n> > index 747eb5563e..431f34fa9c 100755\n> > --- a/t/t2016-checkout-patch.sh\n> > +++ b/t/t2016-checkout-patch.sh\n> > @@ -12,6 +12,7 @@ test_expect_success 'setup' '\n> > \tgit commit -m initial &&\n> > \ttest_tick &&\n> > \ttest_commit second dir/foo head &&\n> > +\tgit branch newbranch &&\n> > \tset_and_save_state bar bar_work bar_index &&\n> > \tsave_head\n> > '\n> > +# Note: 'newbranch' points to the same commit as HEAD. And it is technically\n> > +# allowed to name a branch '@' as of now, however in below test '@'\n> > +# represents the shortcut for HEAD.\n> > +for opt in \"HEAD\" \"@\" \"newbranch\"\n> > +do\n> > +\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n> > +\t\tset_and_save_state dir/foo work head &&\n> > +\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n> > +\t\tverify_saved_state bar &&\n> > +\t\tverify_saved_state dir/foo &&\n> > +\t\ttest_grep \"Discard\" output\n> > +\t'\n>\n> I think this change in behaviour, especially for \"newbranch\" that\n> used to use the \"_nothead\" variants of directions and messages, is\n> way too confusing. Users may consider \"HEAD\" and \"@\" the same and\n> may want them to behave the same way, but the user, when explicitly\n> naming \"newbranch\", means they want to \"check contents out of that\n> OTHER thing named 'newbranch', not the current branch\"; it may or\n> may not happen to be pointing at the same commit as HEAD, but if\n> the user meant to say \"check contents out of the current commit,\n> (partially) reverting the local changes I have\", the user would have\n> said HEAD. After all, the user may not even be immediately aware\n> that \"newbranch\" happens to point at the same commit as HEAD.\n>\n> So, after thinking about it a bit more, I do not think I agree with\n> the NEEDSWORK comment. I can buy \"@\", but not an arbitrary revision\n> name that happens to point at the same commit as HEAD. In other\n> words, I may be persuaded to thinking into it is a good idea to add:\n>\n> static inline int user_means_HEAD(const char *a)\n> {\n> \treturn !strcmp(a, \"HEAD\") || !strcmp(a, \"@\");\n> }\n>\n> and replace \"!strcmp(rev, \"HEAD\")\" with \"user_means_HEAD(rev)\", but\n> I would not go any further than that.\n\nYes, however, '@' can also be a branch name. And there is also the case\nof '@' being a branch which points to same commit as HEAD, which would\nagain be confusing as you pointed out above, if \"_head\" variant is used\nin that. For this to work, we would need to check if a branch named '@'\nexists and if it does, then '@' should not be treated as a shortcut for\nHEAD. (or should it still be treated as a shortcut for HEAD? As 'git\npush origin @' pushes HEAD to remote inspite of a branch named '@' existing\nlocally at a different commit than HEAD).\n\nThanks.\n"},{"id":"487590","messageId":"CYRU26F9KCDF.2XI7VRT7N04OC@gmail.com","threadId":"60801","inReplyTo":"ZbeQmv_KcChtrPqJ@tanuki","subject":"Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-30T06:39:15Z","receivedAt":"2024-01-30T06:39:20Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon Jan 29, 2024 at 5:18 PM IST, Patrick Steinhardt wrote:\n> On Sun, Jan 28, 2024 at 11:41:22PM +0530, Ghanshyam Thakkar wrote:\n>\n> We typically start commit messages with an explanation of what the\n> actual problem is that the commit is trying to solve. This helps to set\n> the stage for any reviewers so that they know why you're doing changes\n> in the first place.\n\nI will keep that in mind for future patches.\n\n> > Add a new function reveq(), which takes repository struct and two revision\n> > strings as arguments and returns 0 if the revisions point to the same\n> > object. Passing a rev which does not point to an object is considered\n> > undefined behavior as the underlying function memcmp() will be called\n> > with NULL hash strings.\n> > \n> > Subsequently, replace literal string comparison to HEAD in run_add_p()\n> > with reveq() to handle more ways of saying HEAD (such as '@' or '$branch'\n> > where $branch points to same commit as HEAD). This addresses the\n> > NEEDSWORK comment in run_add_p().\n> > \n> > However, in ADD_P_RESET mode keep string comparison in logical OR with\n> > reveq() to handle unborn HEAD.\n> > \n> > As for the behavior change, with this patch applied if the given\n> > revision points to the same object as HEAD, the patch mode will be set to\n> > patch_mode_(reset,checkout,worktree)_head instead of\n> > patch_mode_(...)_nothead. That is equivalent of not setting -R flag in\n> > diff-index, which would have been otherwise set before this patch.\n> > However, when given same set of inputs, the actual outcome is same as\n> > before this patch. Therefore, this does not affect any automated scripts.\n>\n> So this is the closest to an actual description of what your goal is.\n> But it doesn't say why that is a good idea, it only explains the change\n> in behaviour.\n>\n> I think the best thing to do would be to give a sequence of Git commands\n> that demonstrate the problem that you are trying to solve. This would\n> help the reader gain a high-level understanding of what you propose to\n> change.\n\nYeah, my original motive was to support '@' as a shorthand for HEAD.\nBut, since '@' can also be used as branch name, I thought of comparing\nobject ids instead of string comparison in accordance with the\nNEEDSWORK comment. However, as Junio pointed out, treating a branch\nname revision that points to same commit as HEAD, as HEAD would just\ncause confusion.\n\n> > Also, add testcases to check the similarity of result between different\n> > ways of saying HEAD.\n> > \n> > Signed-off-by: Ghanshyam Thakkar \n> > ---\n> > Should the return values of repo_get_oid() be checked in reveq()? As\n> > reveq() is not a global function and is only used in run_add_p(), the\n> > validity of revisions is already checked beforehand by builtin/checkout.c\n> > and builtin/reset.c before the call to run_add_p().\n> > \n> > add-patch.c | 28 +++++++++++++++-------\n> > t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------\n> > t/t2071-restore-patch.sh | 21 ++++++++++------\n> > t/t7105-reset-patch.sh | 14 +++++++++++\n> > 4 files changed, 77 insertions(+), 36 deletions(-)\n> > \n> > diff --git a/add-patch.c b/add-patch.c\n> > index 79eda168eb..01eb71d90e 100644\n> > --- a/add-patch.c\n> > +++ b/add-patch.c\n> > @@ -14,6 +14,7 @@\n> > #include \"color.h\"\n> > #include \"compat/terminal.h\"\n> > #include \"prompt.h\"\n> > +#include \"hash.h\"\n> > \n> > enum prompt_mode_type {\n> > \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n> > @@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,\n> > \t\t INDEX_ENVIRONMENT \"=%s\", s->s.r->index_file);\n> > }\n> > \n> > +// Check if two revisions point to the same object. Passing a rev which does not\n> > +// point to an object is undefined behavior.\n>\n> We only use `/* */`-style comments in the Git codebase.\n>\n> > +static inline int reveq(struct repository *r, const char *rev1,\n> > +\t\t\tconst char *rev2)\n> > +{\n> > +\tstruct object_id oid_rev1, oid_rev2;\n> > +\trepo_get_oid(r, rev1, &oid_rev1);\n> > +\trepo_get_oid(r, rev2, &oid_rev2);\n> > +\n> > +\treturn !oideq(&oid_rev1, &oid_rev2);\n> > +}\n>\n> I don't think it's a good idea to allow for undefined behaviour here.\n> While more tedious for the caller, I think it's preferable to handle the\n> case correctly where revisions don't resolve, e.g. by returning `-1` in\n> case either of the revisions does not resolve.\n\nWill update it. \n\nThanks.\n\n"},{"id":"487601","messageId":"xmqqsf2ebwnh.fsf@gitster.g","threadId":"60801","inReplyTo":"CYRU26F9KCDF.2XI7VRT7N04OC@gmail.com","subject":"Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-30T16:42:26Z","receivedAt":"2024-01-30T16:42:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ghanshyam Thakkar\" writes:\n\n> Yeah, my original motive was to support '@' as a shorthand for HEAD.\n> But, since '@' can also be used as branch name, I thought of comparing\n> object ids instead of string comparison in accordance with the\n> NEEDSWORK comment. However, as Junio pointed out, treating a branch\n> name revision that points to same commit as HEAD, as HEAD would just\n> cause confusion.\n\nFWIW, if we are not doing so in our documentation already, we may\nwant to discourage use of \"refs/heads/@\", given that \"@\" is used as\na synonym for \"HEAD\" in some[*] contexts, specifying the HEAD\n(i.e. \"work on the branch that is currently checked out, or in the\ndetached state\") and specifying the concrete name of a branch\n(i.e. \"work on this branch\") mean totally different things and may\nresult in (what may appear to the user as a) confusing behaviour.\n\nGranted, the user who names their branch \"@\" is only hurting\nthemselves and it falls into the \"Doctor, it hurts when I do\nthis. Then don't do that!\" category. \n\nBut the documentation is where we tell them \"Then don't do that!\"\nand we should know better how it hurts when they do so than those\nwho learn from the documentation, so ...\n\n\n[Footnote]\n\n * Even use of \"@\" as a synonym for \"HEAD\" may want to be\n discouraged, as there are still unnecessary differences between\n them that are not worth our engineering resource to fix. Do\n people know what \"git checkout HEAD\" and \"git checkout @\" do, for\n example?\n"},{"id":"487818","messageId":"20240202150434.11256-1-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240128181202.986753-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 0/2] add-patch: Support '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-02T15:03:48Z","receivedAt":"2024-02-02T15:05:16Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This patch series removes a non-relevant NEEDSWORK comment and addresses\ndisparity between '@' and 'HEAD' in patch mode for (checkout, restore,\nreset) commands.\n\nThe removal of the NEEDSWORK comment and the '@' support are split into\ndifferent patches because the former is still up for discussion. And if\nit is decided against, the NEEDSWORK comment can still go as it would not\nbe the desired solution anyway (described in the commit message).\n\nGhanshyam Thakkar (2):\n add-patch: remove unnecessary NEEDSWORK comment\n add-patch: classify '@' as a synonym for 'HEAD'\n\n add-patch.c | 19 +++++++---------\n builtin/checkout.c | 11 +++++-----\n t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 10 +++++++++\n 5 files changed, 61 insertions(+), 43 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"487819","messageId":"20240202150434.11256-2-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240202150434.11256-1-shyamthakkar001@gmail.com","subject":"[PATCH v2 1/2] add-patch: remove non-relevant NEEDSWORK comment","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-02T15:03:49Z","receivedAt":"2024-02-02T15:05:20Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The comment suggested to compare commit objects instead of string\ncomparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.\n'@'). However, this approach has unintended behavior of also counting a\nnon-checked out branch pointing to same commit as HEAD, as HEAD. This\nwould cause confusion to the user.\n\nJunio described it best as[1]:\n\n\"Users may consider 'HEAD' and '@' the same and\nmay want them to behave the same way, but the user, when explicitly\nnaming '$branch', means they want to \"check contents out of that\nOTHER thing named '$branch', not the current branch\"; it may or\nmay not happen to be pointing at the same commit as HEAD, but if\nthe user meant to say \"check contents out of the current commit,\n(partially) reverting the local changes I have\", the user would have\nsaid HEAD. After all, the user may not even be immediately aware\nthat '$branch' happens to point at the same commit as HEAD.\"\n\n[1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/\n\nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..68f525b35c 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\t/*\n-\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n-\t\t * compare the commit objects instead so that other ways of\n-\t\t * saying the same thing (such as \"@\") are also handled\n-\t\t * appropriately.\n-\t\t *\n-\t\t * This applies to the cases below too.\n-\t\t */\n \t\tif (!revision || !strcmp(revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n-- \n2.43.0\n\n"},{"id":"487820","messageId":"20240202150434.11256-3-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240202150434.11256-1-shyamthakkar001@gmail.com","subject":"[PATCH v2 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-02T15:03:50Z","receivedAt":"2024-02-02T15:05:23Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Currently, (checkout, reset, restore) commands correctly take '@' as a\nsynonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and\n'HEAD', different prompts/messages are given by the commands mentioned\nabove. This is due to the literal and only string comparison with the word\n'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously\ndesired, especially since '@' already resolves to 'HEAD'.\n\nTherefore, make a new function user_meant_head() which takes the\nrevision string and compares it to 'HEAD' as well as '@'. However, in\nbuiltin/checkout.c, there is some logic to convert all revs besides\n'HEAD' to hex revs due to 'diff-index', which is used in patch mode\nmachinery, not being able to handle '...' revs. Therefore, in\naddition to 'HEAD', do not convert '@' as well, so it can be later\nused in assigning patch_mode_(...)_head.\n\nThere is one unintended side-effect/behavior change of this, even if\nthere exists a branch named '@', when providing '@' as a rev-source to\n(checkout, reset, restore) commands in patch mode, it will consider it\nas HEAD. This is due to the behavior of diff-index. However, naming a\nbranch '@' is an obvious foot-gun and there are many existing commands\nwhich take '@' for 'HEAD' even if 'refs/heads/@' exists (e.g., 'git log\n@', 'git push origin @' etc.). Therefore, this should be fine.\n\nAlso, add tests to check the above mentioned synonymity between 'HEAD'\nand '@'.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 11 +++++++---\n builtin/checkout.c | 11 +++++-----\n t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 10 +++++++++\n 5 files changed, 61 insertions(+), 35 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 68f525b35c..6c70a0240c 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \treturn 0;\n }\n \n+static inline int is_rev_head(const char *rev)\n+{\n+\treturn !strcmp(rev, \"HEAD\") || !strcmp(rev, \"@\");\n+}\n+\n static int is_octal(const char *p, size_t len)\n {\n \tif (!len)\n@@ -1729,21 +1734,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\tif (!revision || !strcmp(revision, \"HEAD\"))\n+\t\tif (!revision || is_rev_head(revision))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_reset_nothead;\n \t} else if (mode == ADD_P_CHECKOUT) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (is_rev_head(revision))\n \t\t\ts.mode = &patch_mode_checkout_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_checkout_nothead;\n \t} else if (mode == ADD_P_WORKTREE) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (is_rev_head(revision))\n \t\t\ts.mode = &patch_mode_worktree_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_worktree_nothead;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6e30931b5..79e208ee6d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * recognized by diff-index), we will always replace the name\n \t\t * with the hex of the commit (whether it's in `...` form or\n \t\t * not) for the run_add_interactive() machinery to work\n-\t\t * properly. However, there is special logic for the HEAD case\n-\t\t * so we mustn't replace that. Also, when we were given a\n-\t\t * tree-object, new_branch_info->commit would be NULL, but we\n-\t\t * do not have to do any replacement, either.\n+\t\t * properly. However, there is special logic for the 'HEAD' and\n+\t\t * '@' case so we mustn't replace that. Also, when we were\n+\t\t * given a tree-object, new_branch_info->commit would be NULL,\n+\t\t * but we do not have to do any replacement, either.\n \t\t */\n-\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n+\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n+\t\t strcmp(rev, \"@\"))\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n \n \t\tif (opts->checkout_index && opts->checkout_worktree)\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 747eb5563e..c4f9bf09aa 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '\n-\tset_and_save_state dir/foo work head &&\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_saved_state dir/foo\n-'\n-\n-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '\n-\ttest_write_lines n y y | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n-\n-test_expect_success 'git checkout -p HEAD with change already staged' '\n-\tset_state dir/foo index index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n+\t\tset_and_save_state dir/foo work head &&\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_saved_state dir/foo &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: apply\" '\n+\t\ttest_write_lines n y y | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with change already staged\" '\n+\t\tset_state dir/foo index index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success 'git checkout -p HEAD^...' '\n \t# the third n is to get out in case it mistakenly does not apply\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex b5c5c0ff7e..3dc9184b4a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD' '\n-\tset_state dir/foo work index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git restore -p --source=HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head index\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n+\t\tset_state dir/foo work index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head index &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 05079c7246..ec7f16dfb6 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' '\n \ttest_grep \"Unstage\" output\n '\n \n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git reset -p $opt\" '\n+\t\ttest_write_lines n y | git reset -p $opt >output &&\n+\t\tverify_state dir/foo work head &&\n+\t\tverify_saved_state bar &&\n+\t\ttest_grep \"Unstage\" output\n+\t'\n+done\n+\n test_expect_success PERL 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n-- \n2.43.0\n\n"},{"id":"487828","messageId":"xmqqh6iqu73y.fsf@gitster.g","threadId":"60801","inReplyTo":"20240202150434.11256-3-shyamthakkar001@gmail.com","subject":"Re: [PATCH v2 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-02T17:08:17Z","receivedAt":"2024-02-02T17:08:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Currently, (checkout, reset, restore) commands correctly take '@' as a\n> synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and\n> 'HEAD', different prompts/messages are given by the commands mentioned\n> above. This is due to the literal and only string comparison with the word\n> 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously\n> desired, especially since '@' already resolves to 'HEAD'.\n>\n> Therefore, make a new function user_meant_head() which takes the\n> revision string and compares it to 'HEAD' as well as '@'. However, in\n> builtin/checkout.c, there is some logic to convert all revs besides\n> 'HEAD' to hex revs due to 'diff-index', which is used in patch mode\n> machinery, not being able to handle '...' revs. Therefore, in\n> addition to 'HEAD', do not convert '@' as well, so it can be later\n> used in assigning patch_mode_(...)_head.\n\nIn this context ... names a single rev (not two revs) that is\nthe merge base of and . Perhaps\n\n ... there is a logic to convert all command line input rev to\n the raw object name for underlying machinery (e.g., diff-index)\n that does not recognize the ... notation, but we'd need to\n leave \"HEAD\" intact. Now we need to teach that \"@\" is a synonym\n to \"HEAD\" to that code, too.\n\nwhich may be a bit shorter.\n\nYou decided to use is_rev_head() instead of user_meant_head(), so\nyou'd need to update the above description to match, I think.\n\n> -\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n> +\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n> +\t\t strcmp(rev, \"@\"))\n\nShouldn't this be\n\n\t\tif (rev && new_branch_info->commit && !is_rev_head(rev))\n\ninstead of \"HEAD\" and \"@\" spelled out?\n\nOther than the above, nicely done.\n"},{"id":"487831","messageId":"CYURSXQPY5QA.34WZH01SLKI55@gmail.com","threadId":"60801","inReplyTo":"20240202150434.11256-1-shyamthakkar001@gmail.com","subject":"Re: [PATCH v2 0/2] add-patch: Support '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-02T17:31:09Z","receivedAt":"2024-02-02T17:31:14Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Feb 2, 2024 at 8:33 PM IST, Ghanshyam Thakkar wrote:\n> This patch series removes a non-relevant NEEDSWORK comment and addresses\n> disparity between '@' and 'HEAD' in patch mode for (checkout, restore,\n> reset) commands.\n>\n> The removal of the NEEDSWORK comment and the '@' support are split into\n> different patches because the former is still up for discussion. And if\ns/former/latter/\n\n> it is decided against, the NEEDSWORK comment can still go as it would not\n> be the desired solution anyway (described in the commit message).\n"},{"id":"487833","messageId":"xmqqv876sqwf.fsf@gitster.g","threadId":"60801","inReplyTo":"xmqqh6iqu73y.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-02T17:43:44Z","receivedAt":"2024-02-02T17:43:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> You decided to use is_rev_head() instead of user_meant_head(), so\n> you'd need to update the above description to match, I think.\n\nHaving said this, I have a slight fear that normal users would\nexpect is_rev_head(X) to say \"yes\" for \"master\" when the current\nbranch is \"master\", though. is_head(X) would have the same\ndownside.\n\n\n\n"},{"id":"487835","messageId":"CYUS8RWV9JY6.15PV2RSRKOXMP@gmail.com","threadId":"60801","inReplyTo":"xmqqh6iqu73y.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-02T17:51:50Z","receivedAt":"2024-02-02T17:51:54Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Feb 2, 2024 at 10:38 PM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n>\n> > Currently, (checkout, reset, restore) commands correctly take '@' as a\n> > synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and\n> > 'HEAD', different prompts/messages are given by the commands mentioned\n> > above. This is due to the literal and only string comparison with the word\n> > 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously\n> > desired, especially since '@' already resolves to 'HEAD'.\n> >\n> > Therefore, make a new function user_meant_head() which takes the\n> > revision string and compares it to 'HEAD' as well as '@'. However, in\n> > builtin/checkout.c, there is some logic to convert all revs besides\n> > 'HEAD' to hex revs due to 'diff-index', which is used in patch mode\n> > machinery, not being able to handle '...' revs. Therefore, in\n> > addition to 'HEAD', do not convert '@' as well, so it can be later\n> > used in assigning patch_mode_(...)_head.\n>\n> In this context ... names a single rev (not two revs) that is\n> the merge base of and . Perhaps\nI meant revs which are spelled out in the form of ... and not the\n and themselves. But I can see how it can be confusing. I will\nchange it.\n\n> ... there is a logic to convert all command line input rev to\n> the raw object name for underlying machinery (e.g., diff-index)\n> that does not recognize the ... notation, but we'd need to\n> leave \"HEAD\" intact. Now we need to teach that \"@\" is a synonym\n> to \"HEAD\" to that code, too.\n>\n> which may be a bit shorter.\nYeah, that is much better and clearer also.\n\n> You decided to use is_rev_head() instead of user_meant_head(), so\n> you'd need to update the above description to match, I think.\nWill update.\n\n> > -\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n> > +\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n> > +\t\t strcmp(rev, \"@\"))\n>\n> Shouldn't this be\n>\n> \t\tif (rev && new_branch_info->commit && !is_rev_head(rev))\n>\n> instead of \"HEAD\" and \"@\" spelled out?\nis_rev_head() is in add-patch.c and the above is in\nbuiltin/checkout.c and is_rev_head() is not exported. I can also define\nit in builtin/checkout.c, but this would be the only instance in that\nfile which will use it. So, I think it is better to just add\nstrcmp(rev, \"@\") to the if condition.\n"},{"id":"487836","messageId":"CYUS9OD6A01I.1IQC872DESCQ0@gmail.com","threadId":"60801","inReplyTo":"xmqqv876sqwf.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-02T17:53:01Z","receivedAt":"2024-02-02T17:53:06Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Feb 2, 2024 at 11:13 PM IST, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> > You decided to use is_rev_head() instead of user_meant_head(), so\n> > you'd need to update the above description to match, I think.\n>\n> Having said this, I have a slight fear that normal users would\n> expect is_rev_head(X) to say \"yes\" for \"master\" when the current\n> branch is \"master\", though. is_head(X) would have the same\n> downside.\nYeah, user_meant_head() looks like the better pick. I'll update it.\n"},{"id":"487869","messageId":"20240203112619.979239-2-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240202150434.11256-1-shyamthakkar001@gmail.com","subject":"[PATCH v3 0/2] add-patch: '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-03T11:25:46Z","receivedAt":"2024-02-03T12:07:42Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The v3 of the series updates the commit message for patch [2/2] and\nreplaces the function name from is_rev_head() to user_meant_head().\n\nPatch [1/2] remains unchanged.\n\nGhanshyam Thakkar (2):\n add-patch: remove unnecessary NEEDSWORK comment\n add-patch: classify '@' as a synonym for 'HEAD'\n\n add-patch.c | 19 +++++++---------\n builtin/checkout.c | 11 +++++-----\n t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 10 +++++++++\n 5 files changed, 61 insertions(+), 43 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"487870","messageId":"20240203112619.979239-4-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240202150434.11256-1-shyamthakkar001@gmail.com","subject":"[PATCH v3 1/2] add-patch: remove unnecessary NEEDSWORK comment","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-03T11:25:48Z","receivedAt":"2024-02-03T12:07:55Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The comment suggested to compare commit objects instead of string\ncomparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.\n'@'). However, this approach would also count a non-checked out branch\npointing to same commit as HEAD, as HEAD. This would cause confusion to\nthe user.\n\nJunio described it best as[1]:\n\n\"Users may consider 'HEAD' and '@' the same and may want them to behave\nthe same way, but the user, when explicitly naming '$branch', means they\nwant to \"check contents out of that OTHER thing named '$branch', not the\ncurrent branch\"; it may or may not happen to be pointing at the same\ncommit as HEAD, but if the user meant to say \"check contents out of the\ncurrent commit, (partially) reverting the local changes I have\", the user\nwould have said HEAD. After all, the user may not even be immediately\naware that '$branch' happens to point at the same commit as HEAD.\"\n\n[1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/\n\nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..68f525b35c 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\t/*\n-\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n-\t\t * compare the commit objects instead so that other ways of\n-\t\t * saying the same thing (such as \"@\") are also handled\n-\t\t * appropriately.\n-\t\t *\n-\t\t * This applies to the cases below too.\n-\t\t */\n \t\tif (!revision || !strcmp(revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n-- \n2.43.0\n\n"},{"id":"487871","messageId":"20240203112619.979239-6-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240202150434.11256-1-shyamthakkar001@gmail.com","subject":"[PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-03T11:25:50Z","receivedAt":"2024-02-03T12:08:31Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Currently, (checkout, reset, restore) commands correctly take '@' as a\nsynonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and\n'HEAD', different prompts/messages are given by the commands mentioned\nabove. This is due to the literal and only string comparison with the word\n'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously\ndesired, especially since '@' already resolves to 'HEAD'.\n\nTherefore, make a new function user_meant_head() which takes the\nrevision string and compares it to 'HEAD' as well as '@'. However, in\nbuiltin/checkout.c, there is a logic to convert all command line input\nrev to the raw object name for underlying machinery (e.g., diff-index)\nthat does not recognize the ... notation, but we'd need to leave\n'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD'\n to that code and leave '@' intact, too.\n\nThere is one unintended side-effect/behavior change of this, even if\nthere exists a branch named '@', when providing '@' as a rev-source to\n(checkout, reset, restore) commands in patch mode, it will consider it\nas HEAD. This is due to the behavior of diff-index. However, naming a\nbranch '@' is an obvious foot-gun and there are many existing commands\nwhich take '@' for 'HEAD' even if 'refs/heads/@' exists (e.g., 'git log\n@', 'git push origin @' etc.). Therefore, this should be fine.\n\nAlso, add tests to check the above mentioned synonymity between 'HEAD'\nand '@'.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 11 +++++++---\n builtin/checkout.c | 11 +++++-----\n t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 10 +++++++++\n 5 files changed, 61 insertions(+), 35 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 68f525b35c..7d565dcb33 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \treturn 0;\n }\n \n+static inline int user_meant_head(const char *rev)\n+{\n+\treturn !strcmp(rev, \"HEAD\") || !strcmp(rev, \"@\");\n+}\n+\n static int is_octal(const char *p, size_t len)\n {\n \tif (!len)\n@@ -1729,21 +1734,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\tif (!revision || !strcmp(revision, \"HEAD\"))\n+\t\tif (!revision || user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_reset_nothead;\n \t} else if (mode == ADD_P_CHECKOUT) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_checkout_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_checkout_nothead;\n \t} else if (mode == ADD_P_WORKTREE) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_worktree_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_worktree_nothead;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6e30931b5..79e208ee6d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * recognized by diff-index), we will always replace the name\n \t\t * with the hex of the commit (whether it's in `...` form or\n \t\t * not) for the run_add_interactive() machinery to work\n-\t\t * properly. However, there is special logic for the HEAD case\n-\t\t * so we mustn't replace that. Also, when we were given a\n-\t\t * tree-object, new_branch_info->commit would be NULL, but we\n-\t\t * do not have to do any replacement, either.\n+\t\t * properly. However, there is special logic for the 'HEAD' and\n+\t\t * '@' case so we mustn't replace that. Also, when we were\n+\t\t * given a tree-object, new_branch_info->commit would be NULL,\n+\t\t * but we do not have to do any replacement, either.\n \t\t */\n-\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n+\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n+\t\t strcmp(rev, \"@\"))\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n \n \t\tif (opts->checkout_index && opts->checkout_worktree)\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 747eb5563e..c4f9bf09aa 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '\n-\tset_and_save_state dir/foo work head &&\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_saved_state dir/foo\n-'\n-\n-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '\n-\ttest_write_lines n y y | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n-\n-test_expect_success 'git checkout -p HEAD with change already staged' '\n-\tset_state dir/foo index index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n+\t\tset_and_save_state dir/foo work head &&\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_saved_state dir/foo &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: apply\" '\n+\t\ttest_write_lines n y y | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with change already staged\" '\n+\t\tset_state dir/foo index index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success 'git checkout -p HEAD^...' '\n \t# the third n is to get out in case it mistakenly does not apply\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex b5c5c0ff7e..3dc9184b4a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD' '\n-\tset_state dir/foo work index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git restore -p --source=HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head index\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n+\t\tset_state dir/foo work index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head index &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 05079c7246..ec7f16dfb6 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' '\n \ttest_grep \"Unstage\" output\n '\n \n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git reset -p $opt\" '\n+\t\ttest_write_lines n y | git reset -p $opt >output &&\n+\t\tverify_state dir/foo work head &&\n+\t\tverify_saved_state bar &&\n+\t\ttest_grep \"Unstage\" output\n+\t'\n+done\n+\n test_expect_success PERL 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n-- \n2.43.0\n\n"},{"id":"487881","messageId":"xmqq34u9qiu5.fsf@gitster.g","threadId":"60801","inReplyTo":"20240203112619.979239-6-shyamthakkar001@gmail.com","subject":"Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-03T22:33:06Z","receivedAt":"2024-02-03T22:33:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Therefore, make a new function user_meant_head() which takes the\n> revision string and compares it to 'HEAD' as well as '@'. However, in\n> builtin/checkout.c, there is a logic to convert all command line input\n> rev to the raw object name for underlying machinery (e.g., diff-index)\n> that does not recognize the ... notation, but we'd need to leave\n> 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD'\n> to that code and leave '@' intact, too.\n\nI am not sure what that \"However\" wants to say.\n\n - Now we have a helper function to see what the end-user said, and\n tell if the end-user meant the state that is currently checked\n out (aka \"HEAD\" but some folks like a synonym \"@\"[*]), or if the\n end-user meant some other \"concrete\" branch.\n\n - In builtin/checkout.c, there is a logic to convert unless what\n the end-user meant is the state that is currently checked out.\n\nIsn't the natural conclusion that follows these two stentences\n\"therefore, the latter is a very good place to use that helper\nfunction, too\"?\n\n\tSide note: the \"@\" is already problematic not just because\n\t\"git branch @\" would not refuse to create \"refs/heads/@\",\n\tbut there is no ref \"@\" (like $GIT_DIR/refs/@ or $GIT_DIR/@)\n\twhen it is used as a synonym for \"HEAD\". There is a check\n\tin builtin/checkout.c:update_refs_for_switch() that runs\n\tstrcmp() on a token given by the end-user from the command\n\tline with \"HEAD\" to notice the no-op case \"git checkout\n\tHEAD\" but the code does not trigger when \"@\" is given, and\n\tit happens to work by accident. I really wish we didn't add\n\tthat oddball synonym, but that is water under the bridge by\n\tnow.\n\nIn any case, I think we'd find more places that currently treats the\ntoken \"HEAD\" given directly by the end-user specially and may want\nto teach at least some of them to also accept \"@\" the same way, and\nthe helper function you are introducing may become useful in the\nfuture, at which time we may move it to a more public header. If it\nneeds to be shared already between add-patch.c and builtin/checkout.c\n(I am guessing what you meant with \"However\" as an excuse for open\ncoding it instead of sharing the code), perhaps we should do so without\nwaiting for that future, though. I dunno.\n\nIf we choose to do so, for now, a squashable patch may look like the\nattached, but we'd need to update the log message while squashing it\nin.\n\n add-interactive.h | 14 ++++++++++++++\n add-patch.c | 11 +++--------\n builtin/checkout.c | 3 +--\n 3 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git c/add-interactive.h w/add-interactive.h\nindex 693f125e8e..ca7326336d 100644\n--- c/add-interactive.h\n+++ w/add-interactive.h\n@@ -38,4 +38,18 @@ enum add_p_mode {\n int run_add_p(struct repository *r, enum add_p_mode mode,\n \t const char *revision, const struct pathspec *ps);\n \n+/*\n+ * When the user gives these tokens from the command line, they mean\n+ * the state that the currently checked out state came from. This\n+ * single bit of information affects the direction in which the patch\n+ * is presented to the end-user: are we showing a patch to go back to\n+ * the currently committed state, or are we showing a patch to move\n+ * forward to the given commit that may be different from the\n+ * committed state we started with?\n+ */\n+static inline int the_user_meant_head(const char *rev)\n+{\n+\treturn !strcmp(rev, \"HEAD\") || !strcmp(rev, \"@\");\n+}\n+\n #endif\ndiff --git c/add-patch.c w/add-patch.c\nindex 7d565dcb33..5502acebb8 100644\n--- c/add-patch.c\n+++ w/add-patch.c\n@@ -378,11 +378,6 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \treturn 0;\n }\n \n-static inline int user_meant_head(const char *rev)\n-{\n-\treturn !strcmp(rev, \"HEAD\") || !strcmp(rev, \"@\");\n-}\n-\n static int is_octal(const char *p, size_t len)\n {\n \tif (!len)\n@@ -1734,21 +1729,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\tif (!revision || user_meant_head(revision))\n+\t\tif (!revision || the_user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_reset_nothead;\n \t} else if (mode == ADD_P_CHECKOUT) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (user_meant_head(revision))\n+\t\telse if (the_user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_checkout_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_checkout_nothead;\n \t} else if (mode == ADD_P_WORKTREE) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (user_meant_head(revision))\n+\t\telse if (the_user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_worktree_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_worktree_nothead;\ndiff --git c/builtin/checkout.c w/builtin/checkout.c\nindex 79e208ee6d..63c669b157 100644\n--- c/builtin/checkout.c\n+++ w/builtin/checkout.c\n@@ -544,8 +544,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * given a tree-object, new_branch_info->commit would be NULL,\n \t\t * but we do not have to do any replacement, either.\n \t\t */\n-\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n-\t\t strcmp(rev, \"@\"))\n+\t\tif (rev && new_branch_info->commit && !the_user_meant_head(rev))\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n \n \t\tif (opts->checkout_index && opts->checkout_worktree)\n"},{"id":"487931","messageId":"CYX8RO5UFS8T.2XRXYWJ4JO8K6@gmail.com","threadId":"60801","inReplyTo":"xmqq34u9qiu5.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-05T15:14:08Z","receivedAt":"2024-02-05T15:14:14Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Sun Feb 4, 2024 at 4:03 AM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n>\n> > Therefore, make a new function user_meant_head() which takes the\n> > revision string and compares it to 'HEAD' as well as '@'. However, in\n> > builtin/checkout.c, there is a logic to convert all command line input\n> > rev to the raw object name for underlying machinery (e.g., diff-index)\n> > that does not recognize the ... notation, but we'd need to leave\n> > 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD'\n> > to that code and leave '@' intact, too.\n>\n> I am not sure what that \"However\" wants to say.\n>\n> - Now we have a helper function to see what the end-user said, and\n> tell if the end-user meant the state that is currently checked\n> out (aka \"HEAD\" but some folks like a synonym \"@\"[*]), or if the\n> end-user meant some other \"concrete\" branch.\n>\n> - In builtin/checkout.c, there is a logic to convert unless what\n> the end-user meant is the state that is currently checked out.\n>\n> Isn't the natural conclusion that follows these two stentences\n> \"therefore, the latter is a very good place to use that helper\n> function, too\"?\nYeah, I did not use the helper function in builtin/checkout.c. Hence the\n\"However\". But I agree on the point of exporting the function.\nTherefore I have attached the patch with the updated message below.\n\n> \tSide note: the \"@\" is already problematic not just because\n> \t\"git branch @\" would not refuse to create \"refs/heads/@\",\n> \tbut there is no ref \"@\" (like $GIT_DIR/refs/@ or $GIT_DIR/@)\n> \twhen it is used as a synonym for \"HEAD\". There is a check\n> \tin builtin/checkout.c:update_refs_for_switch() that runs\n> \tstrcmp() on a token given by the end-user from the command\n> \tline with \"HEAD\" to notice the no-op case \"git checkout\n> \tHEAD\" but the code does not trigger when \"@\" is given, and\n> \tit happens to work by accident. I really wish we didn't add\n> \tthat oddball synonym, but that is water under the bridge by\n> \tnow.\nwell, I suppose it is maybe annoying from the development perspective,\nbut users seem to like the concept of it[1].\n\n> In any case, I think we'd find more places that currently treats the\n> token \"HEAD\" given directly by the end-user specially and may want\n> to teach at least some of them to also accept \"@\" the same way, and\n> the helper function you are introducing may become useful in the\n> future, at which time we may move it to a more public header. If it\n> needs to be shared already between add-patch.c and builtin/checkout.c\n> (I am guessing what you meant with \"However\" as an excuse for open\n> coding it instead of sharing the code), perhaps we should do so without\n> waiting for that future, though. I dunno.\n\nYeah, that \"However\" was for not using the helper function.\n\n> If we choose to do so, for now, a squashable patch may look like the\n> attached, but we'd need to update the log message while squashing it\n> in.\nThanks for the patch. Updated message is below.\n\n[Footnote]\n[1]: https://www.reddit.com/r/git/comments/k15cqm/do_you_know_is_a_shortcut_for_head/\n\n-- >8 --\nSubject: [PATCH] add-patch: classify '@' as a synonym for 'HEAD'\n\nCurrently, (checkout, reset, restore) commands correctly take '@' as a\nsynonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and\n'HEAD', different prompts/messages are given by the commands mentioned\nabove (because of applying reverse mode(-R) in case of '@'). This is due to\nthe literal and only string comparison with the word 'HEAD' in run_add_p().\nSynonymity between '@' and 'HEAD' is obviously desired, especially since\n'@' already resolves to 'HEAD'.\n\nTherefore, make a new function the_user_meant_head() which takes the\nrevision string and compares it to 'HEAD' as well as '@'. Also in\nbuiltin/checkout.c, there is a logic to convert all command line input\nrev to the raw object name for underlying machinery (e.g., diff-index)\nthat does not recognize the ... notation, but we'd need to leave\n'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD'\nto that code and leave '@' intact, too. Therefore, this function is\nalready useful in add-patch.c and builtin/checkout.c and may also\nbecome helpful in other places in future. Therefore, it makes sense to\nexport it.\n\nThere is one unintended side-effect/behavior change of this, even if\nthere exists a branch named '@', when providing '@' as a rev-source to\n(checkout, reset, restore) commands in patch mode, it will consider it\nas HEAD. This is due to the behavior of diff-index. However, naming a\nbranch '@' is an obvious foot-gun and there are many existing commands\nwhich already take '@' for 'HEAD' regardless of whether 'refs/heads/@'\nexists or not (e.g., 'git log @', 'git push origin @' etc.). Therefore,\nthis should be fine.\n\nAlso, add tests for checking the above mentioned synonymity.\n\nCo-authored-by: Junio C Hamano \nSigned-off-by: Ghanshyam Thakkar \n---\n add-interactive.h | 14 ++++++++++++\n add-patch.c | 6 ++---\n builtin/checkout.c | 10 ++++-----\n t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 10 +++++++++\n 6 files changed, 69 insertions(+), 35 deletions(-)\n\ndiff --git a/add-interactive.h b/add-interactive.h\nindex 693f125e8e..ca7326336d 100644\n--- a/add-interactive.h\n+++ b/add-interactive.h\n@@ -38,4 +38,18 @@ enum add_p_mode {\n int run_add_p(struct repository *r, enum add_p_mode mode,\n \t const char *revision, const struct pathspec *ps);\n \n+/*\n+ * When the user gives these tokens from the command line, they mean\n+ * the state that the currently checked out state came from. This\n+ * single bit of information affects the direction in which the patch\n+ * is presented to the end-user: are we showing a patch to go back to\n+ * the currently committed state, or are we showing a patch to move\n+ * forward to the given commit that may be different from the\n+ * committed state we started with?\n+ */\n+static inline int the_user_meant_head(const char *rev)\n+{\n+\treturn !strcmp(rev, \"HEAD\") || !strcmp(rev, \"@\");\n+}\n+\n #endif\ndiff --git a/add-patch.c b/add-patch.c\nindex 68f525b35c..5502acebb8 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1729,21 +1729,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\tif (!revision || !strcmp(revision, \"HEAD\"))\n+\t\tif (!revision || the_user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_reset_nothead;\n \t} else if (mode == ADD_P_CHECKOUT) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (the_user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_checkout_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_checkout_nothead;\n \t} else if (mode == ADD_P_WORKTREE) {\n \t\tif (!revision)\n \t\t\ts.mode = &patch_mode_checkout_index;\n-\t\telse if (!strcmp(revision, \"HEAD\"))\n+\t\telse if (the_user_meant_head(revision))\n \t\t\ts.mode = &patch_mode_worktree_head;\n \t\telse\n \t\t\ts.mode = &patch_mode_worktree_nothead;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6e30931b5..63c669b157 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -539,12 +539,12 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * recognized by diff-index), we will always replace the name\n \t\t * with the hex of the commit (whether it's in `...` form or\n \t\t * not) for the run_add_interactive() machinery to work\n-\t\t * properly. However, there is special logic for the HEAD case\n-\t\t * so we mustn't replace that. Also, when we were given a\n-\t\t * tree-object, new_branch_info->commit would be NULL, but we\n-\t\t * do not have to do any replacement, either.\n+\t\t * properly. However, there is special logic for the 'HEAD' and\n+\t\t * '@' case so we mustn't replace that. Also, when we were\n+\t\t * given a tree-object, new_branch_info->commit would be NULL,\n+\t\t * but we do not have to do any replacement, either.\n \t\t */\n-\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n+\t\tif (rev && new_branch_info->commit && !the_user_meant_head(rev))\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n \n \t\tif (opts->checkout_index && opts->checkout_worktree)\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 747eb5563e..c4f9bf09aa 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '\n-\tset_and_save_state dir/foo work head &&\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_saved_state dir/foo\n-'\n-\n-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '\n-\ttest_write_lines n y y | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n-\n-test_expect_success 'git checkout -p HEAD with change already staged' '\n-\tset_state dir/foo index index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n+\t\tset_and_save_state dir/foo work head &&\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_saved_state dir/foo &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: apply\" '\n+\t\ttest_write_lines n y y | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with change already staged\" '\n+\t\tset_state dir/foo index index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success 'git checkout -p HEAD^...' '\n \t# the third n is to get out in case it mistakenly does not apply\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex b5c5c0ff7e..3dc9184b4a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD' '\n-\tset_state dir/foo work index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git restore -p --source=HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head index\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n+\t\tset_state dir/foo work index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head index &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 05079c7246..ec7f16dfb6 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' '\n \ttest_grep \"Unstage\" output\n '\n \n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git reset -p $opt\" '\n+\t\ttest_write_lines n y | git reset -p $opt >output &&\n+\t\tverify_state dir/foo work head &&\n+\t\tverify_saved_state bar &&\n+\t\ttest_grep \"Unstage\" output\n+\t'\n+done\n+\n test_expect_success PERL 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n-- \n2.43.0\n\n"},{"id":"487937","messageId":"9f9f26f1-5460-468e-a893-5caf7fbea981@gmail.com","threadId":"60801","inReplyTo":"20240203112619.979239-6-shyamthakkar001@gmail.com","subject":"Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-05T16:37:22Z","receivedAt":"2024-02-05T16:37:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nI think this is a useful addition, I've left a couple of comments below\n\nOn 03/02/2024 11:25, Ghanshyam Thakkar wrote:\n> diff --git a/add-patch.c b/add-patch.c\n> index 68f525b35c..7d565dcb33 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n> \treturn 0;\n> }\n> \n> +static inline int user_meant_head(const char *rev)\n> +{\n> +\treturn !strcmp(rev, \"HEAD\") || !strcmp(rev, \"@\");\n> +}\n> +\n\nAs well as the places you have converted we also have an explicit test \nfor \"HEAD\" in parse_diff() which looks like\n\n\tif (s->revision) {\n\t\tstruct object_id oid;\n\t\tstrvec_push(&args,\n\t\t\t /* could be on an unborn branch */\n\t\t\t !strcmp(\"HEAD\", s->revision) &&\n\t\t\t repo_get_oid(the_repository, \"HEAD\", &oid) ?\n\t\t\t empty_tree_oid_hex() : s->revision);\n\t}\n\nI suspect we need to update that code as well to accept \"@\" as a synonym \nfor \"HEAD\" on an unborn branch.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index a6e30931b5..79e208ee6d 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \t\t * recognized by diff-index), we will always replace the name\n> \t\t * with the hex of the commit (whether it's in `...` form or\n> \t\t * not) for the run_add_interactive() machinery to work\n> -\t\t * properly. However, there is special logic for the HEAD case\n> -\t\t * so we mustn't replace that. Also, when we were given a\n> -\t\t * tree-object, new_branch_info->commit would be NULL, but we\n> -\t\t * do not have to do any replacement, either.\n> +\t\t * properly. However, there is special logic for the 'HEAD' and\n> +\t\t * '@' case so we mustn't replace that. Also, when we were\n> +\t\t * given a tree-object, new_branch_info->commit would be NULL,\n> +\t\t * but we do not have to do any replacement, either.\n> \t\t */\n> -\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n> +\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n> +\t\t strcmp(rev, \"@\"))\n> \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n\nI agree with Junio's suggestion to use the user_meant_head() here. \nLooking at this made me wonder why builtin/reset.c does not need \nupdating. The answer seems to be that reset passes in the revision as \ngiven on the commandline after checking it refers to a valid tree \nwhereas for checkout the revision for on the commandline might contain \n\"...\" which run_add_p() cannot handle.\n> diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\n> index b5c5c0ff7e..3dc9184b4a 100755\n> --- a/t/t2071-restore-patch.sh\n> +++ b/t/t2071-restore-patch.sh\n> @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n\nIt is a pre-existing problem but all these \"PERL\" prerequisites are \nno-longer required as we've removed the perl implementation of \"add -p\"\n\n> \tverify_state dir/foo index index\n> '\n> \n> -test_expect_success PERL 'git restore -p --source=HEAD' '\n> -\tset_state dir/foo work index &&\n> -\t# the third n is to get out in case it mistakenly does not apply\n> -\ttest_write_lines n y n | git restore -p --source=HEAD &&\n> -\tverify_saved_state bar &&\n> -\tverify_state dir/foo head index\n> -'\n> +for opt in \"HEAD\" \"@\"\n> +do\n> +\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n> +\t\tset_state dir/foo work index &&\n> +\t\t# the third n is to get out in case it mistakenly does not apply\n> +\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n> +\t\tverify_saved_state bar &&\n> +\t\tverify_state dir/foo head index &&\n> +\t\ttest_grep \"Discard\" output\n\nIt is good to see that we're now testing for a reversed patch here.\n\nBest Wishes\n\nPhillip\n"},{"id":"487950","messageId":"CYXFNV97P2SC.1ABUMGOIR1GQ0@gmail.com","threadId":"60801","inReplyTo":"9f9f26f1-5460-468e-a893-5caf7fbea981@gmail.com","subject":"Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-05T20:38:18Z","receivedAt":"2024-02-05T20:38:24Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon Feb 5, 2024 at 10:07 PM IST, Phillip Wood wrote:\n> On 03/02/2024 11:25, Ghanshyam Thakkar wrote:\n> > diff --git a/add-patch.c b/add-patch.c\n> > index 68f525b35c..7d565dcb33 100644\n> > --- a/add-patch.c\n> > +++ b/add-patch.c\n> > @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n> > \treturn 0;\n> > }\n> > \n> > +static inline int user_meant_head(const char *rev)\n> > +{\n> > +\treturn !strcmp(rev, \"HEAD\") || !strcmp(rev, \"@\");\n> > +}\n> > +\n>\n> As well as the places you have converted we also have an explicit test \n> for \"HEAD\" in parse_diff() which looks like\n>\n> \tif (s->revision) {\n> \t\tstruct object_id oid;\n> \t\tstrvec_push(&args,\n> \t\t\t /* could be on an unborn branch */\n> \t\t\t !strcmp(\"HEAD\", s->revision) &&\n> \t\t\t repo_get_oid(the_repository, \"HEAD\", &oid) ?\n> \t\t\t empty_tree_oid_hex() : s->revision);\n> \t}\n>\n> I suspect we need to update that code as well to accept \"@\" as a synonym \n> for \"HEAD\" on an unborn branch.\nI had already considered that. Updating here will not have any effect,\nbecause on unborn branch, we do not allow naming HEAD or @. This case is\nfor when we run without naming any revision (i.e. git reset -p) on\nunborn branch. In such cases, we pass 'HEAD' as a default value.\n>\n> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n> > index a6e30931b5..79e208ee6d 100644\n> > --- a/builtin/checkout.c\n> > +++ b/builtin/checkout.c\n> > @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts,\n> > \t\t * recognized by diff-index), we will always replace the name\n> > \t\t * with the hex of the commit (whether it's in `...` form or\n> > \t\t * not) for the run_add_interactive() machinery to work\n> > -\t\t * properly. However, there is special logic for the HEAD case\n> > -\t\t * so we mustn't replace that. Also, when we were given a\n> > -\t\t * tree-object, new_branch_info->commit would be NULL, but we\n> > -\t\t * do not have to do any replacement, either.\n> > +\t\t * properly. However, there is special logic for the 'HEAD' and\n> > +\t\t * '@' case so we mustn't replace that. Also, when we were\n> > +\t\t * given a tree-object, new_branch_info->commit would be NULL,\n> > +\t\t * but we do not have to do any replacement, either.\n> > \t\t */\n> > -\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n> > +\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n> > +\t\t strcmp(rev, \"@\"))\n> > \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n>\n> I agree with Junio's suggestion to use the user_meant_head() here. \n> Looking at this made me wonder why builtin/reset.c does not need \n> updating. The answer seems to be that reset passes in the revision as \n> given on the commandline after checking it refers to a valid tree \n> whereas for checkout the revision for on the commandline might contain \n> \"...\" which run_add_p() cannot handle.\nI was not able to run reset with '...'. I ran,\n'git reset main...$ANOTHERBRANCH'\nbut it gave me \"fatal: ambiguous argument 'main...$ANOTHERBRANCH'\"\nerror, with or without -p. While,\n'git restore --source=main...$ANOTHERBRANCH .' and \n'git checkout main...$ANOTHERBRANCH' works fine, with or without -p.\n\n> > diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\n> > index b5c5c0ff7e..3dc9184b4a 100755\n> > --- a/t/t2071-restore-patch.sh\n> > +++ b/t/t2071-restore-patch.sh\n> > @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n>\n> It is a pre-existing problem but all these \"PERL\" prerequisites are \n> no-longer required as we've removed the perl implementation of \"add -p\"\nI can send a separate patch to clean up this script, removing PERL\npre-req from all tests.\n\n> > \tverify_state dir/foo index index\n> > '\n> > \n> > -test_expect_success PERL 'git restore -p --source=HEAD' '\n> > -\tset_state dir/foo work index &&\n> > -\t# the third n is to get out in case it mistakenly does not apply\n> > -\ttest_write_lines n y n | git restore -p --source=HEAD &&\n> > -\tverify_saved_state bar &&\n> > -\tverify_state dir/foo head index\n> > -'\n> > +for opt in \"HEAD\" \"@\"\n> > +do\n> > +\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n> > +\t\tset_state dir/foo work index &&\n> > +\t\t# the third n is to get out in case it mistakenly does not apply\n> > +\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n> > +\t\tverify_saved_state bar &&\n> > +\t\tverify_state dir/foo head index &&\n> > +\t\ttest_grep \"Discard\" output\n>\n> It is good to see that we're now testing for a reversed patch here.\n>\n> Best Wishes\n>\n> Phillip\n\nThanks for the review.\n\n"},{"id":"487967","messageId":"xmqq34u6o6hs.fsf@gitster.g","threadId":"60801","inReplyTo":"9f9f26f1-5460-468e-a893-5caf7fbea981@gmail.com","subject":"Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-05T23:07:11Z","receivedAt":"2024-02-05T23:07:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> As well as the places you have converted we also have an explicit test\n> for \"HEAD\" in parse_diff() which looks like\n>\n> \tif (s->revision) {\n> \t\tstruct object_id oid;\n> \t\tstrvec_push(&args,\n> \t\t\t /* could be on an unborn branch */\n> \t\t\t !strcmp(\"HEAD\", s->revision) &&\n> \t\t\t repo_get_oid(the_repository, \"HEAD\", &oid) ?\n> \t\t\t empty_tree_oid_hex() : s->revision);\n> \t}\n>\n> I suspect we need to update that code as well to accept \"@\" as a\n> synonym for \"HEAD\" on an unborn branch.\n\nI actually think \"@\" in the input should be normalized to \"HEAD\" as\nsoon as possible. After the if/elseif/ cascade in run_add_p() the\npatch in this thread touches, there is an assignment\n\n\ts.revision = revision;\n\nand even though we rewrite !strcmp(revision, \"HEAD\") to \"user means\nHEAD\" to additionally accept \"@\" in that if/elseif/ cascade, here we\nwill stuff different values to s.revision here. We could normalize\nthe end-user supplied \"@\" to \"HEAD\" before making this assignment,\nthen you do not have to worry about the code in parse_diff() above,\nand more importantly, we do not have to rely on what the current set\nof callers happen to do and do not happen to do (e.g., \"git reset\n-p\" happens to use hardcoded \"HEAD\" for unborn case without using\nanything obtained from the user, so the above code in parse_diff()\nmight be safe in that case, but we do not want to rely on such\nsubtlety to make sure our code is correct)\n\nCome to think of it, we could even do the \"normalizing\" even before,\nand that might greatly simplify things. For example, if we did so\nat the very beginning of run_add_p(), before we come to that if/else\nif/ cascade, we may not even have to worry about the \"user meant HEAD\"\nhelper. After the normalization, we can just keep using !strcmp()\nwith \"HEAD\" alone. Simple and clean, no?\n\nThanks.\n\n\n"},{"id":"488108","messageId":"20240206225122.1095766-3-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240203112619.979239-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 0/3] '@' as a synonym for 'HEAD' in patch mode","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-06T22:50:35Z","receivedAt":"2024-02-06T22:57:04Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Since v3, I have taken Junio's suggestion to replace '@' with 'HEAD'\nat the beginning of run_add_p(). Due to this, I no longer think we\nneed to have/export the_user_meant_head() function just for the single\ninstance in builtin/checkout.c. Therefore, I have hardcoded the '@'\ncondition in builtin/checkout.c.\n\nAlso, Phillip mentioned that the PERL prerequisites in the test files\nthat are touched by this series are unnecessary and therefore I have\nattached a patch to remove them.\n\nGhanshyam Thakkar (3):\n add-patch: remove unnecessary NEEDSWORK comment\n add-patch: classify '@' as a synonym for 'HEAD'\n add -p tests: remove Perl prerequisite\n\n add-patch.c | 12 ++++------\n builtin/checkout.c | 11 +++++-----\n t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------\n t/t2071-restore-patch.sh | 46 +++++++++++++++++++++------------------\n t/t7105-reset-patch.sh | 32 +++++++++++++++++----------\n 5 files changed, 82 insertions(+), 65 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"488109","messageId":"20240206225122.1095766-4-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240203112619.979239-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 1/3] add-patch: remove unnecessary NEEDSWORK comment","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-06T22:50:36Z","receivedAt":"2024-02-06T22:57:13Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The comment suggested to compare commit objects instead of string\ncomparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.\n'@'). However, this approach would also count a non-checked out branch\npointing to same commit as HEAD, as HEAD. This would cause confusion to\nthe user.\n\nJunio described it best as[1]:\n\n\"Users may consider 'HEAD' and '@' the same and\nmay want them to behave the same way, but the user, when explicitly\nnaming '$branch', means they want to \"check contents out of that\nOTHER thing named '$branch', not the current branch\"; it may or\nmay not happen to be pointing at the same commit as HEAD, but if\nthe user meant to say \"check contents out of the current commit,\n(partially) reverting the local changes I have\", the user would have\nsaid HEAD. After all, the user may not even be immediately aware\nthat '$branch' happens to point at the same commit as HEAD.\"\n\n[1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/\n\nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..68f525b35c 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\t/*\n-\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n-\t\t * compare the commit objects instead so that other ways of\n-\t\t * saying the same thing (such as \"@\") are also handled\n-\t\t * appropriately.\n-\t\t *\n-\t\t * This applies to the cases below too.\n-\t\t */\n \t\tif (!revision || !strcmp(revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\n-- \n2.43.0\n\n"},{"id":"488111","messageId":"20240206225122.1095766-6-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240203112619.979239-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 2/3] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-06T22:50:38Z","receivedAt":"2024-02-06T22:58:49Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Currently, (checkout, reset, restore) commands correctly take '@' as a\nsynonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@'\nand 'HEAD', different prompts/messages are given by the commands\nmentioned above (because of applying reverse mode(-R) in case of '@').\nThis is due to the literal and only string comparison with the word\n'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously\ndesired, especially since '@' already resolves to 'HEAD'.\n\nTherefore, replace '@' to 'HEAD' at the beginning of\nadd-patch.c:run_add_p(). There is also logic in builtin/checkout.c to\nconvert all command line input rev to the raw object name for underlying\nmachinery (e.g., diff-index) that does not recognize the ...\nnotation, but we'd need to leave 'HEAD' intact. Now we need to teach\nthat '@' is a synonym to 'HEAD' to that code and leave '@' intact, too.\n\nThere is one unintended side-effect/behavior change of this, even if\nthere exists a branch named '@', when providing '@' as a rev-source to\n(checkout, reset, restore) commands in patch mode, it will consider it\nas HEAD. This is due to the behavior of diff-index. However, naming a\nbranch '@' is an obvious foot-gun and there are many existing commands\nwhich already take '@' for 'HEAD' regardless of whether 'refs/heads/@'\nexists or not (e.g., 'git log @', 'git push origin @' etc.). Therefore,\nthis should be fine.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 4 ++++\n builtin/checkout.c | 11 +++++-----\n t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++-----------------\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 10 +++++++++\n 5 files changed, 57 insertions(+), 32 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 68f525b35c..6f4ca8f4e4 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1726,6 +1726,10 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \n \tinit_add_i_state(&s.s, r);\n \n+\t/* helpful in deciding the patch mode ahead */\n+\tif(revision && !strcmp(revision, \"@\"))\n+\t\trevision = \"HEAD\";\n+\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6e30931b5..79e208ee6d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * recognized by diff-index), we will always replace the name\n \t\t * with the hex of the commit (whether it's in `...` form or\n \t\t * not) for the run_add_interactive() machinery to work\n-\t\t * properly. However, there is special logic for the HEAD case\n-\t\t * so we mustn't replace that. Also, when we were given a\n-\t\t * tree-object, new_branch_info->commit would be NULL, but we\n-\t\t * do not have to do any replacement, either.\n+\t\t * properly. However, there is special logic for the 'HEAD' and\n+\t\t * '@' case so we mustn't replace that. Also, when we were\n+\t\t * given a tree-object, new_branch_info->commit would be NULL,\n+\t\t * but we do not have to do any replacement, either.\n \t\t */\n-\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n+\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\") &&\n+\t\t strcmp(rev, \"@\"))\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n \n \t\tif (opts->checkout_index && opts->checkout_worktree)\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 747eb5563e..c4f9bf09aa 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '\n-\tset_and_save_state dir/foo work head &&\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_saved_state dir/foo\n-'\n-\n-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '\n-\ttest_write_lines n y y | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n-\n-test_expect_success 'git checkout -p HEAD with change already staged' '\n-\tset_state dir/foo index index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n+\t\tset_and_save_state dir/foo work head &&\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_saved_state dir/foo &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: apply\" '\n+\t\ttest_write_lines n y y | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with change already staged\" '\n+\t\tset_state dir/foo index index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success 'git checkout -p HEAD^...' '\n \t# the third n is to get out in case it mistakenly does not apply\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex b5c5c0ff7e..dbbefc188d 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD' '\n-\tset_state dir/foo work index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git restore -p --source=HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head index\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git restore -p --source=$opt\" '\n+\t\tset_state dir/foo work index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head index &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 05079c7246..7147148138 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' '\n \ttest_grep \"Unstage\" output\n '\n \n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git reset -p $opt\" '\n+\t\ttest_write_lines n y | git reset -p $opt >output &&\n+\t\tverify_state dir/foo work head &&\n+\t\tverify_saved_state bar &&\n+\t\ttest_grep \"Unstage\" output\n+\t'\n+done\n+\n test_expect_success PERL 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n-- \n2.43.0\n\n"},{"id":"488112","messageId":"20240206225122.1095766-7-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240203112619.979239-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 3/3] add -p tests: remove Perl prerequisite","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-06T22:50:39Z","receivedAt":"2024-02-06T22:58:55Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The Perl version of the add -i/-p commands has been removed since\n20b813d (add: remove \"add.interactive.useBuiltin\" & Perl \"git\nadd--interactive\", 2023-02-07)\n\nTherefore, Perl prerequisite in t2071-restore-patch and\nt7105-reset-patch is not necessary.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t2071-restore-patch.sh | 26 +++++++++++++-------------\n t/t7105-reset-patch.sh | 22 +++++++++++-----------\n 2 files changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex dbbefc188d..27e85be40a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -4,7 +4,7 @@ test_description='git restore --patch'\n \n . ./lib-patch-mode.sh\n \n-test_expect_success PERL 'setup' '\n+test_expect_success 'setup' '\n \tmkdir dir &&\n \techo parent >dir/foo &&\n \techo dummy >bar &&\n@@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '\n \tsave_head\n '\n \n-test_expect_success PERL 'restore -p without pathspec is fine' '\n+test_expect_success 'restore -p without pathspec is fine' '\n \techo q >cmd &&\n \tgit restore -p dir/foo &&\n \techo dummy > bar &&\n@@ -19,14 +19,14 @@ test_expect_success PERL 'setup' '\n \n # note: bar sorts before foo, so the first 'n' is always to skip 'bar'\n \n-test_expect_success PERL 'saying \"n\" does nothing' '\n+test_expect_success 'saying \"n\" does nothing' '\n \tset_and_save_state dir/foo work work &&\n \ttest_write_lines n n | git reset -p &&\n \tverify_saved_state dir/foo &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p' '\n+test_expect_success 'git reset -p' '\n \ttest_write_lines n y | git reset -p >output &&\n \tverify_state dir/foo work head &&\n \tverify_saved_state bar &&\n@@ -43,28 +43,28 @@ do\n \t'\n done\n \n-test_expect_success PERL 'git reset -p HEAD^' '\n+test_expect_success 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar &&\n \ttest_grep \"Apply\" output\n '\n \n-test_expect_success PERL 'git reset -p HEAD^^{tree}' '\n+test_expect_success 'git reset -p HEAD^^{tree}' '\n \ttest_write_lines n y | git reset -p HEAD^^{tree} >output &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar &&\n \ttest_grep \"Apply\" output\n '\n \n-test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '\n+test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '\n \tset_and_save_state dir/foo work work &&\n \ttest_must_fail git reset -p HEAD^:dir/foo &&\n \tverify_saved_state dir/foo &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n+test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '\n \tset_and_save_state dir/foo work work &&\n \ttest_must_fail git reset -p aaaaaaaa &&\n \tverify_saved_state dir/foo &&\n@@ -76,27 +76,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n # dir/foo. There's always an extra 'n' to reject edits to dir/foo in\n # the failure case (and thus get out of the loop).\n \n-test_expect_success PERL 'git reset -p dir' '\n+test_expect_success 'git reset -p dir' '\n \tset_state dir/foo work work &&\n \ttest_write_lines y n | git reset -p dir &&\n \tverify_state dir/foo work head &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p -- foo (inside dir)' '\n+test_expect_success 'git reset -p -- foo (inside dir)' '\n \tset_state dir/foo work work &&\n \ttest_write_lines y n | (cd dir && git reset -p -- foo) &&\n \tverify_state dir/foo work head &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p HEAD^ -- dir' '\n+test_expect_success 'git reset -p HEAD^ -- dir' '\n \ttest_write_lines y n | git reset -p HEAD^ -- dir &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'none of this moved HEAD' '\n+test_expect_success 'none of this moved HEAD' '\n \tverify_saved_head\n '\n \n-- \n2.43.0\n\n"},{"id":"488114","messageId":"xmqqil31dqx6.fsf@gitster.g","threadId":"60801","inReplyTo":"20240206225122.1095766-6-shyamthakkar001@gmail.com","subject":"Re: [PATCH v4 2/3] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T01:05:57Z","receivedAt":"2024-02-07T01:06:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Currently, (checkout, reset, restore) commands correctly take '@' as a\n> synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@'\n> and 'HEAD', different prompts/messages are given by the commands\n> mentioned above (because of applying reverse mode(-R) in case of '@').\n> This is due to the literal and only string comparison with the word\n> 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously\n> desired, especially since '@' already resolves to 'HEAD'.\n>\n> Therefore, replace '@' to 'HEAD' at the beginning of\n> add-patch.c:run_add_p().\n\nOf course there is only one possible downside for this approach, in\nthat if we are using \"revision\" in an error message, users who asked\nfor \"@\" may complain when an error message says \"HEAD\" in it. I think\nthe simplicity of the implementation far outweighs this downside.\n\n> There is also logic in builtin/checkout.c to\n> convert all command line input rev to the raw object name for underlying\n> machinery (e.g., diff-index) that does not recognize the ...\n> notation, but we'd need to leave 'HEAD' intact. Now we need to teach\n> that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too.\n\nMakes me wonder why we cannot use the same \"normalize @ to HEAD\nupfront\" approach here, though?\n\nIt would involve translating \"@\" given to new_branch_info->name to\n\"HEAD\" early, possibly in setup_new_branch_info_and_source_tree(),\nand that probably will fix the other strcmp() with \"HEAD\" that\nappears in builtin/checkout.c:update_refs_for_switch() as well, no?\n\n> +\t/* helpful in deciding the patch mode ahead */\n> +\tif(revision && !strcmp(revision, \"@\"))\n> +\t\trevision = \"HEAD\";\n\nStyle. \"if (revision ...)\"\n"},{"id":"488130","messageId":"1aabcaec-7266-40f8-b7ce-e82b8df58891@gmail.com","threadId":"60801","inReplyTo":"xmqqil31dqx6.fsf@gitster.g","subject":"Re: [PATCH v4 2/3] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-07T10:38:41Z","receivedAt":"2024-02-07T10:38:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/02/2024 01:05, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n> \n>> Currently, (checkout, reset, restore) commands correctly take '@' as a\n>> synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@'\n>> and 'HEAD', different prompts/messages are given by the commands\n>> mentioned above (because of applying reverse mode(-R) in case of '@').\n>> This is due to the literal and only string comparison with the word\n>> 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously\n>> desired, especially since '@' already resolves to 'HEAD'.\n>>\n>> Therefore, replace '@' to 'HEAD' at the beginning of\n>> add-patch.c:run_add_p().\n> \n> Of course there is only one possible downside for this approach, in\n> that if we are using \"revision\" in an error message, users who asked\n> for \"@\" may complain when an error message says \"HEAD\" in it. I think\n> the simplicity of the implementation far outweighs this downside.\n\nI agree, if we were replacing the revision the user gave us with a hex \nobject id that would be confusing but as \"@\" is just a shortcut for \n\"HEAD\" I think replacing it in the error message is fine. It was a good \nidea just to replace \"@\" with \"HEAD\", this version is much simpler.\n\nBest Wishes\n\nPhillip\n\n>> There is also logic in builtin/checkout.c to\n>> convert all command line input rev to the raw object name for underlying\n>> machinery (e.g., diff-index) that does not recognize the ...\n>> notation, but we'd need to leave 'HEAD' intact. Now we need to teach\n>> that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too.\n> \n> Makes me wonder why we cannot use the same \"normalize @ to HEAD\n> upfront\" approach here, though?\n> \n> It would involve translating \"@\" given to new_branch_info->name to\n> \"HEAD\" early, possibly in setup_new_branch_info_and_source_tree(),\n> and that probably will fix the other strcmp() with \"HEAD\" that\n> appears in builtin/checkout.c:update_refs_for_switch() as well, no?\n> \n>> +\t/* helpful in deciding the patch mode ahead */\n>> +\tif(revision && !strcmp(revision, \"@\"))\n>> +\t\trevision = \"HEAD\";\n> \n> Style. \"if (revision ...)\"\n> \n\n"},{"id":"488131","messageId":"8baa44ef-4960-4f0d-8cab-38d3d6ff971a@gmail.com","threadId":"60801","inReplyTo":"20240206225122.1095766-7-shyamthakkar001@gmail.com","subject":"Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-07T10:50:07Z","receivedAt":"2024-02-07T10:50:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOn 06/02/2024 22:50, Ghanshyam Thakkar wrote:\n > The Perl version of the add -i/-p commands has been removed since\n > 20b813d (add: remove \"add.interactive.useBuiltin\" & Perl \"git\n > add--interactive\", 2023-02-07)\n >\n > Therefore, Perl prerequisite in t2071-restore-patch and\n > t7105-reset-patch is not necessary.\n\nThanks for adding this patch. If you do re-roll I've just noticed that \none of the tests in t7106-reset-unborn-branch.sh and another in \nt2024-checkout-dwim.sh still have PERL prerequisites as well. I don't \nthink it is worth re-rolling just for that as we can clean them up \nseparately if needed.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> t/t2071-restore-patch.sh | 26 +++++++++++++-------------\n> t/t7105-reset-patch.sh | 22 +++++++++++-----------\n> 2 files changed, 24 insertions(+), 24 deletions(-)\n> \n> diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\n> index dbbefc188d..27e85be40a 100755\n> --- a/t/t2071-restore-patch.sh\n> +++ b/t/t2071-restore-patch.sh\n> @@ -4,7 +4,7 @@ test_description='git restore --patch'\n> \n> . ./lib-patch-mode.sh\n> \n> -test_expect_success PERL 'setup' '\n> +test_expect_success 'setup' '\n> \tmkdir dir &&\n> \techo parent >dir/foo &&\n> \techo dummy >bar &&\n> @@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '\n> \tsave_head\n> '\n> \n> -test_expect_success PERL 'restore -p without pathspec is fine' '\n> +test_expect_success 'restore -p without pathspec is fine' '\n> \techo q >cmd &&\n> \tgit restore -p '\n> \n> # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'\n> \n> -test_expect_success PERL 'saying \"n\" does nothing' '\n> +test_expect_success 'saying \"n\" does nothing' '\n> \tset_and_save_state dir/foo work head &&\n> \ttest_write_lines n n | git restore -p &&\n> \tverify_saved_state bar &&\n> \tverify_saved_state dir/foo\n> '\n> \n> -test_expect_success PERL 'git restore -p' '\n> +test_expect_success 'git restore -p' '\n> \tset_and_save_state dir/foo work head &&\n> \ttest_write_lines n y | git restore -p &&\n> \tverify_saved_state bar &&\n> \tverify_state dir/foo head head\n> '\n> \n> -test_expect_success PERL 'git restore -p with staged changes' '\n> +test_expect_success 'git restore -p with staged changes' '\n> \tset_state dir/foo work index &&\n> \ttest_write_lines n y | git restore -p &&\n> \tverify_saved_state bar &&\n> @@ -56,7 +56,7 @@ do\n> \t'\n> done\n> \n> -test_expect_success PERL 'git restore -p --source=HEAD^' '\n> +test_expect_success 'git restore -p --source=HEAD^' '\n> \tset_state dir/foo work index &&\n> \t# the third n is to get out in case it mistakenly does not apply\n> \ttest_write_lines n y n | git restore -p --source=HEAD^ &&\n> @@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '\n> \tverify_state dir/foo parent index\n> '\n> \n> -test_expect_success PERL 'git restore -p --source=HEAD^...' '\n> +test_expect_success 'git restore -p --source=HEAD^...' '\n> \tset_state dir/foo work index &&\n> \t# the third n is to get out in case it mistakenly does not apply\n> \ttest_write_lines n y n | git restore -p --source=HEAD^... &&\n> @@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '\n> \tverify_state dir/foo parent index\n> '\n> \n> -test_expect_success PERL 'git restore -p handles deletion' '\n> +test_expect_success 'git restore -p handles deletion' '\n> \tset_state dir/foo work index &&\n> \trm dir/foo &&\n> \ttest_write_lines n y | git restore -p &&\n> @@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '\n> # dir/foo. There's always an extra 'n' to reject edits to dir/foo in\n> # the failure case (and thus get out of the loop).\n> \n> -test_expect_success PERL 'path limiting works: dir' '\n> +test_expect_success 'path limiting works: dir' '\n> \tset_state dir/foo work head &&\n> \ttest_write_lines y n | git restore -p dir &&\n> \tverify_saved_state bar &&\n> \tverify_state dir/foo head head\n> '\n> \n> -test_expect_success PERL 'path limiting works: -- dir' '\n> +test_expect_success 'path limiting works: -- dir' '\n> \tset_state dir/foo work head &&\n> \ttest_write_lines y n | git restore -p -- dir &&\n> \tverify_saved_state bar &&\n> \tverify_state dir/foo head head\n> '\n> \n> -test_expect_success PERL 'path limiting works: HEAD^ -- dir' '\n> +test_expect_success 'path limiting works: HEAD^ -- dir' '\n> \tset_state dir/foo work head &&\n> \t# the third n is to get out in case it mistakenly does not apply\n> \ttest_write_lines y n n | git restore -p --source=HEAD^ -- dir &&\n> @@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '\n> \tverify_state dir/foo parent head\n> '\n> \n> -test_expect_success PERL 'path limiting works: foo inside dir' '\n> +test_expect_success 'path limiting works: foo inside dir' '\n> \tset_state dir/foo work head &&\n> \t# the third n is to get out in case it mistakenly does not apply\n> \ttest_write_lines y n n | (cd dir && git restore -p foo) &&\n> @@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '\n> \tverify_state dir/foo head head\n> '\n> \n> -test_expect_success PERL 'none of this moved HEAD' '\n> +test_expect_success 'none of this moved HEAD' '\n> \tverify_saved_head\n> '\n> \n> diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\n> index 7147148138..3691b94d1b 100755\n> --- a/t/t7105-reset-patch.sh\n> +++ b/t/t7105-reset-patch.sh\n> @@ -5,7 +5,7 @@ test_description='git reset --patch'\n> TEST_PASSES_SANITIZE_LEAK=true\n> . ./lib-patch-mode.sh\n> \n> -test_expect_success PERL 'setup' '\n> +test_expect_success 'setup' '\n> \tmkdir dir &&\n> \techo parent > dir/foo &&\n> \techo dummy > bar &&\n> @@ -19,14 +19,14 @@ test_expect_success PERL 'setup' '\n> \n> # note: bar sorts before foo, so the first 'n' is always to skip 'bar'\n> \n> -test_expect_success PERL 'saying \"n\" does nothing' '\n> +test_expect_success 'saying \"n\" does nothing' '\n> \tset_and_save_state dir/foo work work &&\n> \ttest_write_lines n n | git reset -p &&\n> \tverify_saved_state dir/foo &&\n> \tverify_saved_state bar\n> '\n> \n> -test_expect_success PERL 'git reset -p' '\n> +test_expect_success 'git reset -p' '\n> \ttest_write_lines n y | git reset -p >output &&\n> \tverify_state dir/foo work head &&\n> \tverify_saved_state bar &&\n> @@ -43,28 +43,28 @@ do\n> \t'\n> done\n> \n> -test_expect_success PERL 'git reset -p HEAD^' '\n> +test_expect_success 'git reset -p HEAD^' '\n> \ttest_write_lines n y | git reset -p HEAD^ >output &&\n> \tverify_state dir/foo work parent &&\n> \tverify_saved_state bar &&\n> \ttest_grep \"Apply\" output\n> '\n> \n> -test_expect_success PERL 'git reset -p HEAD^^{tree}' '\n> +test_expect_success 'git reset -p HEAD^^{tree}' '\n> \ttest_write_lines n y | git reset -p HEAD^^{tree} >output &&\n> \tverify_state dir/foo work parent &&\n> \tverify_saved_state bar &&\n> \ttest_grep \"Apply\" output\n> '\n> \n> -test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '\n> +test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '\n> \tset_and_save_state dir/foo work work &&\n> \ttest_must_fail git reset -p HEAD^:dir/foo &&\n> \tverify_saved_state dir/foo &&\n> \tverify_saved_state bar\n> '\n> \n> -test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n> +test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '\n> \tset_and_save_state dir/foo work work &&\n> \ttest_must_fail git reset -p aaaaaaaa &&\n> \tverify_saved_state dir/foo &&\n> @@ -76,27 +76,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n> # dir/foo. There's always an extra 'n' to reject edits to dir/foo in\n> # the failure case (and thus get out of the loop).\n> \n> -test_expect_success PERL 'git reset -p dir' '\n> +test_expect_success 'git reset -p dir' '\n> \tset_state dir/foo work work &&\n> \ttest_write_lines y n | git reset -p dir &&\n> \tverify_state dir/foo work head &&\n> \tverify_saved_state bar\n> '\n> \n> -test_expect_success PERL 'git reset -p -- foo (inside dir)' '\n> +test_expect_success 'git reset -p -- foo (inside dir)' '\n> \tset_state dir/foo work work &&\n> \ttest_write_lines y n | (cd dir && git reset -p -- foo) &&\n> \tverify_state dir/foo work head &&\n> \tverify_saved_state bar\n> '\n> \n> -test_expect_success PERL 'git reset -p HEAD^ -- dir' '\n> +test_expect_success 'git reset -p HEAD^ -- dir' '\n> \ttest_write_lines y n | git reset -p HEAD^ -- dir &&\n> \tverify_state dir/foo work parent &&\n> \tverify_saved_state bar\n> '\n> \n> -test_expect_success PERL 'none of this moved HEAD' '\n> +test_expect_success 'none of this moved HEAD' '\n> \tverify_saved_head\n> '\n> \n\n"},{"id":"488132","messageId":"99fcba4a-a4f7-4d72-8238-09d9bf7851ce@gmail.com","threadId":"60801","inReplyTo":"20240206225122.1095766-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH v4 1/3] add-patch: remove unnecessary NEEDSWORK comment","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-07T10:51:28Z","receivedAt":"2024-02-07T10:51:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOn 06/02/2024 22:50, Ghanshyam Thakkar wrote:\n> The comment suggested to compare commit objects instead of string\n> comparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.\n> '@'). However, this approach would also count a non-checked out branch\n> pointing to same commit as HEAD, as HEAD. This would cause confusion to\n> the user.\n\nI forgot to say before, but I think this could be squashed into the next \npatch so we remove the comment at the same time as fixing the issue it \ndescribes.\n\nBest Wishes\n\nPhillip\n\n> Junio described it best as[1]:\n> \n> \"Users may consider 'HEAD' and '@' the same and\n> may want them to behave the same way, but the user, when explicitly\n> naming '$branch', means they want to \"check contents out of that\n> OTHER thing named '$branch', not the current branch\"; it may or\n> may not happen to be pointing at the same commit as HEAD, but if\n> the user meant to say \"check contents out of the current commit,\n> (partially) reverting the local changes I have\", the user would have\n> said HEAD. After all, the user may not even be immediately aware\n> that '$branch' happens to point at the same commit as HEAD.\"\n> \n> [1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/\n> \n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> add-patch.c | 8 --------\n> 1 file changed, 8 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 79eda168eb..68f525b35c 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> \tif (mode == ADD_P_STASH)\n> \t\ts.mode = &patch_mode_stash;\n> \telse if (mode == ADD_P_RESET) {\n> -\t\t/*\n> -\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n> -\t\t * compare the commit objects instead so that other ways of\n> -\t\t * saying the same thing (such as \"@\") are also handled\n> -\t\t * appropriately.\n> -\t\t *\n> -\t\t * This applies to the cases below too.\n> -\t\t */\n> \t\tif (!revision || !strcmp(revision, \"HEAD\"))\n> \t\t\ts.mode = &patch_mode_reset_head;\n> \t\telse\n\n"},{"id":"488134","messageId":"df1fc65f-8716-47bb-b379-1e1f1eeece8b@gmail.com","threadId":"60801","inReplyTo":"8baa44ef-4960-4f0d-8cab-38d3d6ff971a@gmail.com","subject":"Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-07T13:51:39Z","receivedAt":"2024-02-07T13:51:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/02/2024 10:50, Phillip Wood wrote:\n> Hi Ghanshyam\n> \n> On 06/02/2024 22:50, Ghanshyam Thakkar wrote:\n> > The Perl version of the add -i/-p commands has been removed since\n> > 20b813d (add: remove \"add.interactive.useBuiltin\" & Perl \"git\n> > add--interactive\", 2023-02-07)\n> >\n> > Therefore, Perl prerequisite in t2071-restore-patch and\n> > t7105-reset-patch is not necessary.\n> \n> Thanks for adding this patch. If you do re-roll I've just noticed that \n> one of the tests in t7106-reset-unborn-branch.sh and another in \n> t2024-checkout-dwim.sh still have PERL prerequisites as well. I don't \n> think it is worth re-rolling just for that as we can clean them up \n> separately if needed.\n\nI didn't cast my net wide enough when I was grepping earlier, \nt7514-commit-patch.sh and t3904-stash-patch.sh also have unnecessary \nPERL prerequisites\n\nBest Wishes\n\nPhillip\n\n"},{"id":"488140","messageId":"xmqqsf24clfa.fsf@gitster.g","threadId":"60801","inReplyTo":"df1fc65f-8716-47bb-b379-1e1f1eeece8b@gmail.com","subject":"Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T16:02:17Z","receivedAt":"2024-02-07T16:02:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 07/02/2024 10:50, Phillip Wood wrote:\n>> Hi Ghanshyam\n>> On 06/02/2024 22:50, Ghanshyam Thakkar wrote:\n>> > The Perl version of the add -i/-p commands has been removed since\n>> > 20b813d (add: remove \"add.interactive.useBuiltin\" & Perl \"git\n>> > add--interactive\", 2023-02-07)\n>> >\n>> > Therefore, Perl prerequisite in t2071-restore-patch and\n>> > t7105-reset-patch is not necessary.\n>> Thanks for adding this patch. If you do re-roll I've just noticed\n>> that one of the tests in t7106-reset-unborn-branch.sh and another in\n>> t2024-checkout-dwim.sh still have PERL prerequisites as well. I\n>> don't think it is worth re-rolling just for that as we can clean\n>> them up separately if needed.\n>\n> I didn't cast my net wide enough when I was grepping earlier,\n> t7514-commit-patch.sh and t3904-stash-patch.sh also have unnecessary\n> PERL prerequisites\n\nThanks for helping usher this topic forward. Very much appreciated.\n"},{"id":"488159","messageId":"CAPig+cT4gY_Smhvxs9AHTZVDWaLTtCABNaTDFsR3ukJx_pYJGg@mail.gmail.com","threadId":"60801","inReplyTo":"df1fc65f-8716-47bb-b379-1e1f1eeece8b@gmail.com","subject":"Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-07T16:58:40Z","receivedAt":"2024-02-07T16:58:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 7, 2024 at 8:51 AM Phillip Wood wrote:\n> On 07/02/2024 10:50, Phillip Wood wrote:\n> > On 06/02/2024 22:50, Ghanshyam Thakkar wrote:\n> > > The Perl version of the add -i/-p commands has been removed since\n> > > 20b813d (add: remove \"add.interactive.useBuiltin\" & Perl \"git\n> > > add--interactive\", 2023-02-07)\n> > >\n> > > Therefore, Perl prerequisite in t2071-restore-patch and\n> > > t7105-reset-patch is not necessary.\n> >\n> > Thanks for adding this patch. If you do re-roll I've just noticed that\n> > one of the tests in t7106-reset-unborn-branch.sh and another in\n> > t2024-checkout-dwim.sh still have PERL prerequisites as well. I don't\n> > think it is worth re-rolling just for that as we can clean them up\n> > separately if needed.\n>\n> I didn't cast my net wide enough when I was grepping earlier,\n> t7514-commit-patch.sh and t3904-stash-patch.sh also have unnecessary\n> PERL prerequisites\n\nAdditionally, patch [2/3] drops a PERL prerequisite when it moves an\nexisting test into a loop, but the removal of the prerequisite is not\nmentioned in the commit message. Presumably, the relocation-into-loop\nand prerequisite-removal should have been done separately (in patches\n[2/3] and [3/3], respectively), and that's how I'd suggest doing it.\n"},{"id":"488293","messageId":"CZ0O76ZJ2BPR.1PSK0WMUHWT8H@gmail.com","threadId":"60801","inReplyTo":"xmqqil31dqx6.fsf@gitster.g","subject":"Re: [PATCH v4 2/3] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-09T15:57:41Z","receivedAt":"2024-02-09T15:57:47Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Wed Feb 7, 2024 at 6:35 AM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n> > There is also logic in builtin/checkout.c to\n> > convert all command line input rev to the raw object name for underlying\n> > machinery (e.g., diff-index) that does not recognize the ...\n> > notation, but we'd need to leave 'HEAD' intact. Now we need to teach\n> > that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too.\n>\n> Makes me wonder why we cannot use the same \"normalize @ to HEAD\n> upfront\" approach here, though?\n>\n> It would involve translating \"@\" given to new_branch_info->name to\n> \"HEAD\" early, possibly in setup_new_branch_info_and_source_tree(),\n> and that probably will fix the other strcmp() with \"HEAD\" that\n> appears in builtin/checkout.c:update_refs_for_switch() as well, no?\n\nI was wondering about \"git checkout -b @\". If we were to make the change\nin setup_new_branch_info_and_source_tree(), then '@' would not be\ntreated as 'HEAD' in the above mentioned case and there would also be\nsome disparity on error messages showing '@' when running the above\ncommand, and after hitting setup_new_branch_info_and_source_tree() would\nshow 'HEAD'. However, making the change in builtin/checkout.c:checkout_main()\nwould disallow creating a '@' branch alltogether. Hence, I would like some\nfeedback on whether we should change the behavior of 'git checkout -b @'\nor not.\n"},{"id":"488419","messageId":"20240211202035.7196-2-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240206225122.1095766-3-shyamthakkar001@gmail.com","subject":"[PATCH v5 0/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-11T20:20:09Z","receivedAt":"2024-02-11T20:21:15Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Since v4, I have taken Junio's suggestion to change '@' to 'HEAD' as\nearly as reasonably possible. And also added new test to check the\nno-op case of 'checkout HEAD/@'. \n\nAnd the second patch removes all the perl prerequisites from the tests\nwhich use patch mode functionality, as pointed out by Phillip. And have\nalso taken into account Eric's suggestion to not remove perl\nprerequisites while amending them in patch (1/2) and do it all\ntogether in patch (2/2).\n\nGhanshyam Thakkar (2):\n add-patch: classify '@' as a synonym for 'HEAD'\n add -p tests: remove PERL prerequisites\n\n add-patch.c | 8 ------\n builtin/checkout.c | 4 ++-\n builtin/reset.c | 4 ++-\n t/t2016-checkout-patch.sh | 46 +++++++++++++++++++---------------\n t/t2020-checkout-detach.sh | 12 +++++++++\n t/t2024-checkout-dwim.sh | 2 +-\n t/t2071-restore-patch.sh | 46 ++++++++++++++++++----------------\n t/t3904-stash-patch.sh | 6 -----\n t/t7105-reset-patch.sh | 37 ++++++++++++++-------------\n t/t7106-reset-unborn-branch.sh | 2 +-\n t/t7514-commit-patch.sh | 6 -----\n 11 files changed, 91 insertions(+), 82 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"488420","messageId":"20240211202035.7196-4-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240206225122.1095766-3-shyamthakkar001@gmail.com","subject":"[PATCH v5 1/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-11T20:20:11Z","receivedAt":"2024-02-11T20:23:00Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Currently, (restore, checkout, reset) commands correctly take '@' as a\nsynonym for 'HEAD'. However, in patch mode different prompts/messages\nare given on command line due to patch mode machinery not considering\n'@' to be a synonym for 'HEAD' due to literal string comparison with\nthe word 'HEAD', and therefore assigning patch_mode_($command)_nothead\nand triggering reverse mode (-R in diff-index). The NEEDSWORK comment\nsuggested comparing commit objects to get around this. However, doing\nso would also take a non-checked out branch pointing to the same commit\nas HEAD, as HEAD. This would cause confusion to the user.\n\nTherefore, after parsing '@', replace it with 'HEAD' as reasonably\nearly as possible. This also solves another problem of disparity\nbetween 'git checkout HEAD' and 'git checkout @' (latter detaches at\nthe HEAD commit and the former does not).\n\nTrade-offs:\n- Some of the errors would show the revision argument as 'HEAD' when\n given '@'. This should be fine, as most users who probably use '@'\n would be aware that it is a shortcut for 'HEAD' and most probably\n used to use 'HEAD'. There is also relevant documentation in\n 'gitrevisions' manpage about '@' being the shortcut for 'HEAD'. Also,\n the simplicity of the solution far outweighs this cost.\n\n- Consider '@' as a shortcut for 'HEAD' even if 'refs/heads/@' exists\n at a different commit. Naming a branch '@' is an obvious foot-gun and\n many existing commands already take '@' for 'HEAD' even if\n 'refs/heads/@' exists at a different commit or does not exist at all\n (e.g. 'git log @', 'git push origin @' etc.). Therefore this is an\n existing assumption and should not be a problem.\n\nHelped-by: Junio C Hamano \nHelped-by: Phillip Wood \nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 8 -------\n builtin/checkout.c | 4 +++-\n builtin/reset.c | 4 +++-\n t/t2016-checkout-patch.sh | 46 +++++++++++++++++++++-----------------\n t/t2020-checkout-detach.sh | 12 ++++++++++\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 15 ++++++++-----\n 7 files changed, 64 insertions(+), 43 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..68f525b35c 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\t/*\n-\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n-\t\t * compare the commit objects instead so that other ways of\n-\t\t * saying the same thing (such as \"@\") are also handled\n-\t\t * appropriately.\n-\t\t *\n-\t\t * This applies to the cases below too.\n-\t\t */\n \t\tif (!revision || !strcmp(revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6e30931b5..067c251933 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(\n \tstruct tree **source_tree = &opts->source_tree;\n \tstruct object_id branch_rev;\n \n-\tnew_branch_info->name = xstrdup(arg);\n+\t/* treat '@' as a shortcut for 'HEAD' */\n+\tnew_branch_info->name = !strcmp(arg, \"@\") ? xstrdup(\"HEAD\") :\n+\t\t\t\t\t\t xstrdup(arg);\n \tsetup_branch_path(new_branch_info);\n \n \tif (!check_refname_format(new_branch_info->path, 0) &&\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 8390bfe4c4..f0bf29a478 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,\n \t\t\tverify_filename(prefix, argv[0], 1);\n \t\t}\n \t}\n-\t*rev_ret = rev;\n+\n+\t/* treat '@' as a shortcut for 'HEAD' */\n+\t*rev_ret = !strcmp(\"@\", rev) ? \"HEAD\" : rev;\n \n \tparse_pathspec(pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 747eb5563e..c4f9bf09aa 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '\n-\tset_and_save_state dir/foo work head &&\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_saved_state dir/foo\n-'\n-\n-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '\n-\ttest_write_lines n y y | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n-\n-test_expect_success 'git checkout -p HEAD with change already staged' '\n-\tset_state dir/foo index index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n+\t\tset_and_save_state dir/foo work head &&\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_saved_state dir/foo &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: apply\" '\n+\t\ttest_write_lines n y y | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with change already staged\" '\n+\t\tset_state dir/foo index index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success 'git checkout -p HEAD^...' '\n \t# the third n is to get out in case it mistakenly does not apply\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 8202ef8c74..bce284c297 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -45,6 +45,18 @@ test_expect_success 'checkout branch does not detach' '\n \tcheck_not_detached\n '\n \n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"checkout $opt no-op/don't detach\" '\n+\t\treset &&\n+\t\tcat .git/HEAD >expect &&\n+\t\tgit checkout $opt &&\n+\t\tcat .git/HEAD >actual &&\n+\t\tcheck_not_detached &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_expect_success 'checkout tag detaches' '\n \treset &&\n \tgit checkout tag &&\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex b5c5c0ff7e..3dc9184b4a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD' '\n-\tset_state dir/foo work index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git restore -p --source=HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head index\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n+\t\tset_state dir/foo work index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head index &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 05079c7246..0f597416d8 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -26,12 +26,15 @@ test_expect_success PERL 'saying \"n\" does nothing' '\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p' '\n-\ttest_write_lines n y | git reset -p >output &&\n-\tverify_state dir/foo work head &&\n-\tverify_saved_state bar &&\n-\ttest_grep \"Unstage\" output\n-'\n+for opt in \"HEAD\" \"@\" \"\"\n+do\n+\ttest_expect_success PERL \"git reset -p $opt\" '\n+\t\ttest_write_lines n y | git reset -p $opt >output &&\n+\t\tverify_state dir/foo work head &&\n+\t\tverify_saved_state bar &&\n+\t\ttest_grep \"Unstage\" output\n+\t'\n+done\n \n test_expect_success PERL 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n-- \n2.43.0\n\n"},{"id":"488421","messageId":"20240211202035.7196-5-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240206225122.1095766-3-shyamthakkar001@gmail.com","subject":"[PATCH v5 2/2] add -p tests: remove PERL prerequisites","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-11T20:20:12Z","receivedAt":"2024-02-11T20:23:04Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The Perl version of the add -i/-p commands has been removed since\n20b813d (add: remove \"add.interactive.useBuiltin\" & Perl \"git\nadd--interactive\", 2023-02-07)\n\nTherefore, Perl prerequisite in the test scripts which use the patch\nmode functionality is not neccessary.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t2024-checkout-dwim.sh | 2 +-\n t/t2071-restore-patch.sh | 28 ++++++++++++++--------------\n t/t3904-stash-patch.sh | 6 ------\n t/t7105-reset-patch.sh | 22 +++++++++++-----------\n t/t7106-reset-unborn-branch.sh | 2 +-\n t/t7514-commit-patch.sh | 6 ------\n 6 files changed, 27 insertions(+), 39 deletions(-)\n\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex a97416ce65..a3b1449ef1 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -113,7 +113,7 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'\n \ttest_grep ! \"^hint: \" stderr\n '\n \n-test_expect_success PERL 'checkout -p with multiple remotes does not print advice' '\n+test_expect_success 'checkout -p with multiple remotes does not print advice' '\n \tgit checkout -B main &&\n \ttest_might_fail git branch -D foo &&\n \ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex 3dc9184b4a..27e85be40a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -4,7 +4,7 @@ test_description='git restore --patch'\n \n . ./lib-patch-mode.sh\n \n-test_expect_success PERL 'setup' '\n+test_expect_success 'setup' '\n \tmkdir dir &&\n \techo parent >dir/foo &&\n \techo dummy >bar &&\n@@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '\n \tsave_head\n '\n \n-test_expect_success PERL 'restore -p without pathspec is fine' '\n+test_expect_success 'restore -p without pathspec is fine' '\n \techo q >cmd &&\n \tgit restore -p output &&\n@@ -56,7 +56,7 @@ do\n \t'\n done\n \n-test_expect_success PERL 'git restore -p --source=HEAD^' '\n+test_expect_success 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines n y n | git restore -p --source=HEAD^ &&\n@@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tverify_state dir/foo parent index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD^...' '\n+test_expect_success 'git restore -p --source=HEAD^...' '\n \tset_state dir/foo work index &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines n y n | git restore -p --source=HEAD^... &&\n@@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '\n \tverify_state dir/foo parent index\n '\n \n-test_expect_success PERL 'git restore -p handles deletion' '\n+test_expect_success 'git restore -p handles deletion' '\n \tset_state dir/foo work index &&\n \trm dir/foo &&\n \ttest_write_lines n y | git restore -p &&\n@@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '\n # dir/foo. There's always an extra 'n' to reject edits to dir/foo in\n # the failure case (and thus get out of the loop).\n \n-test_expect_success PERL 'path limiting works: dir' '\n+test_expect_success 'path limiting works: dir' '\n \tset_state dir/foo work head &&\n \ttest_write_lines y n | git restore -p dir &&\n \tverify_saved_state bar &&\n \tverify_state dir/foo head head\n '\n \n-test_expect_success PERL 'path limiting works: -- dir' '\n+test_expect_success 'path limiting works: -- dir' '\n \tset_state dir/foo work head &&\n \ttest_write_lines y n | git restore -p -- dir &&\n \tverify_saved_state bar &&\n \tverify_state dir/foo head head\n '\n \n-test_expect_success PERL 'path limiting works: HEAD^ -- dir' '\n+test_expect_success 'path limiting works: HEAD^ -- dir' '\n \tset_state dir/foo work head &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines y n n | git restore -p --source=HEAD^ -- dir &&\n@@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '\n \tverify_state dir/foo parent head\n '\n \n-test_expect_success PERL 'path limiting works: foo inside dir' '\n+test_expect_success 'path limiting works: foo inside dir' '\n \tset_state dir/foo work head &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines y n n | (cd dir && git restore -p foo) &&\n@@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '\n \tverify_state dir/foo head head\n '\n \n-test_expect_success PERL 'none of this moved HEAD' '\n+test_expect_success 'none of this moved HEAD' '\n \tverify_saved_head\n '\n \ndiff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\nindex accfe3845c..368fc2a6cc 100755\n--- a/t/t3904-stash-patch.sh\n+++ b/t/t3904-stash-patch.sh\n@@ -3,12 +3,6 @@\n test_description='stash -p'\n . ./lib-patch-mode.sh\n \n-if ! test_have_prereq PERL\n-then\n-\tskip_all='skipping stash -p tests, perl not available'\n-\ttest_done\n-fi\n-\n test_expect_success 'setup' '\n \tmkdir dir &&\n \techo parent > dir/foo &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 0f597416d8..4e7ad2b0f3 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -5,7 +5,7 @@ test_description='git reset --patch'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-patch-mode.sh\n \n-test_expect_success PERL 'setup' '\n+test_expect_success 'setup' '\n \tmkdir dir &&\n \techo parent > dir/foo &&\n \techo dummy > bar &&\n@@ -19,7 +19,7 @@ test_expect_success PERL 'setup' '\n \n # note: bar sorts before foo, so the first 'n' is always to skip 'bar'\n \n-test_expect_success PERL 'saying \"n\" does nothing' '\n+test_expect_success 'saying \"n\" does nothing' '\n \tset_and_save_state dir/foo work work &&\n \ttest_write_lines n n | git reset -p &&\n \tverify_saved_state dir/foo &&\n@@ -28,7 +28,7 @@ test_expect_success PERL 'saying \"n\" does nothing' '\n \n for opt in \"HEAD\" \"@\" \"\"\n do\n-\ttest_expect_success PERL \"git reset -p $opt\" '\n+\ttest_expect_success \"git reset -p $opt\" '\n \t\ttest_write_lines n y | git reset -p $opt >output &&\n \t\tverify_state dir/foo work head &&\n \t\tverify_saved_state bar &&\n@@ -36,28 +36,28 @@ do\n \t'\n done\n \n-test_expect_success PERL 'git reset -p HEAD^' '\n+test_expect_success 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar &&\n \ttest_grep \"Apply\" output\n '\n \n-test_expect_success PERL 'git reset -p HEAD^^{tree}' '\n+test_expect_success 'git reset -p HEAD^^{tree}' '\n \ttest_write_lines n y | git reset -p HEAD^^{tree} >output &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar &&\n \ttest_grep \"Apply\" output\n '\n \n-test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '\n+test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '\n \tset_and_save_state dir/foo work work &&\n \ttest_must_fail git reset -p HEAD^:dir/foo &&\n \tverify_saved_state dir/foo &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n+test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '\n \tset_and_save_state dir/foo work work &&\n \ttest_must_fail git reset -p aaaaaaaa &&\n \tverify_saved_state dir/foo &&\n@@ -69,27 +69,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n # dir/foo. There's always an extra 'n' to reject edits to dir/foo in\n # the failure case (and thus get out of the loop).\n \n-test_expect_success PERL 'git reset -p dir' '\n+test_expect_success 'git reset -p dir' '\n \tset_state dir/foo work work &&\n \ttest_write_lines y n | git reset -p dir &&\n \tverify_state dir/foo work head &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p -- foo (inside dir)' '\n+test_expect_success 'git reset -p -- foo (inside dir)' '\n \tset_state dir/foo work work &&\n \ttest_write_lines y n | (cd dir && git reset -p -- foo) &&\n \tverify_state dir/foo work head &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p HEAD^ -- dir' '\n+test_expect_success 'git reset -p HEAD^ -- dir' '\n \ttest_write_lines y n | git reset -p HEAD^ -- dir &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'none of this moved HEAD' '\n+test_expect_success 'none of this moved HEAD' '\n \tverify_saved_head\n '\n \ndiff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh\nindex d20e5709f9..88d1c8adf4 100755\n--- a/t/t7106-reset-unborn-branch.sh\n+++ b/t/t7106-reset-unborn-branch.sh\n@@ -34,7 +34,7 @@ test_expect_success 'reset $file' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success PERL 'reset -p' '\n+test_expect_success 'reset -p' '\n \trm .git/index &&\n \tgit add a &&\n \techo y >yes &&\ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex 998a2103c7..b4de10a5dd 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -3,12 +3,6 @@\n test_description='hunk edit with \"commit -p -m\"'\n . ./test-lib.sh\n \n-if ! test_have_prereq PERL\n-then\n-\tskip_all=\"skipping '$test_description' tests, perl not available\"\n-\ttest_done\n-fi\n-\n test_expect_success 'setup (initial)' '\n \techo line1 >file &&\n \tgit add file &&\n-- \n2.43.0\n\n"},{"id":"488475","messageId":"xmqqwmr94aru.fsf@gitster.g","threadId":"60801","inReplyTo":"20240211202035.7196-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-12T21:45:25Z","receivedAt":"2024-02-12T21:45:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> add-patch.c | 8 -------\n> builtin/checkout.c | 4 +++-\n> builtin/reset.c | 4 +++-\n> t/t2016-checkout-patch.sh | 46 +++++++++++++++++++++-----------------\n> t/t2020-checkout-detach.sh | 12 ++++++++++\n> t/t2071-restore-patch.sh | 18 +++++++++------\n> t/t7105-reset-patch.sh | 15 ++++++++-----\n> 7 files changed, 64 insertions(+), 43 deletions(-)\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index 79eda168eb..68f525b35c 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> \tif (mode == ADD_P_STASH)\n> \t\ts.mode = &patch_mode_stash;\n> \telse if (mode == ADD_P_RESET) {\n> -\t\t/*\n> -\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n> -\t\t * compare the commit objects instead so that other ways of\n> -\t\t * saying the same thing (such as \"@\") are also handled\n> -\t\t * appropriately.\n> -\t\t *\n> -\t\t * This applies to the cases below too.\n> -\t\t */\n> \t\tif (!revision || !strcmp(revision, \"HEAD\"))\n> \t\t\ts.mode = &patch_mode_reset_head;\n> \t\telse\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index a6e30931b5..067c251933 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(\n> \tstruct tree **source_tree = &opts->source_tree;\n> \tstruct object_id branch_rev;\n> \n> -\tnew_branch_info->name = xstrdup(arg);\n> +\t/* treat '@' as a shortcut for 'HEAD' */\n> +\tnew_branch_info->name = !strcmp(arg, \"@\") ? xstrdup(\"HEAD\") :\n> +\t\t\t\t\t\t xstrdup(arg);\n> \tsetup_branch_path(new_branch_info);\n> \n> \tif (!check_refname_format(new_branch_info->path, 0) &&\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 8390bfe4c4..f0bf29a478 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,\n> \t\t\tverify_filename(prefix, argv[0], 1);\n> \t\t}\n> \t}\n> -\t*rev_ret = rev;\n> +\n> +\t/* treat '@' as a shortcut for 'HEAD' */\n> +\t*rev_ret = !strcmp(\"@\", rev) ? \"HEAD\" : rev;\n> \n> \tparse_pathspec(pathspec, 0,\n> \t\t PATHSPEC_PREFER_FULL |\n\nNice. Things have become so much simpler, without having to touch\nmillions of strcmp() with \"HEAD\" everywhere in the code paths.\n\nWill queue. Thanks.\n"},{"id":"488494","messageId":"20240213000601.520731-2-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240211202035.7196-2-shyamthakkar001@gmail.com","subject":"[PATCH v6 0/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-13T00:05:27Z","receivedAt":"2024-02-13T00:08:48Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"I just noticed after sending v5 that, in one of the tests, while\nmoving it inside the loop for also testing '@', set_and_save_state was\nnot used therefore the subsequent tests after the first $opt will not have\nthe changes for which we prove 'y' thereby making the tests useless.\n(Although it would not fail regardless of this). This got unnoticed by\nthe previous reviews since v1 and me as well.\n\nApologies for the oversight and noise.\n\nGhanshyam Thakkar (2):\n add-patch: classify '@' as a synonym for 'HEAD'\n add -p tests: remove PERL prerequisites\n\n add-patch.c | 8 ------\n builtin/checkout.c | 4 ++-\n builtin/reset.c | 4 ++-\n t/t2016-checkout-patch.sh | 46 +++++++++++++++++++---------------\n t/t2020-checkout-detach.sh | 12 +++++++++\n t/t2024-checkout-dwim.sh | 2 +-\n t/t2071-restore-patch.sh | 46 ++++++++++++++++++----------------\n t/t3904-stash-patch.sh | 6 -----\n t/t7105-reset-patch.sh | 38 +++++++++++++++-------------\n t/t7106-reset-unborn-branch.sh | 2 +-\n t/t7514-commit-patch.sh | 6 -----\n 11 files changed, 92 insertions(+), 82 deletions(-)\n\nchange since v5:\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 0f597416d8..453872c8ba 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -29,6 +29,7 @@ test_expect_success PERL 'saying \"n\" does nothing' '\n for opt in \"HEAD\" \"@\" \"\"\n do\n test_expect_success PERL \"git reset -p $opt\" '\n+ set_and_save_state dir/foo work work &&\n test_write_lines n y | git reset -p $opt >output &&\n verify_state dir/foo work head &&\n verify_saved_state bar &&\n\n-- \n2.43.0\n\n"},{"id":"488495","messageId":"20240213000601.520731-4-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240211202035.7196-2-shyamthakkar001@gmail.com","subject":"[PATCH v6 1/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-13T00:05:29Z","receivedAt":"2024-02-13T00:09:20Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Currently, (restore, checkout, reset) commands correctly take '@' as a\nsynonym for 'HEAD'. However, in patch mode different prompts/messages\nare given on command line due to patch mode machinery not considering\n'@' to be a synonym for 'HEAD' due to literal string comparison with\nthe word 'HEAD', and therefore assigning patch_mode_($command)_nothead\nand triggering reverse mode (-R in diff-index). The NEEDSWORK comment\nsuggested comparing commit objects to get around this. However, doing\nso would also take a non-checked out branch pointing to the same commit\nas HEAD, as HEAD. This would cause confusion to the user.\n\nTherefore, after parsing '@', replace it with 'HEAD' as reasonably\nearly as possible. This also solves another problem of disparity\nbetween 'git checkout HEAD' and 'git checkout @' (latter detaches at\nthe HEAD commit and the former does not).\n\nTrade-offs:\n- Some of the errors would show the revision argument as 'HEAD' when\n given '@'. This should be fine, as most users who probably use '@'\n would be aware that it is a shortcut for 'HEAD' and most probably\n used to use 'HEAD'. There is also relevant documentation in\n 'gitrevisions' manpage about '@' being the shortcut for 'HEAD'. Also,\n the simplicity of the solution far outweighs this cost.\n\n- Consider '@' as a shortcut for 'HEAD' even if 'refs/heads/@' exists\n at a different commit. Naming a branch '@' is an obvious foot-gun and\n many existing commands already take '@' for 'HEAD' even if\n 'refs/heads/@' exists at a different commit or does not exist at all\n (e.g. 'git log @', 'git push origin @' etc.). Therefore this is an\n existing assumption and should not be a problem.\n\nHelped-by: Junio C Hamano \nHelped-by: Phillip Wood \nSigned-off-by: Ghanshyam Thakkar \n---\n add-patch.c | 8 -------\n builtin/checkout.c | 4 +++-\n builtin/reset.c | 4 +++-\n t/t2016-checkout-patch.sh | 46 +++++++++++++++++++++-----------------\n t/t2020-checkout-detach.sh | 12 ++++++++++\n t/t2071-restore-patch.sh | 18 +++++++++------\n t/t7105-reset-patch.sh | 16 ++++++++-----\n 7 files changed, 65 insertions(+), 43 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..68f525b35c 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \tif (mode == ADD_P_STASH)\n \t\ts.mode = &patch_mode_stash;\n \telse if (mode == ADD_P_RESET) {\n-\t\t/*\n-\t\t * NEEDSWORK: Instead of comparing to the literal \"HEAD\",\n-\t\t * compare the commit objects instead so that other ways of\n-\t\t * saying the same thing (such as \"@\") are also handled\n-\t\t * appropriately.\n-\t\t *\n-\t\t * This applies to the cases below too.\n-\t\t */\n \t\tif (!revision || !strcmp(revision, \"HEAD\"))\n \t\t\ts.mode = &patch_mode_reset_head;\n \t\telse\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a6e30931b5..067c251933 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(\n \tstruct tree **source_tree = &opts->source_tree;\n \tstruct object_id branch_rev;\n \n-\tnew_branch_info->name = xstrdup(arg);\n+\t/* treat '@' as a shortcut for 'HEAD' */\n+\tnew_branch_info->name = !strcmp(arg, \"@\") ? xstrdup(\"HEAD\") :\n+\t\t\t\t\t\t xstrdup(arg);\n \tsetup_branch_path(new_branch_info);\n \n \tif (!check_refname_format(new_branch_info->path, 0) &&\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 8390bfe4c4..f0bf29a478 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,\n \t\t\tverify_filename(prefix, argv[0], 1);\n \t\t}\n \t}\n-\t*rev_ret = rev;\n+\n+\t/* treat '@' as a shortcut for 'HEAD' */\n+\t*rev_ret = !strcmp(\"@\", rev) ? \"HEAD\" : rev;\n \n \tparse_pathspec(pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex 747eb5563e..c4f9bf09aa 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '\n-\tset_and_save_state dir/foo work head &&\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_saved_state dir/foo\n-'\n-\n-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '\n-\ttest_write_lines n y y | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n-\n-test_expect_success 'git checkout -p HEAD with change already staged' '\n-\tset_state dir/foo index index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git checkout -p HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head head\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: abort\" '\n+\t\tset_and_save_state dir/foo work head &&\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_saved_state dir/foo &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with NO staged changes: apply\" '\n+\t\ttest_write_lines n y y | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+\n+\ttest_expect_success \"git checkout -p $opt with change already staged\" '\n+\t\tset_state dir/foo index index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git checkout -p $opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head head &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success 'git checkout -p HEAD^...' '\n \t# the third n is to get out in case it mistakenly does not apply\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 8202ef8c74..bce284c297 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -45,6 +45,18 @@ test_expect_success 'checkout branch does not detach' '\n \tcheck_not_detached\n '\n \n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success \"checkout $opt no-op/don't detach\" '\n+\t\treset &&\n+\t\tcat .git/HEAD >expect &&\n+\t\tgit checkout $opt &&\n+\t\tcat .git/HEAD >actual &&\n+\t\tcheck_not_detached &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_expect_success 'checkout tag detaches' '\n \treset &&\n \tgit checkout tag &&\ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex b5c5c0ff7e..3dc9184b4a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' '\n \tverify_state dir/foo index index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD' '\n-\tset_state dir/foo work index &&\n-\t# the third n is to get out in case it mistakenly does not apply\n-\ttest_write_lines n y n | git restore -p --source=HEAD &&\n-\tverify_saved_state bar &&\n-\tverify_state dir/foo head index\n-'\n+for opt in \"HEAD\" \"@\"\n+do\n+\ttest_expect_success PERL \"git restore -p --source=$opt\" '\n+\t\tset_state dir/foo work index &&\n+\t\t# the third n is to get out in case it mistakenly does not apply\n+\t\ttest_write_lines n y n | git restore -p --source=$opt >output &&\n+\t\tverify_saved_state bar &&\n+\t\tverify_state dir/foo head index &&\n+\t\ttest_grep \"Discard\" output\n+\t'\n+done\n \n test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 05079c7246..453872c8ba 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -26,12 +26,16 @@ test_expect_success PERL 'saying \"n\" does nothing' '\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p' '\n-\ttest_write_lines n y | git reset -p >output &&\n-\tverify_state dir/foo work head &&\n-\tverify_saved_state bar &&\n-\ttest_grep \"Unstage\" output\n-'\n+for opt in \"HEAD\" \"@\" \"\"\n+do\n+\ttest_expect_success PERL \"git reset -p $opt\" '\n+\t\tset_and_save_state dir/foo work work &&\n+\t\ttest_write_lines n y | git reset -p $opt >output &&\n+\t\tverify_state dir/foo work head &&\n+\t\tverify_saved_state bar &&\n+\t\ttest_grep \"Unstage\" output\n+\t'\n+done\n \n test_expect_success PERL 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n-- \n2.43.0\n\n"},{"id":"488496","messageId":"20240213000601.520731-5-shyamthakkar001@gmail.com","threadId":"60801","inReplyTo":"20240211202035.7196-2-shyamthakkar001@gmail.com","subject":"[PATCH v6 2/2] add -p tests: remove PERL prerequisites","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-02-13T00:05:30Z","receivedAt":"2024-02-13T00:09:25Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The Perl version of the add -i/-p commands has been removed since\n20b813d (add: remove \"add.interactive.useBuiltin\" & Perl \"git\nadd--interactive\", 2023-02-07)\n\nTherefore, Perl prerequisite in the test scripts which use the patch\nmode functionality is not neccessary.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t2024-checkout-dwim.sh | 2 +-\n t/t2071-restore-patch.sh | 28 ++++++++++++++--------------\n t/t3904-stash-patch.sh | 6 ------\n t/t7105-reset-patch.sh | 22 +++++++++++-----------\n t/t7106-reset-unborn-branch.sh | 2 +-\n t/t7514-commit-patch.sh | 6 ------\n 6 files changed, 27 insertions(+), 39 deletions(-)\n\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex a97416ce65..a3b1449ef1 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -113,7 +113,7 @@ test_expect_success 'checkout of branch from multiple remotes fails with advice'\n \ttest_grep ! \"^hint: \" stderr\n '\n \n-test_expect_success PERL 'checkout -p with multiple remotes does not print advice' '\n+test_expect_success 'checkout -p with multiple remotes does not print advice' '\n \tgit checkout -B main &&\n \ttest_might_fail git branch -D foo &&\n \ndiff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh\nindex 3dc9184b4a..27e85be40a 100755\n--- a/t/t2071-restore-patch.sh\n+++ b/t/t2071-restore-patch.sh\n@@ -4,7 +4,7 @@ test_description='git restore --patch'\n \n . ./lib-patch-mode.sh\n \n-test_expect_success PERL 'setup' '\n+test_expect_success 'setup' '\n \tmkdir dir &&\n \techo parent >dir/foo &&\n \techo dummy >bar &&\n@@ -16,28 +16,28 @@ test_expect_success PERL 'setup' '\n \tsave_head\n '\n \n-test_expect_success PERL 'restore -p without pathspec is fine' '\n+test_expect_success 'restore -p without pathspec is fine' '\n \techo q >cmd &&\n \tgit restore -p output &&\n@@ -56,7 +56,7 @@ do\n \t'\n done\n \n-test_expect_success PERL 'git restore -p --source=HEAD^' '\n+test_expect_success 'git restore -p --source=HEAD^' '\n \tset_state dir/foo work index &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines n y n | git restore -p --source=HEAD^ &&\n@@ -64,7 +64,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '\n \tverify_state dir/foo parent index\n '\n \n-test_expect_success PERL 'git restore -p --source=HEAD^...' '\n+test_expect_success 'git restore -p --source=HEAD^...' '\n \tset_state dir/foo work index &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines n y n | git restore -p --source=HEAD^... &&\n@@ -72,7 +72,7 @@ test_expect_success PERL 'git restore -p --source=HEAD^...' '\n \tverify_state dir/foo parent index\n '\n \n-test_expect_success PERL 'git restore -p handles deletion' '\n+test_expect_success 'git restore -p handles deletion' '\n \tset_state dir/foo work index &&\n \trm dir/foo &&\n \ttest_write_lines n y | git restore -p &&\n@@ -85,21 +85,21 @@ test_expect_success PERL 'git restore -p handles deletion' '\n # dir/foo. There's always an extra 'n' to reject edits to dir/foo in\n # the failure case (and thus get out of the loop).\n \n-test_expect_success PERL 'path limiting works: dir' '\n+test_expect_success 'path limiting works: dir' '\n \tset_state dir/foo work head &&\n \ttest_write_lines y n | git restore -p dir &&\n \tverify_saved_state bar &&\n \tverify_state dir/foo head head\n '\n \n-test_expect_success PERL 'path limiting works: -- dir' '\n+test_expect_success 'path limiting works: -- dir' '\n \tset_state dir/foo work head &&\n \ttest_write_lines y n | git restore -p -- dir &&\n \tverify_saved_state bar &&\n \tverify_state dir/foo head head\n '\n \n-test_expect_success PERL 'path limiting works: HEAD^ -- dir' '\n+test_expect_success 'path limiting works: HEAD^ -- dir' '\n \tset_state dir/foo work head &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines y n n | git restore -p --source=HEAD^ -- dir &&\n@@ -107,7 +107,7 @@ test_expect_success PERL 'path limiting works: HEAD^ -- dir' '\n \tverify_state dir/foo parent head\n '\n \n-test_expect_success PERL 'path limiting works: foo inside dir' '\n+test_expect_success 'path limiting works: foo inside dir' '\n \tset_state dir/foo work head &&\n \t# the third n is to get out in case it mistakenly does not apply\n \ttest_write_lines y n n | (cd dir && git restore -p foo) &&\n@@ -115,7 +115,7 @@ test_expect_success PERL 'path limiting works: foo inside dir' '\n \tverify_state dir/foo head head\n '\n \n-test_expect_success PERL 'none of this moved HEAD' '\n+test_expect_success 'none of this moved HEAD' '\n \tverify_saved_head\n '\n \ndiff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\nindex accfe3845c..368fc2a6cc 100755\n--- a/t/t3904-stash-patch.sh\n+++ b/t/t3904-stash-patch.sh\n@@ -3,12 +3,6 @@\n test_description='stash -p'\n . ./lib-patch-mode.sh\n \n-if ! test_have_prereq PERL\n-then\n-\tskip_all='skipping stash -p tests, perl not available'\n-\ttest_done\n-fi\n-\n test_expect_success 'setup' '\n \tmkdir dir &&\n \techo parent > dir/foo &&\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 453872c8ba..f4f3b7a677 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -5,7 +5,7 @@ test_description='git reset --patch'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-patch-mode.sh\n \n-test_expect_success PERL 'setup' '\n+test_expect_success 'setup' '\n \tmkdir dir &&\n \techo parent > dir/foo &&\n \techo dummy > bar &&\n@@ -19,7 +19,7 @@ test_expect_success PERL 'setup' '\n \n # note: bar sorts before foo, so the first 'n' is always to skip 'bar'\n \n-test_expect_success PERL 'saying \"n\" does nothing' '\n+test_expect_success 'saying \"n\" does nothing' '\n \tset_and_save_state dir/foo work work &&\n \ttest_write_lines n n | git reset -p &&\n \tverify_saved_state dir/foo &&\n@@ -28,7 +28,7 @@ test_expect_success PERL 'saying \"n\" does nothing' '\n \n for opt in \"HEAD\" \"@\" \"\"\n do\n-\ttest_expect_success PERL \"git reset -p $opt\" '\n+\ttest_expect_success \"git reset -p $opt\" '\n \t\tset_and_save_state dir/foo work work &&\n \t\ttest_write_lines n y | git reset -p $opt >output &&\n \t\tverify_state dir/foo work head &&\n@@ -37,28 +37,28 @@ do\n \t'\n done\n \n-test_expect_success PERL 'git reset -p HEAD^' '\n+test_expect_success 'git reset -p HEAD^' '\n \ttest_write_lines n y | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar &&\n \ttest_grep \"Apply\" output\n '\n \n-test_expect_success PERL 'git reset -p HEAD^^{tree}' '\n+test_expect_success 'git reset -p HEAD^^{tree}' '\n \ttest_write_lines n y | git reset -p HEAD^^{tree} >output &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar &&\n \ttest_grep \"Apply\" output\n '\n \n-test_expect_success PERL 'git reset -p HEAD^:dir/foo (blob fails)' '\n+test_expect_success 'git reset -p HEAD^:dir/foo (blob fails)' '\n \tset_and_save_state dir/foo work work &&\n \ttest_must_fail git reset -p HEAD^:dir/foo &&\n \tverify_saved_state dir/foo &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n+test_expect_success 'git reset -p aaaaaaaa (unknown fails)' '\n \tset_and_save_state dir/foo work work &&\n \ttest_must_fail git reset -p aaaaaaaa &&\n \tverify_saved_state dir/foo &&\n@@ -70,27 +70,27 @@ test_expect_success PERL 'git reset -p aaaaaaaa (unknown fails)' '\n # dir/foo. There's always an extra 'n' to reject edits to dir/foo in\n # the failure case (and thus get out of the loop).\n \n-test_expect_success PERL 'git reset -p dir' '\n+test_expect_success 'git reset -p dir' '\n \tset_state dir/foo work work &&\n \ttest_write_lines y n | git reset -p dir &&\n \tverify_state dir/foo work head &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p -- foo (inside dir)' '\n+test_expect_success 'git reset -p -- foo (inside dir)' '\n \tset_state dir/foo work work &&\n \ttest_write_lines y n | (cd dir && git reset -p -- foo) &&\n \tverify_state dir/foo work head &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'git reset -p HEAD^ -- dir' '\n+test_expect_success 'git reset -p HEAD^ -- dir' '\n \ttest_write_lines y n | git reset -p HEAD^ -- dir &&\n \tverify_state dir/foo work parent &&\n \tverify_saved_state bar\n '\n \n-test_expect_success PERL 'none of this moved HEAD' '\n+test_expect_success 'none of this moved HEAD' '\n \tverify_saved_head\n '\n \ndiff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh\nindex d20e5709f9..88d1c8adf4 100755\n--- a/t/t7106-reset-unborn-branch.sh\n+++ b/t/t7106-reset-unborn-branch.sh\n@@ -34,7 +34,7 @@ test_expect_success 'reset $file' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success PERL 'reset -p' '\n+test_expect_success 'reset -p' '\n \trm .git/index &&\n \tgit add a &&\n \techo y >yes &&\ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex 998a2103c7..b4de10a5dd 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -3,12 +3,6 @@\n test_description='hunk edit with \"commit -p -m\"'\n . ./test-lib.sh\n \n-if ! test_have_prereq PERL\n-then\n-\tskip_all=\"skipping '$test_description' tests, perl not available\"\n-\ttest_done\n-fi\n-\n test_expect_success 'setup (initial)' '\n \techo line1 >file &&\n \tgit add file &&\n-- \n2.43.0\n\n"},{"id":"488636","messageId":"497155c2-43d0-4e3f-884f-b0296a56823e@gmail.com","threadId":"60801","inReplyTo":"20240213000601.520731-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH v6 0/2] add-patch: classify '@' as a synonym for 'HEAD'","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-14T11:06:13Z","receivedAt":"2024-02-14T11:06:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOn 13/02/2024 00:05, Ghanshyam Thakkar wrote:\n> I just noticed after sending v5 that, in one of the tests, while\n> moving it inside the loop for also testing '@', set_and_save_state was\n> not used therefore the subsequent tests after the first $opt will not have\n> the changes for which we prove 'y' thereby making the tests useless.\n> (Although it would not fail regardless of this). This got unnoticed by\n> the previous reviews since v1 and me as well.\n\nThis version all looks fine to me, thanks for working on it. Thanks for \nremoving the PERL prerequisite from the remaining tests. I think the \nchange to \"git checkout @\" is a good idea as it makes \"@\" act like \n\"HEAD\", hopefully there aren't too many users relying on \"git checkout \n@\" to detach HEAD.\n\nBest Wishes\n\nPhillip\n\n> Apologies for the oversight and noise.\n> \n> Ghanshyam Thakkar (2):\n> add-patch: classify '@' as a synonym for 'HEAD'\n> add -p tests: remove PERL prerequisites\n> \n> add-patch.c | 8 ------\n> builtin/checkout.c | 4 ++-\n> builtin/reset.c | 4 ++-\n> t/t2016-checkout-patch.sh | 46 +++++++++++++++++++---------------\n> t/t2020-checkout-detach.sh | 12 +++++++++\n> t/t2024-checkout-dwim.sh | 2 +-\n> t/t2071-restore-patch.sh | 46 ++++++++++++++++++----------------\n> t/t3904-stash-patch.sh | 6 -----\n> t/t7105-reset-patch.sh | 38 +++++++++++++++-------------\n> t/t7106-reset-unborn-branch.sh | 2 +-\n> t/t7514-commit-patch.sh | 6 -----\n> 11 files changed, 92 insertions(+), 82 deletions(-)\n> \n> change since v5:\n> diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\n> index 0f597416d8..453872c8ba 100755\n> --- a/t/t7105-reset-patch.sh\n> +++ b/t/t7105-reset-patch.sh\n> @@ -29,6 +29,7 @@ test_expect_success PERL 'saying \"n\" does nothing' '\n> for opt in \"HEAD\" \"@\" \"\"\n> do\n> test_expect_success PERL \"git reset -p $opt\" '\n> + set_and_save_state dir/foo work work &&\n> test_write_lines n y | git reset -p $opt >output &&\n> verify_state dir/foo work head &&\n> verify_saved_state bar &&\n> \n"}]}