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.
