| Message ID | 20240203112619.979239-6-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > Therefore, make a new function user_meant_head() which takes the > revision string and compares it to 'HEAD' as well as '@'. However, in > builtin/checkout.c, there is a logic to convert all command line input > rev to the raw object name for underlying machinery (e.g., diff-index) > that does not recognize the <a>...<b> notation, but we'd need to leave > 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD' > to that code and leave '@' intact, too. I am not sure what that "However" wants to say. - Now we have a helper function to see what the end-user said, and tell if the end-user meant the state that is currently checked out (aka "HEAD" but some folks like a synonym "@"[*]), or if the end-user meant some other "concrete" branch. - In builtin/checkout.c, there is a logic to convert unless what the end-user meant is the state that is currently checked out. Isn't the natural conclusion that follows these two stentences "therefore, the latter is a very good place to use that helper function, too"? Side note: the "@" is already problematic not just because "git branch @" would not refuse to create "refs/heads/@", but there is no ref "@" (like $GIT_DIR/refs/@ or $GIT_DIR/@) when it is used as a synonym for "HEAD". There is a check in builtin/checkout.c:update_refs_for_switch() that runs strcmp() on a token given by the end-user from the command line with "HEAD" to notice the no-op case "git checkout HEAD" but the code does not trigger when "@" is given, and it happens to work by accident. I really wish we didn't add that oddball synonym, but that is water under the bridge by now. In any case, I think we'd find more places that currently treats the token "HEAD" given directly by the end-user specially and may want to teach at least some of them to also accept "@" the same way, and the helper function you are introducing may become useful in the future, at which time we may move it to a more public header. If it needs to be shared already between add-patch.c and builtin/checkout.c (I am guessing what you meant with "However" as an excuse for open coding it instead of sharing the code), perhaps we should do so without waiting for that future, though. I dunno. If we choose to do so, for now, a squashable patch may look like the attached, but we'd need to update the log message while squashing it in. add-interactive.h | 14 ++++++++++++++ add-patch.c | 11 +++-------- builtin/checkout.c | 3 +-- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git c/add-interactive.h w/add-interactive.h index 693f125e8e..ca7326336d 100644 --- c/add-interactive.h +++ w/add-interactive.h @@ -38,4 +38,18 @@ enum add_p_mode { int run_add_p(struct repository *r, enum add_p_mode mode, const char *revision, const struct pathspec *ps); +/* + * When the user gives these tokens from the command line, they mean + * the state that the currently checked out state came from. This + * single bit of information affects the direction in which the patch + * is presented to the end-user: are we showing a patch to go back to + * the currently committed state, or are we showing a patch to move + * forward to the given commit that may be different from the + * committed state we started with? + */ +static inline int the_user_meant_head(const char *rev) +{ + return !strcmp(rev, "HEAD") || !strcmp(rev, "@"); +} + #endif diff --git c/add-patch.c w/add-patch.c index 7d565dcb33..5502acebb8 100644 --- c/add-patch.c +++ w/add-patch.c @@ -378,11 +378,6 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) return 0; } -static inline int user_meant_head(const char *rev) -{ - return !strcmp(rev, "HEAD") || !strcmp(rev, "@"); -} - static int is_octal(const char *p, size_t len) { if (!len) @@ -1734,21 +1729,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode, if (mode == ADD_P_STASH) s.mode = &patch_mode_stash; else if (mode == ADD_P_RESET) { - if (!revision || user_meant_head(revision)) + if (!revision || the_user_meant_head(revision)) s.mode = &patch_mode_reset_head; else s.mode = &patch_mode_reset_nothead; } else if (mode == ADD_P_CHECKOUT) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (user_meant_head(revision)) + else if (the_user_meant_head(revision)) s.mode = &patch_mode_checkout_head; else s.mode = &patch_mode_checkout_nothead; } else if (mode == ADD_P_WORKTREE) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (user_meant_head(revision)) + else if (the_user_meant_head(revision)) s.mode = &patch_mode_worktree_head; else s.mode = &patch_mode_worktree_nothead; diff --git c/builtin/checkout.c w/builtin/checkout.c index 79e208ee6d..63c669b157 100644 --- c/builtin/checkout.c +++ w/builtin/checkout.c @@ -544,8 +544,7 @@ static int checkout_paths(const struct checkout_opts *opts, * given a tree-object, new_branch_info->commit would be NULL, * but we do not have to do any replacement, either. */ - if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && - strcmp(rev, "@")) + if (rev && new_branch_info->commit && !the_user_meant_head(rev)) rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid); if (opts->checkout_index && opts->checkout_worktree)
On Sun Feb 4, 2024 at 4:03 AM IST, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > > > Therefore, make a new function user_meant_head() which takes the > > revision string and compares it to 'HEAD' as well as '@'. However, in > > builtin/checkout.c, there is a logic to convert all command line input > > rev to the raw object name for underlying machinery (e.g., diff-index) > > that does not recognize the <a>...<b> notation, but we'd need to leave > > 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD' > > to that code and leave '@' intact, too. > > I am not sure what that "However" wants to say. > > - Now we have a helper function to see what the end-user said, and > tell if the end-user meant the state that is currently checked > out (aka "HEAD" but some folks like a synonym "@"[*]), or if the > end-user meant some other "concrete" branch. > > - In builtin/checkout.c, there is a logic to convert unless what > the end-user meant is the state that is currently checked out. > > Isn't the natural conclusion that follows these two stentences > "therefore, the latter is a very good place to use that helper > function, too"? Yeah, I did not use the helper function in builtin/checkout.c. Hence the "However". But I agree on the point of exporting the function. Therefore I have attached the patch with the updated message below. > Side note: the "@" is already problematic not just because > "git branch @" would not refuse to create "refs/heads/@", > but there is no ref "@" (like $GIT_DIR/refs/@ or $GIT_DIR/@) > when it is used as a synonym for "HEAD". There is a check > in builtin/checkout.c:update_refs_for_switch() that runs > strcmp() on a token given by the end-user from the command > line with "HEAD" to notice the no-op case "git checkout > HEAD" but the code does not trigger when "@" is given, and > it happens to work by accident. I really wish we didn't add > that oddball synonym, but that is water under the bridge by > now. well, I suppose it is maybe annoying from the development perspective, but users seem to like the concept of it[1]. > In any case, I think we'd find more places that currently treats the > token "HEAD" given directly by the end-user specially and may want > to teach at least some of them to also accept "@" the same way, and > the helper function you are introducing may become useful in the > future, at which time we may move it to a more public header. If it > needs to be shared already between add-patch.c and builtin/checkout.c > (I am guessing what you meant with "However" as an excuse for open > coding it instead of sharing the code), perhaps we should do so without > waiting for that future, though. I dunno. Yeah, that "However" was for not using the helper function. > If we choose to do so, for now, a squashable patch may look like the > attached, but we'd need to update the log message while squashing it > in. Thanks for the patch. Updated message is below. [Footnote] [1]: https://www.reddit.com/r/git/comments/k15cqm/do_you_know_is_a_shortcut_for_head/ -- >8 -- Subject: [PATCH] add-patch: classify '@' as a synonym for 'HEAD' Currently, (checkout, reset, restore) commands correctly take '@' as a synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and 'HEAD', different prompts/messages are given by the commands mentioned above (because of applying reverse mode(-R) in case of '@'). This is due to the literal and only string comparison with the word 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously desired, especially since '@' already resolves to 'HEAD'. Therefore, make a new function the_user_meant_head() which takes the revision string and compares it to 'HEAD' as well as '@'. Also in builtin/checkout.c, there is a logic to convert all command line input rev to the raw object name for underlying machinery (e.g., diff-index) that does not recognize the <a>...<b> notation, but we'd need to leave 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too. Therefore, this function is already useful in add-patch.c and builtin/checkout.c and may also become helpful in other places in future. Therefore, it makes sense to export it. There is one unintended side-effect/behavior change of this, even if there exists a branch named '@', when providing '@' as a rev-source to (checkout, reset, restore) commands in patch mode, it will consider it as HEAD. This is due to the behavior of diff-index. However, naming a branch '@' is an obvious foot-gun and there are many existing commands which already take '@' for 'HEAD' regardless of whether 'refs/heads/@' exists or not (e.g., 'git log @', 'git push origin @' etc.). Therefore, this should be fine. Also, add tests for checking the above mentioned synonymity. Co-authored-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- add-interactive.h | 14 ++++++++++++ add-patch.c | 6 ++--- builtin/checkout.c | 10 ++++----- t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++----------------- t/t2071-restore-patch.sh | 18 +++++++++------ t/t7105-reset-patch.sh | 10 +++++++++ 6 files changed, 69 insertions(+), 35 deletions(-) diff --git a/add-interactive.h b/add-interactive.h index 693f125e8e..ca7326336d 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -38,4 +38,18 @@ enum add_p_mode { int run_add_p(struct repository *r, enum add_p_mode mode, const char *revision, const struct pathspec *ps); +/* + * When the user gives these tokens from the command line, they mean + * the state that the currently checked out state came from. This + * single bit of information affects the direction in which the patch + * is presented to the end-user: are we showing a patch to go back to + * the currently committed state, or are we showing a patch to move + * forward to the given commit that may be different from the + * committed state we started with? + */ +static inline int the_user_meant_head(const char *rev) +{ + return !strcmp(rev, "HEAD") || !strcmp(rev, "@"); +} + #endif diff --git a/add-patch.c b/add-patch.c index 68f525b35c..5502acebb8 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1729,21 +1729,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode, if (mode == ADD_P_STASH) s.mode = &patch_mode_stash; else if (mode == ADD_P_RESET) { - if (!revision || !strcmp(revision, "HEAD")) + if (!revision || the_user_meant_head(revision)) s.mode = &patch_mode_reset_head; else s.mode = &patch_mode_reset_nothead; } else if (mode == ADD_P_CHECKOUT) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (the_user_meant_head(revision)) s.mode = &patch_mode_checkout_head; else s.mode = &patch_mode_checkout_nothead; } else if (mode == ADD_P_WORKTREE) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (the_user_meant_head(revision)) s.mode = &patch_mode_worktree_head; else s.mode = &patch_mode_worktree_nothead; diff --git a/builtin/checkout.c b/builtin/checkout.c index a6e30931b5..63c669b157 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -539,12 +539,12 @@ static int checkout_paths(const struct checkout_opts *opts, * recognized by diff-index), we will always replace the name * with the hex of the commit (whether it's in `...` form or * not) for the run_add_interactive() machinery to work - * properly. However, there is special logic for the HEAD case - * so we mustn't replace that. Also, when we were given a - * tree-object, new_branch_info->commit would be NULL, but we - * do not have to do any replacement, either. + * properly. However, there is special logic for the 'HEAD' and + * '@' case so we mustn't replace that. Also, when we were + * given a tree-object, new_branch_info->commit would be NULL, + * but we do not have to do any replacement, either. */ - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) + if (rev && new_branch_info->commit && !the_user_meant_head(rev)) rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid); if (opts->checkout_index && opts->checkout_worktree) diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh index 747eb5563e..c4f9bf09aa 100755 --- a/t/t2016-checkout-patch.sh +++ b/t/t2016-checkout-patch.sh @@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success 'git checkout -p HEAD with NO staged changes: abort' ' - set_and_save_state dir/foo work head && - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_saved_state dir/foo -' - -test_expect_success 'git checkout -p HEAD with NO staged changes: apply' ' - test_write_lines n y y | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' - -test_expect_success 'git checkout -p HEAD with change already staged' ' - set_state dir/foo index index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' +for opt in "HEAD" "@" +do + test_expect_success "git checkout -p $opt with NO staged changes: abort" ' + set_and_save_state dir/foo work head && + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_saved_state dir/foo && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with NO staged changes: apply" ' + test_write_lines n y y | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with change already staged" ' + set_state dir/foo index index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' +done test_expect_success 'git checkout -p HEAD^...' ' # the third n is to get out in case it mistakenly does not apply diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh index b5c5c0ff7e..3dc9184b4a 100755 --- a/t/t2071-restore-patch.sh +++ b/t/t2071-restore-patch.sh @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success PERL 'git restore -p --source=HEAD' ' - set_state dir/foo work index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git restore -p --source=HEAD && - verify_saved_state bar && - verify_state dir/foo head index -' +for opt in "HEAD" "@" +do + test_expect_success PERL "git restore -p --source=$opt" ' + set_state dir/foo work index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git restore -p --source=$opt >output && + verify_saved_state bar && + verify_state dir/foo head index && + test_grep "Discard" output + ' +done test_expect_success PERL 'git restore -p --source=HEAD^' ' set_state dir/foo work index && diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh index 05079c7246..ec7f16dfb6 100755 --- a/t/t7105-reset-patch.sh +++ b/t/t7105-reset-patch.sh @@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' ' test_grep "Unstage" output ' +for opt in "HEAD" "@" +do + test_expect_success PERL "git reset -p $opt" ' + test_write_lines n y | git reset -p $opt >output && + verify_state dir/foo work head && + verify_saved_state bar && + test_grep "Unstage" output + ' +done + test_expect_success PERL 'git reset -p HEAD^' ' test_write_lines n y | git reset -p HEAD^ >output && verify_state dir/foo work parent &&
Hi Ghanshyam I think this is a useful addition, I've left a couple of comments below On 03/02/2024 11:25, Ghanshyam Thakkar wrote: > diff --git a/add-patch.c b/add-patch.c > index 68f525b35c..7d565dcb33 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) > return 0; > } > > +static inline int user_meant_head(const char *rev) > +{ > + return !strcmp(rev, "HEAD") || !strcmp(rev, "@"); > +} > + As well as the places you have converted we also have an explicit test for "HEAD" in parse_diff() which looks like if (s->revision) { struct object_id oid; strvec_push(&args, /* could be on an unborn branch */ !strcmp("HEAD", s->revision) && repo_get_oid(the_repository, "HEAD", &oid) ? empty_tree_oid_hex() : s->revision); } I suspect we need to update that code as well to accept "@" as a synonym for "HEAD" on an unborn branch. > diff --git a/builtin/checkout.c b/builtin/checkout.c > index a6e30931b5..79e208ee6d 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts, > * recognized by diff-index), we will always replace the name > * with the hex of the commit (whether it's in `...` form or > * not) for the run_add_interactive() machinery to work > - * properly. However, there is special logic for the HEAD case > - * so we mustn't replace that. Also, when we were given a > - * tree-object, new_branch_info->commit would be NULL, but we > - * do not have to do any replacement, either. > + * properly. However, there is special logic for the 'HEAD' and > + * '@' case so we mustn't replace that. Also, when we were > + * given a tree-object, new_branch_info->commit would be NULL, > + * but we do not have to do any replacement, either. > */ > - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) > + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && > + strcmp(rev, "@")) > rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid); I agree with Junio's suggestion to use the user_meant_head() here. Looking at this made me wonder why builtin/reset.c does not need updating. The answer seems to be that reset passes in the revision as given on the commandline after checking it refers to a valid tree whereas for checkout the revision for on the commandline might contain "..." which run_add_p() cannot handle. > diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh > index b5c5c0ff7e..3dc9184b4a 100755 > --- a/t/t2071-restore-patch.sh > +++ b/t/t2071-restore-patch.sh > @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' ' It is a pre-existing problem but all these "PERL" prerequisites are no-longer required as we've removed the perl implementation of "add -p" > verify_state dir/foo index index > ' > > -test_expect_success PERL 'git restore -p --source=HEAD' ' > - set_state dir/foo work index && > - # the third n is to get out in case it mistakenly does not apply > - test_write_lines n y n | git restore -p --source=HEAD && > - verify_saved_state bar && > - verify_state dir/foo head index > -' > +for opt in "HEAD" "@" > +do > + test_expect_success PERL "git restore -p --source=$opt" ' > + set_state dir/foo work index && > + # the third n is to get out in case it mistakenly does not apply > + test_write_lines n y n | git restore -p --source=$opt >output && > + verify_saved_state bar && > + verify_state dir/foo head index && > + test_grep "Discard" output It is good to see that we're now testing for a reversed patch here. Best Wishes Phillip
On Mon Feb 5, 2024 at 10:07 PM IST, Phillip Wood wrote: > On 03/02/2024 11:25, Ghanshyam Thakkar wrote: > > diff --git a/add-patch.c b/add-patch.c > > index 68f525b35c..7d565dcb33 100644 > > --- a/add-patch.c > > +++ b/add-patch.c > > @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) > > return 0; > > } > > > > +static inline int user_meant_head(const char *rev) > > +{ > > + return !strcmp(rev, "HEAD") || !strcmp(rev, "@"); > > +} > > + > > As well as the places you have converted we also have an explicit test > for "HEAD" in parse_diff() which looks like > > if (s->revision) { > struct object_id oid; > strvec_push(&args, > /* could be on an unborn branch */ > !strcmp("HEAD", s->revision) && > repo_get_oid(the_repository, "HEAD", &oid) ? > empty_tree_oid_hex() : s->revision); > } > > I suspect we need to update that code as well to accept "@" as a synonym > for "HEAD" on an unborn branch. I had already considered that. Updating here will not have any effect, because on unborn branch, we do not allow naming HEAD or @. This case is for when we run without naming any revision (i.e. git reset -p) on unborn branch. In such cases, we pass 'HEAD' as a default value. > > > diff --git a/builtin/checkout.c b/builtin/checkout.c > > index a6e30931b5..79e208ee6d 100644 > > --- a/builtin/checkout.c > > +++ b/builtin/checkout.c > > @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts, > > * recognized by diff-index), we will always replace the name > > * with the hex of the commit (whether it's in `...` form or > > * not) for the run_add_interactive() machinery to work > > - * properly. However, there is special logic for the HEAD case > > - * so we mustn't replace that. Also, when we were given a > > - * tree-object, new_branch_info->commit would be NULL, but we > > - * do not have to do any replacement, either. > > + * properly. However, there is special logic for the 'HEAD' and > > + * '@' case so we mustn't replace that. Also, when we were > > + * given a tree-object, new_branch_info->commit would be NULL, > > + * but we do not have to do any replacement, either. > > */ > > - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) > > + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && > > + strcmp(rev, "@")) > > rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid); > > I agree with Junio's suggestion to use the user_meant_head() here. > Looking at this made me wonder why builtin/reset.c does not need > updating. The answer seems to be that reset passes in the revision as > given on the commandline after checking it refers to a valid tree > whereas for checkout the revision for on the commandline might contain > "..." which run_add_p() cannot handle. I was not able to run reset with '...'. I ran, 'git reset main...$ANOTHERBRANCH' but it gave me "fatal: ambiguous argument 'main...$ANOTHERBRANCH'" error, with or without -p. While, 'git restore --source=main...$ANOTHERBRANCH .' and 'git checkout main...$ANOTHERBRANCH' works fine, with or without -p. > > diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh > > index b5c5c0ff7e..3dc9184b4a 100755 > > --- a/t/t2071-restore-patch.sh > > +++ b/t/t2071-restore-patch.sh > > @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' ' > > It is a pre-existing problem but all these "PERL" prerequisites are > no-longer required as we've removed the perl implementation of "add -p" I can send a separate patch to clean up this script, removing PERL pre-req from all tests. > > verify_state dir/foo index index > > ' > > > > -test_expect_success PERL 'git restore -p --source=HEAD' ' > > - set_state dir/foo work index && > > - # the third n is to get out in case it mistakenly does not apply > > - test_write_lines n y n | git restore -p --source=HEAD && > > - verify_saved_state bar && > > - verify_state dir/foo head index > > -' > > +for opt in "HEAD" "@" > > +do > > + test_expect_success PERL "git restore -p --source=$opt" ' > > + set_state dir/foo work index && > > + # the third n is to get out in case it mistakenly does not apply > > + test_write_lines n y n | git restore -p --source=$opt >output && > > + verify_saved_state bar && > > + verify_state dir/foo head index && > > + test_grep "Discard" output > > It is good to see that we're now testing for a reversed patch here. > > Best Wishes > > Phillip Thanks for the review.
Phillip Wood <phillip.wood123@gmail.com> writes: > As well as the places you have converted we also have an explicit test > for "HEAD" in parse_diff() which looks like > > if (s->revision) { > struct object_id oid; > strvec_push(&args, > /* could be on an unborn branch */ > !strcmp("HEAD", s->revision) && > repo_get_oid(the_repository, "HEAD", &oid) ? > empty_tree_oid_hex() : s->revision); > } > > I suspect we need to update that code as well to accept "@" as a > synonym for "HEAD" on an unborn branch. I actually think "@" in the input should be normalized to "HEAD" as soon as possible. After the if/elseif/ cascade in run_add_p() the patch in this thread touches, there is an assignment s.revision = revision; and even though we rewrite !strcmp(revision, "HEAD") to "user means HEAD" to additionally accept "@" in that if/elseif/ cascade, here we will stuff different values to s.revision here. We could normalize the end-user supplied "@" to "HEAD" before making this assignment, then you do not have to worry about the code in parse_diff() above, and more importantly, we do not have to rely on what the current set of callers happen to do and do not happen to do (e.g., "git reset -p" happens to use hardcoded "HEAD" for unborn case without using anything obtained from the user, so the above code in parse_diff() might be safe in that case, but we do not want to rely on such subtlety to make sure our code is correct) Come to think of it, we could even do the "normalizing" even before, and that might greatly simplify things. For example, if we did so at the very beginning of run_add_p(), before we come to that if/else if/ cascade, we may not even have to worry about the "user meant HEAD" helper. After the normalization, we can just keep using !strcmp() with "HEAD" alone. Simple and clean, no? Thanks.
diff --git a/add-patch.c b/add-patch.c index 68f525b35c..7d565dcb33 100644 --- a/add-patch.c +++ b/add-patch.c @@ -378,6 +378,11 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) return 0; } +static inline int user_meant_head(const char *rev) +{ + return !strcmp(rev, "HEAD") || !strcmp(rev, "@"); +} + static int is_octal(const char *p, size_t len) { if (!len) @@ -1729,21 +1734,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode, if (mode == ADD_P_STASH) s.mode = &patch_mode_stash; else if (mode == ADD_P_RESET) { - if (!revision || !strcmp(revision, "HEAD")) + if (!revision || user_meant_head(revision)) s.mode = &patch_mode_reset_head; else s.mode = &patch_mode_reset_nothead; } else if (mode == ADD_P_CHECKOUT) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (user_meant_head(revision)) s.mode = &patch_mode_checkout_head; else s.mode = &patch_mode_checkout_nothead; } else if (mode == ADD_P_WORKTREE) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (user_meant_head(revision)) s.mode = &patch_mode_worktree_head; else s.mode = &patch_mode_worktree_nothead; diff --git a/builtin/checkout.c b/builtin/checkout.c index a6e30931b5..79e208ee6d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -539,12 +539,13 @@ static int checkout_paths(const struct checkout_opts *opts, * recognized by diff-index), we will always replace the name * with the hex of the commit (whether it's in `...` form or * not) for the run_add_interactive() machinery to work - * properly. However, there is special logic for the HEAD case - * so we mustn't replace that. Also, when we were given a - * tree-object, new_branch_info->commit would be NULL, but we - * do not have to do any replacement, either. + * properly. However, there is special logic for the 'HEAD' and + * '@' case so we mustn't replace that. Also, when we were + * given a tree-object, new_branch_info->commit would be NULL, + * but we do not have to do any replacement, either. */ - if (rev && new_branch_info->commit && strcmp(rev, "HEAD")) + if (rev && new_branch_info->commit && strcmp(rev, "HEAD") && + strcmp(rev, "@")) rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid); if (opts->checkout_index && opts->checkout_worktree) diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh index 747eb5563e..c4f9bf09aa 100755 --- a/t/t2016-checkout-patch.sh +++ b/t/t2016-checkout-patch.sh @@ -38,26 +38,32 @@ test_expect_success 'git checkout -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success 'git checkout -p HEAD with NO staged changes: abort' ' - set_and_save_state dir/foo work head && - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_saved_state dir/foo -' - -test_expect_success 'git checkout -p HEAD with NO staged changes: apply' ' - test_write_lines n y y | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' - -test_expect_success 'git checkout -p HEAD with change already staged' ' - set_state dir/foo index index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git checkout -p HEAD && - verify_saved_state bar && - verify_state dir/foo head head -' +for opt in "HEAD" "@" +do + test_expect_success "git checkout -p $opt with NO staged changes: abort" ' + set_and_save_state dir/foo work head && + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_saved_state dir/foo && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with NO staged changes: apply" ' + test_write_lines n y y | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' + + test_expect_success "git checkout -p $opt with change already staged" ' + set_state dir/foo index index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git checkout -p $opt >output && + verify_saved_state bar && + verify_state dir/foo head head && + test_grep "Discard" output + ' +done test_expect_success 'git checkout -p HEAD^...' ' # the third n is to get out in case it mistakenly does not apply diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh index b5c5c0ff7e..3dc9184b4a 100755 --- a/t/t2071-restore-patch.sh +++ b/t/t2071-restore-patch.sh @@ -44,13 +44,17 @@ test_expect_success PERL 'git restore -p with staged changes' ' verify_state dir/foo index index ' -test_expect_success PERL 'git restore -p --source=HEAD' ' - set_state dir/foo work index && - # the third n is to get out in case it mistakenly does not apply - test_write_lines n y n | git restore -p --source=HEAD && - verify_saved_state bar && - verify_state dir/foo head index -' +for opt in "HEAD" "@" +do + test_expect_success PERL "git restore -p --source=$opt" ' + set_state dir/foo work index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git restore -p --source=$opt >output && + verify_saved_state bar && + verify_state dir/foo head index && + test_grep "Discard" output + ' +done test_expect_success PERL 'git restore -p --source=HEAD^' ' set_state dir/foo work index && diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh index 05079c7246..ec7f16dfb6 100755 --- a/t/t7105-reset-patch.sh +++ b/t/t7105-reset-patch.sh @@ -33,6 +33,16 @@ test_expect_success PERL 'git reset -p' ' test_grep "Unstage" output ' +for opt in "HEAD" "@" +do + test_expect_success PERL "git reset -p $opt" ' + test_write_lines n y | git reset -p $opt >output && + verify_state dir/foo work head && + verify_saved_state bar && + test_grep "Unstage" output + ' +done + test_expect_success PERL 'git reset -p HEAD^' ' test_write_lines n y | git reset -p HEAD^ >output && verify_state dir/foo work parent &&
Currently, (checkout, reset, restore) commands correctly take '@' as a synonym for 'HEAD'. However, in patch mode (-p/--patch), for both '@' and 'HEAD', different prompts/messages are given by the commands mentioned above. This is due to the literal and only string comparison with the word 'HEAD' in run_add_p(). Synonymity between '@' and 'HEAD' is obviously desired, especially since '@' already resolves to 'HEAD'. Therefore, make a new function user_meant_head() which takes the revision string and compares it to 'HEAD' as well as '@'. However, in builtin/checkout.c, there is a logic to convert all command line input rev to the raw object name for underlying machinery (e.g., diff-index) that does not recognize the <a>...<b> notation, but we'd need to leave 'HEAD' intact. Now we need to teach that '@' is a synonym to 'HEAD' to that code and leave '@' intact, too. There is one unintended side-effect/behavior change of this, even if there exists a branch named '@', when providing '@' as a rev-source to (checkout, reset, restore) commands in patch mode, it will consider it as HEAD. This is due to the behavior of diff-index. However, naming a branch '@' is an obvious foot-gun and there are many existing commands which take '@' for 'HEAD' even if 'refs/heads/@' exists (e.g., 'git log @', 'git push origin @' etc.). Therefore, this should be fine. Also, add tests to check the above mentioned synonymity between 'HEAD' and '@'. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- add-patch.c | 11 +++++++--- builtin/checkout.c | 11 +++++----- t/t2016-checkout-patch.sh | 46 ++++++++++++++++++++++----------------- t/t2071-restore-patch.sh | 18 +++++++++------ t/t7105-reset-patch.sh | 10 +++++++++ 5 files changed, 61 insertions(+), 35 deletions(-)