git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] builtin/checkout: fix `git checkout -p HEAD...` bug
Date: Sun, 04 Oct 2020 11:02:52 -0700	[thread overview]
Message-ID: <xmqqsgat7ttf.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <54f221411f4ec60a88521c376a1c77fa0a3e7553.1601814601.git.liu.denton@gmail.com> (Denton Liu's message of "Sun, 4 Oct 2020 05:30:33 -0700")

Denton Liu <liu.denton@gmail.com> writes:

> Fix this by using the hex of the found commit instead of the given name.
> Note that "HEAD" is handled specially in run_add_interactive() so it's
> explicitly not changed.
> ...
> +		const char *rev = new_branch_info->name;
> +		char rev_oid[GIT_MAX_HEXSZ + 1];
> +
> +		/*
> +		 * Since rev can be in the form of `<a>...<b>`, we must replace
> +		 * the name with the hex of the commit for the
> +		 * run_add_interactive() machinery to work properly. However,
> +		 * there is special logic for the HEAD case so we mustn't
> +		 * replace that.
> +		 */
> +		if (rev && strcmp(rev, "HEAD"))
> +			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);

What the comment explains and the if condition special cases are
different.  Surely, HEAD is treated differently, but the natural
implementation of what the comment wants to achieve would be to not
just make sure it is not HEAD, but to make sure that it is of <a>...<b>
form, e.g.

		if (rev && strstr(rev, "..."))

Having said that, I do like how your version looks like in two ways,
i.e. it makes it clear HEAD is special, and it does not limit the
special case to only the three-dot "merge-base" magic, which might
be more futureproof.  On the other hand, your version would give the
hexadecimal commit object name even when a plain-vanilla branch name
is given and the above comment does not say if and why it is a safe
thing to do.  We used to give "new_branch_info->name" to the helper
that invokes "add -p".  Now we always give a hexadecimal or "HEAD".

Come to think of it, perhaps "add -p" that special cases HEAD is a
mistake.  If a plain vanilla branch name can be safely replaced by
the hexadecimal commit object name, perhaps the code should be
prepared to special case not just string "HEAD" but any other way to
express the commit object referred to with "HEAD" the same way.  But
that is not a suggestion for this patch---it may be a good idea to
add NEEDSWORK: comment here to encourage our future selves to see if
"add -p" needs to be fixed.

>  		if (opts->checkout_index && opts->checkout_worktree)
>  			patch_mode = "--patch=checkout";
> @@ -481,7 +493,7 @@ static int checkout_paths(const struct checkout_opts *opts,
>  		else
>  			BUG("either flag must have been set, worktree=%d, index=%d",
>  			    opts->checkout_worktree, opts->checkout_index);
> -		return run_add_interactive(new_branch_info->name, patch_mode, &opts->pathspec);
> +		return run_add_interactive(rev, patch_mode, &opts->pathspec);
>  	}
>  
>  	repo_hold_locked_index(the_repository, &lock_file, LOCK_DIE_ON_ERROR);
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 47aeb0b167..999620e507 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -59,6 +59,13 @@ test_expect_success PERL 'git checkout -p HEAD with change already staged' '
>  	verify_state dir/foo head head
>  '
>  
> +test_expect_success PERL 'git checkout -p HEAD^...' '
> +	# the third n is to get out in case it mistakenly does not apply
> +	test_write_lines n y n | git checkout -p HEAD^... &&
> +	verify_saved_state bar &&
> +	verify_state dir/foo parent parent
> +'
> +
>  test_expect_success PERL 'git checkout -p HEAD^' '
>  	# the third n is to get out in case it mistakenly does not apply
>  	test_write_lines n y n | git checkout -p HEAD^ &&
> diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
> index 98b2476e7c..b5c5c0ff7e 100755
> --- a/t/t2071-restore-patch.sh
> +++ b/t/t2071-restore-patch.sh
> @@ -60,6 +60,14 @@ test_expect_success PERL 'git restore -p --source=HEAD^' '
>  	verify_state dir/foo parent 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 parent index
> +'
> +
>  test_expect_success PERL 'git restore -p handles deletion' '
>  	set_state dir/foo work index &&
>  	rm dir/foo &&

You are just mimicking what is already there, but I thought "add -p"
can use built-in replacement when GIT_TEST_ADD_I_USE_BUILTIN is
given these days.  Perhaps we need to replace these PERL
prerequisite with a new one, say ADD_I, which requires PERL if and
only if GIT_TEST_ADD_I_USE_BUILTIN is not set, or something.  Again,
that is not in the scope of this patch, but leaving NEEDSWORK:
comment in these test scripts where they first use PERL prerequisite
but they would use ADD_I prerequisite if available.


  reply	other threads:[~2020-10-04 18:04 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-04 12:30 [PATCH 0/2] checkout/restore: " Denton Liu
2020-10-04 12:30 ` [PATCH 1/2] builtin/checkout: " Denton Liu
2020-10-04 18:02   ` Junio C Hamano [this message]
2020-10-04 12:30 ` [PATCH 2/2] Doc: document "A...B" form for <tree-ish> in checkout and switch Denton Liu
2020-10-07  7:56 ` [PATCH v2 0/4] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
2020-10-07  7:56   ` [PATCH v2 1/4] builtin/checkout: " Denton Liu
2020-10-07  7:56   ` [PATCH v2 2/4] Doc: document "A...B" form for <tree-ish> in checkout and switch Denton Liu
2020-10-07  7:56   ` [PATCH v2 3/4] add-patch: add NEEDSWORK about comparing commits Denton Liu
2020-10-07  7:56   ` [PATCH v2 4/4] t2016: add a NEEDSWORK about the PERL prerequisite Denton Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsgat7ttf.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).