| Message ID | 20240206225122.1095766-6-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add-patch: '@' as a synonym for 'HEAD' | expand |
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > Currently, (checkout, reset, restore) commands correctly take '@' as a > synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' > and 'HEAD', different prompts/messages are given by the commands > mentioned above (because of applying reverse mode(-R) in case of '@'). > This is due to the literal and only string comparison with the word > 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously > desired, especially since '@' already resolves to 'HEAD'. > > Therefore, replace '@' to 'HEAD' at the beginning of > add-patch.c:run_add_p(). Of course there is only one possible downside for this approach, in that if we are using "revision" in an error message, users who asked for "@" may complain when an error message says "HEAD" in it. I think the simplicity of the implementation far outweighs this downside. > There is also logic in builtin/checkout.c to > convert all command line input rev to the raw object name for underlying > machinery (e.g., diff-index) that does not recognize the <a>...<b> > notation, but we'd need to leave 'HEAD' intact. Now we need to teach > that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too. Makes me wonder why we cannot use the same "normalize @ to HEAD upfront" approach here, though? It would involve translating "@" given to new_branch_info->name to "HEAD" early, possibly in setup_new_branch_info_and_source_tree(), and that probably will fix the other strcmp() with "HEAD" that appears in builtin/checkout.c:update_refs_for_switch() as well, no? > + /* helpful in deciding the patch mode ahead */ > + if(revision && !strcmp(revision, "@")) > + revision = "HEAD"; Style. "if (revision ...)"
On 07/02/2024 01:05, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > >> Currently, (checkout, reset, restore) commands correctly take '@' as a >> synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' >> and 'HEAD', different prompts/messages are given by the commands >> mentioned above (because of applying reverse mode(-R) in case of '@'). >> This is due to the literal and only string comparison with the word >> 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously >> desired, especially since '@' already resolves to 'HEAD'. >> >> Therefore, replace '@' to 'HEAD' at the beginning of >> add-patch.c:run_add_p(). > > Of course there is only one possible downside for this approach, in > that if we are using "revision" in an error message, users who asked > for "@" may complain when an error message says "HEAD" in it. I think > the simplicity of the implementation far outweighs this downside. I agree, if we were replacing the revision the user gave us with a hex object id that would be confusing but as "@" is just a shortcut for "HEAD" I think replacing it in the error message is fine. It was a good idea just to replace "@" with "HEAD", this version is much simpler. Best Wishes Phillip >> There is also logic in builtin/checkout.c to >> convert all command line input rev to the raw object name for underlying >> machinery (e.g., diff-index) that does not recognize the <a>...<b> >> notation, but we'd need to leave 'HEAD' intact. Now we need to teach >> that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too. > > Makes me wonder why we cannot use the same "normalize @ to HEAD > upfront" approach here, though? > > It would involve translating "@" given to new_branch_info->name to > "HEAD" early, possibly in setup_new_branch_info_and_source_tree(), > and that probably will fix the other strcmp() with "HEAD" that > appears in builtin/checkout.c:update_refs_for_switch() as well, no? > >> + /* helpful in deciding the patch mode ahead */ >> + if(revision && !strcmp(revision, "@")) >> + revision = "HEAD"; > > Style. "if (revision ...)" >
On Wed Feb 7, 2024 at 6:35 AM IST, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > > There is also logic in builtin/checkout.c to > > convert all command line input rev to the raw object name for underlying > > machinery (e.g., diff-index) that does not recognize the <a>...<b> > > notation, but we'd need to leave 'HEAD' intact. Now we need to teach > > that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too. > > Makes me wonder why we cannot use the same "normalize @ to HEAD > upfront" approach here, though? > > It would involve translating "@" given to new_branch_info->name to > "HEAD" early, possibly in setup_new_branch_info_and_source_tree(), > and that probably will fix the other strcmp() with "HEAD" that > appears in builtin/checkout.c:update_refs_for_switch() as well, no? I was wondering about "git checkout -b @". If we were to make the change in setup_new_branch_info_and_source_tree(), then '@' would not be treated as 'HEAD' in the above mentioned case and there would also be some disparity on error messages showing '@' when running the above command, and after hitting setup_new_branch_info_and_source_tree() would show 'HEAD'. However, making the change in builtin/checkout.c:checkout_main() would disallow creating a '@' branch alltogether. Hence, I would like some feedback on whether we should change the behavior of 'git checkout -b @' or not.
diff --git a/add-patch.c b/add-patch.c index 68f525b35c..6f4ca8f4e4 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1726,6 +1726,10 @@ int run_add_p(struct repository *r, enum add_p_mode mode, init_add_i_state(&s.s, r); + /* helpful in deciding the patch mode ahead */ + if(revision && !strcmp(revision, "@")) + revision = "HEAD"; + if (mode == ADD_P_STASH) s.mode = &patch_mode_stash; else if (mode == ADD_P_RESET) { diff --git a/builtin/checkout.c b/builtin/checkout.c index a6e30931b5..79e208ee6d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts, * recognized by diff-index), we will always replace the name * with the hex of the commit (whether it's in `...` form or * not) for the run_add_interactive() machinery to work - * properly. However, there is special logic for the HEAD case - * so we mustn't replace that. Also, when we were given a - * tree-object, new_branch_info->commit would be NULL, but we - * do not have to do any replacement, either. + * properly. However, there is special logic for the 'HEAD' and + * '@' case so we mustn't replace that. Also, when we were + * given a tree-object, new_branch_info->commit would be NULL, + * but we do not have to do any replacement, either. */ - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && + strcmp(rev, "@")) rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid); if (opts->checkout_index && opts->checkout_worktree) diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh index 747eb5563e..c4f9bf09aa 100755 --- a/t/t2016-checkout-patch.sh +++ b/t/t2016-checkout-patch.sh @@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success 'git checkout -p HEAD with NO staged changes: abort' ' - set_and_save_state dir/foo work head && - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_saved_state dir/foo -' - -test_expect_success 'git checkout -p HEAD with NO staged changes: apply' ' - test_write_lines n y y | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' - -test_expect_success 'git checkout -p HEAD with change already staged' ' - set_state dir/foo index index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' +for opt in "HEAD" "@" +do + test_expect_success "git checkout -p $opt with NO staged changes: abort" ' + set_and_save_state dir/foo work head && + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_saved_state dir/foo && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with NO staged changes: apply" ' + test_write_lines n y y | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with change already staged" ' + set_state dir/foo index index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' +done test_expect_success 'git checkout -p HEAD^...' ' # the third n is to get out in case it mistakenly does not apply diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh index b5c5c0ff7e..dbbefc188d 100755 --- a/t/t2071-restore-patch.sh +++ b/t/t2071-restore-patch.sh @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success PERL 'git restore -p --source=HEAD' ' - set_state dir/foo work index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git restore -p --source=HEAD && - verify_saved_state bar && - verify_state dir/foo head index -' +for opt in "HEAD" "@" +do + test_expect_success "git restore -p --source=$opt" ' + set_state dir/foo work index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git restore -p --source=$opt >output && + verify_saved_state bar && + verify_state dir/foo head index && + test_grep "Discard" output + ' +done test_expect_success PERL 'git restore -p --source=HEAD^' ' set_state dir/foo work index && diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh index 05079c7246..7147148138 100755 --- a/t/t7105-reset-patch.sh +++ b/t/t7105-reset-patch.sh @@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' ' test_grep "Unstage" output ' +for opt in "HEAD" "@" +do + test_expect_success "git reset -p $opt" ' + test_write_lines n y | git reset -p $opt >output && + verify_state dir/foo work head && + verify_saved_state bar && + test_grep "Unstage" output + ' +done + test_expect_success PERL 'git reset -p HEAD^' ' test_write_lines n y | git reset -p HEAD^ >output && verify_state dir/foo work parent &&
Currently, (checkout, reset, restore) commands correctly take '@' as a synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and 'HEAD', different prompts/messages are given by the commands mentioned above (because of applying reverse mode(-R) in case of '@'). This is due to the literal and only string comparison with the word 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously desired, especially since '@' already resolves to 'HEAD'. Therefore, replace '@' to 'HEAD' at the beginning of add-patch.c:run_add_p(). There is also logic in builtin/checkout.c to convert all command line input rev to the raw object name for underlying machinery (e.g., diff-index) that does not recognize the <a>...<b> notation, but we'd need to leave 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too. There is one unintended side-effect/behavior change of this, even if there exists a branch named '@', when providing '@' as a rev-source to (checkout, reset, restore) commands in patch mode, it will consider it as HEAD. This is due to the behavior of diff-index. However, naming a branch '@' is an obvious foot-gun and there are many existing commands which already take '@' for 'HEAD' regardless of whether 'refs/heads/@' exists or not (e.g., 'git log @', 'git push origin @' etc.). Therefore, this should be fine. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- add-patch.c | 4 ++++ builtin/checkout.c | 11 +++++----- t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++----------------- t/t2071-restore-patch.sh | 18 +++++++++------ t/t7105-reset-patch.sh | 10 +++++++++ 5 files changed, 57 insertions(+), 32 deletions(-)