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
