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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2024, 23:07 UTC
Message-ID: <xmqq34u6o6hs.fsf@gitster.g>
In-Reply-To: <9f9f26f1-5460-468e-a893-5caf7fbea981@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 14 quoted lines

> 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 actually think "@" in the input should be normalized to "HEAD" as soon as possible. After the if/elseif/ cascade in run_add_p() the patch in this thread touches, there is an assignment

	s.revision = revision;

and even though we rewrite !strcmp(revision, "HEAD") to "user means HEAD" to additionally accept "@" in that if/elseif/ cascade, here we will stuff different values to s.revision here. We could normalize the end-user supplied "@" to "HEAD" before making this assignment, then you do not have to worry about the code in parse_diff() above, and more importantly, we do not have to rely on what the current set of callers happen to do and do not happen to do (e.g., "git reset -p" happens to use hardcoded "HEAD" for unborn case without using anything obtained from the user, so the above code in parse_diff() might be safe in that case, but we do not want to rely on such subtlety to make sure our code is correct)

Come to think of it, we could even do the "normalizing" even before, and that might greatly simplify things. For example, if we did so at the very beginning of run_add_p(), before we come to that if/else if/ cascade, we may not even have to worry about the "user meant HEAD" helper. After the normalization, we can just keep using !strcmp() with "HEAD" alone. Simple and clean, no?

Thanks.