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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 5, 2024, 16:37 UTC
Message-ID: <9f9f26f1-5460-468e-a893-5caf7fbea981@gmail.com>
In-Reply-To: <20240203112619.979239-6-shyamthakkar001@gmail.com>

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:

Show 13 quoted lines

> 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.

Show 21 quoted lines

> 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.

Show 5 quoted lines

> 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"

Show 19 quoted lines

>   	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