Re: [PATCH v3 2/2] add-patch: classify '@' as a synonym for 'HEAD'

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

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.