| Message ID | 20240202150434.11256-3-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add-patch: Support '@' 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. This is due to the literal and only string comparison with the word > 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously > desired, especially since '@' already resolves to 'HEAD'. > > Therefore, make a new function user_meant_head() which takes the > revision string and compares it to 'HEAD' as well as '@'. However, in > builtin/checkout.c, there is some logic to convert all revs besides > 'HEAD' to hex revs due to 'diff-index', which is used in patch mode > machinery, not being able to handle '<a>...<b>' revs. Therefore, in > addition to 'HEAD', do not convert '@' as well, so it can be later > used in assigning patch_mode_(...)_head. In this context <a>...<b> names a single rev (not two revs) that is the merge base of <a> and <b>. Perhaps ... there is a logic to convert all command line input rev to the raw object name for underlying machinery (e.g., diff-index) that does not recognize the <a>...<b> notation, but we'd need to leave "HEAD" intact. Now we need to teach that "@" is a synonym to "HEAD" to that code, too. which may be a bit shorter. You decided to use is_rev_head() instead of user_meant_head(), so you'd need to update the above description to match, I think. > - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) > + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && > + strcmp(rev, "@")) Shouldn't this be if (rev && new_branch_info->commit && !is_rev_head(rev)) instead of "HEAD" and "@" spelled out? Other than the above, nicely done.
Junio C Hamano <gitster@pobox.com> writes: > You decided to use is_rev_head() instead of user_meant_head(), so > you'd need to update the above description to match, I think. Having said this, I have a slight fear that normal users would expect is_rev_head(X) to say "yes" for "master" when the current branch is "master", though. is_head(X) would have the same downside.
On Fri Feb 2, 2024 at 10:38 PM IST, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > > > Currently, (checkout, reset, restore) commands correctly take '@' as a > > synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and > > 'HEAD', different prompts/messages are given by the commands mentioned > > above. This is due to the literal and only string comparison with the word > > 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously > > desired, especially since '@' already resolves to 'HEAD'. > > > > Therefore, make a new function user_meant_head() which takes the > > revision string and compares it to 'HEAD' as well as '@'. However, in > > builtin/checkout.c, there is some logic to convert all revs besides > > 'HEAD' to hex revs due to 'diff-index', which is used in patch mode > > machinery, not being able to handle '<a>...<b>' revs. Therefore, in > > addition to 'HEAD', do not convert '@' as well, so it can be later > > used in assigning patch_mode_(...)_head. > > In this context <a>...<b> names a single rev (not two revs) that is > the merge base of <a> and <b>. Perhaps I meant revs which are spelled out in the form of <a>...<b> and not the <a> and <b> themselves. But I can see how it can be confusing. I will change it. > ... there is a logic to convert all command line input rev to > the raw object name for underlying machinery (e.g., diff-index) > that does not recognize the <a>...<b> notation, but we'd need to > leave "HEAD" intact. Now we need to teach that "@" is a synonym > to "HEAD" to that code, too. > > which may be a bit shorter. Yeah, that is much better and clearer also. > You decided to use is_rev_head() instead of user_meant_head(), so > you'd need to update the above description to match, I think. Will update. > > - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) > > + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && > > + strcmp(rev, "@")) > > Shouldn't this be > > if (rev && new_branch_info->commit && !is_rev_head(rev)) > > instead of "HEAD" and "@" spelled out? is_rev_head() is in add-patch.c and the above is in builtin/checkout.c and is_rev_head() is not exported. I can also define it in builtin/checkout.c, but this would be the only instance in that file which will use it. So, I think it is better to just add strcmp(rev, "@") to the if condition.
On Fri Feb 2, 2024 at 11:13 PM IST, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > You decided to use is_rev_head() instead of user_meant_head(), so > > you'd need to update the above description to match, I think. > > Having said this, I have a slight fear that normal users would > expect is_rev_head(X) to say "yes" for "master" when the current > branch is "master", though. is_head(X) would have the same > downside. Yeah, user_meant_head() looks like the better pick. I'll update it.
diff --git a/add-patch.c b/add-patch.c index 68f525b35c..6c70a0240c 100644 --- a/add-patch.c +++ b/add-patch.c @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) return 0; } +static inline int is_rev_head(const char *rev) +{ + return !strcmp(rev, "HEAD") || !strcmp(rev, "@"); +} + static int is_octal(const char *p, size_t len) { if (!len) @@ -1729,21 +1734,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode, if (mode == ADD_P_STASH) s.mode = &patch_mode_stash; else if (mode == ADD_P_RESET) { - if (!revision || !strcmp(revision, "HEAD")) + if (!revision || is_rev_head(revision)) s.mode = &patch_mode_reset_head; else s.mode = &patch_mode_reset_nothead; } else if (mode == ADD_P_CHECKOUT) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (is_rev_head(revision)) s.mode = &patch_mode_checkout_head; else s.mode = &patch_mode_checkout_nothead; } else if (mode == ADD_P_WORKTREE) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (is_rev_head(revision)) s.mode = &patch_mode_worktree_head; else s.mode = &patch_mode_worktree_nothead; diff --git a/builtin/checkout.c b/builtin/checkout.c index a6e30931b5..79e208ee6d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts, * recognized by diff-index), we will always replace the name * with the hex of the commit (whether it's in `...` form or * not) for the run_add_interactive() machinery to work - * properly. However, there is special logic for the HEAD case - * so we mustn't replace that. Also, when we were given a - * tree-object, new_branch_info->commit would be NULL, but we - * do not have to do any replacement, either. + * properly. However, there is special logic for the 'HEAD' and + * '@' case so we mustn't replace that. Also, when we were + * given a tree-object, new_branch_info->commit would be NULL, + * but we do not have to do any replacement, either. */ - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && + strcmp(rev, "@")) rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid); if (opts->checkout_index && opts->checkout_worktree) diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh index 747eb5563e..c4f9bf09aa 100755 --- a/t/t2016-checkout-patch.sh +++ b/t/t2016-checkout-patch.sh @@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success 'git checkout -p HEAD with NO staged changes: abort' ' - set_and_save_state dir/foo work head && - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_saved_state dir/foo -' - -test_expect_success 'git checkout -p HEAD with NO staged changes: apply' ' - test_write_lines n y y | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' - -test_expect_success 'git checkout -p HEAD with change already staged' ' - set_state dir/foo index index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' +for opt in "HEAD" "@" +do + test_expect_success "git checkout -p $opt with NO staged changes: abort" ' + set_and_save_state dir/foo work head && + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_saved_state dir/foo && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with NO staged changes: apply" ' + test_write_lines n y y | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with change already staged" ' + set_state dir/foo index index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' +done test_expect_success 'git checkout -p HEAD^...' ' # the third n is to get out in case it mistakenly does not apply diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh index b5c5c0ff7e..3dc9184b4a 100755 --- a/t/t2071-restore-patch.sh +++ b/t/t2071-restore-patch.sh @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success PERL 'git restore -p --source=HEAD' ' - set_state dir/foo work index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git restore -p --source=HEAD && - verify_saved_state bar && - verify_state dir/foo head index -' +for opt in "HEAD" "@" +do + test_expect_success PERL "git restore -p --source=$opt" ' + set_state dir/foo work index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git restore -p --source=$opt >output && + verify_saved_state bar && + verify_state dir/foo head index && + test_grep "Discard" output + ' +done test_expect_success PERL 'git restore -p --source=HEAD^' ' set_state dir/foo work index && diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh index 05079c7246..ec7f16dfb6 100755 --- a/t/t7105-reset-patch.sh +++ b/t/t7105-reset-patch.sh @@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' ' test_grep "Unstage" output ' +for opt in "HEAD" "@" +do + test_expect_success PERL "git reset -p $opt" ' + test_write_lines n y | git reset -p $opt >output && + verify_state dir/foo work head && + verify_saved_state bar && + test_grep "Unstage" output + ' +done + test_expect_success PERL 'git reset -p HEAD^' ' test_write_lines n y | git reset -p HEAD^ >output && verify_state dir/foo work parent &&
Currently, (checkout, reset, restore) commands correctly take '@' as a synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and 'HEAD', different prompts/messages are given by the commands mentioned above. This is due to the literal and only string comparison with the word 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously desired, especially since '@' already resolves to 'HEAD'. Therefore, make a new function user_meant_head() which takes the revision string and compares it to 'HEAD' as well as '@'. However, in builtin/checkout.c, there is some logic to convert all revs besides 'HEAD' to hex revs due to 'diff-index', which is used in patch mode machinery, not being able to handle '<a>...<b>' revs. Therefore, in addition to 'HEAD', do not convert '@' as well, so it can be later used in assigning patch_mode_(...)_head. There is one unintended side-effect/behavior change of this, even if there exists a branch named '@', when providing '@' as a rev-source to (checkout, reset, restore) commands in patch mode, it will consider it as HEAD. This is due to the behavior of diff-index. However, naming a branch '@' is an obvious foot-gun and there are many existing commands which take '@' for 'HEAD' even if 'refs/heads/@' exists (e.g., 'git log @', 'git push origin @' etc.). Therefore, this should be fine. Also, add tests to check the above mentioned synonymity between 'HEAD' and '@'. Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- add-patch.c | 11 +++++++--- builtin/checkout.c | 11 +++++----- t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++----------------- t/t2071-restore-patch.sh | 18 +++++++++------ t/t7105-reset-patch.sh | 10 +++++++++ 5 files changed, 61 insertions(+), 35 deletions(-)