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

From: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Date: Feb 2, 2024, 17:51 UTC
Message-ID: <CYUS8RWV9JY6.15PV2RSRKOXMP@gmail.com>
In-Reply-To: <xmqqh6iqu73y.fsf@gitster.g>

On Fri Feb 2, 2024 at 10:38 PM IST, Junio C Hamano wrote:

Show 19 quoted lines

> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > 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 some logic to convert all revs besides
> > 'HEAD' to hex revs due to 'diff-index', which is used in patch mode
> > machinery, not being able to handle '<a>...<b>' revs. Therefore, in
> > addition to 'HEAD', do not convert '@' as well, so it can be later
> > used in assigning patch_mode_(...)_head.
>
> In this context <a>...<b> names a single rev (not two revs) that is
> the merge base of <a> and <b>.  Perhaps

I meant revs which are spelled out in the form of <a>...<b> and not the <a> and <b> themselves. But I can see how it can be confusing. I will change it.

Show 7 quoted lines

>     ... 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, too.
>
> which may be a bit shorter.

Yeah, that is much better and clearer also.

> You decided to use is_rev_head() instead of user_meant_head(), so
> you'd need to update the above description to match, I think.

Will update.

Show 9 quoted lines

> > -		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
> > +		if (rev && new_branch_info->commit && strcmp(rev, "HEAD") &&
> > +		    strcmp(rev, "@"))
>
> Shouldn't this be
>
> 		if (rev && new_branch_info->commit && !is_rev_head(rev))
>
> instead of "HEAD" and "@" spelled out?

is_rev_head() is in add-patch.c and the above is in builtin/checkout.c and is_rev_head() is not exported. I can also define it in builtin/checkout.c, but this would be the only instance in that file which will use it. So, I think it is better to just add strcmp(rev, "@") to the if condition.