From: Phillip Wood Date: Wed, 07 Feb 2024 10:51:28 GMT Subject: Re: [PATCH v4 1/3] add-patch: remove unnecessary NEEDSWORK comment Message-ID: <99fcba4a-a4f7-4d72-8238-09d9bf7851ce@gmail.com> In-Reply-To: <20240206225122.1095766-4-shyamthakkar001@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 > --- > 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