Re: [PATCH v4 1/3] add-patch: remove unnecessary NEEDSWORK comment

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ghanshyam

On 06/02/2024 22:50, Ghanshyam Thakkar wrote:
> The comment suggested to compare commit objects instead of string
> comparison to 'HEAD' for supporting more ways of saying 'HEAD' (e.g.
> '@'). However, this approach would also count a non-checked out branch
> pointing to same commit as HEAD, as HEAD. This would cause confusion to
> the user.

I forgot to say before, but I think this could be squashed into the next 
patch so we remove the comment at the same time as fixing the issue it 
describes.

Best Wishes

Phillip

> Junio described it best as[1]:
> 
> "Users may consider 'HEAD' and '@' the same and
> may want them to behave the same way, but the user, when explicitly
> naming '$branch', means they want to "check contents out of that
> OTHER thing named '$branch', not the current branch"; it may or
> may not happen to be pointing at the same commit as HEAD, but if
> the user meant to say "check contents out of the current commit,
> (partially) reverting the local changes I have", the user would have
> said HEAD.  After all, the user may not even be immediately aware
> that '$branch' happens to point at the same commit as HEAD."
> 
> [1]: https://lore.kernel.org/git/xmqqmssohu69.fsf@gitster.g/
> 
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>   add-patch.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 79eda168eb..68f525b35c 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>   	if (mode == ADD_P_STASH)
>   		s.mode = &patch_mode_stash;
>   	else if (mode == ADD_P_RESET) {
> -		/*
> -		 * NEEDSWORK: Instead of comparing to the literal "HEAD",
> -		 * compare the commit objects instead so that other ways of
> -		 * saying the same thing (such as "@") are also handled
> -		 * appropriately.
> -		 *
> -		 * This applies to the cases below too.
> -		 */
>   		if (!revision || !strcmp(revision, "HEAD"))
>   			s.mode = &patch_mode_reset_head;
>   		else