git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] checkout/restore: fix `git checkout -p HEAD...` bug
@ 2020-10-04 12:30 Denton Liu
  2020-10-04 12:30 ` [PATCH 1/2] builtin/checkout: " Denton Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Denton Liu @ 2020-10-04 12:30 UTC (permalink / raw)
  To: Git Mailing List

As reported earlier[0], `git checkout -p HEAD...` results in the following error:

	$ git checkout -p HEAD...
	usage: git diff-index [-m] [--cached] [<common-diff-options>] <tree-ish> [<path>...]
	common diff options:
	  -z            output diff-raw with lines terminated with NUL.
	  -p            output patch format.
	  -u            synonym for -p.
	  --patch-with-raw
			output both a patch and the diff-raw format.
	  --stat        show diffstat instead of patch.
	  --numstat     show numeric diffstat instead of patch.
	  --patch-with-stat
			output a patch and prepend its diffstat.
	  --name-only   show only names of changed files.
	  --name-status show names and status of changed files.
	  --full-index  show full object name on index lines.
	  --abbrev=<n>  abbreviate object names in diff-tree header and diff-raw.
	  -R            swap input file pairs.
	  -B            detect complete rewrites.
	  -M            detect renames.
	  -C            detect copies.
	  --find-copies-harder
			try unchanged files as candidate for copy detection.
	  -l<n>         limit rename attempts up to <n> paths.
	  -O<file>      reorder diffs according to the <file>.
	  -S<string>    find filepair whose only one side contains the string.
	  --pickaxe-all
			show all files diff when -S is used and hit is found.
	  -a  --text    treat all files as text.

	Cannot close git diff-index --cached --numstat --summary HEAD... -- () at <redacted>/libexec/git-core/git-add--interactive line 183.

Since it was decided that <tree-ish> should also officially support the
merge-base revs, fix `git checkout -p <rev>...` and officially document the
support.

[0]: https://lore.kernel.org/git/20200929061830.GA40759@generichostname/

Denton Liu (2):
  builtin/checkout: fix `git checkout -p HEAD...` bug
  Doc: document "A...B" form for <tree-ish> in checkout and switch

 Documentation/git-checkout.txt |  4 ++++
 Documentation/git-restore.txt  |  4 ++++
 builtin/checkout.c             | 14 +++++++++++++-
 t/t2016-checkout-patch.sh      |  7 +++++++
 t/t2071-restore-patch.sh       |  8 ++++++++
 5 files changed, 36 insertions(+), 1 deletion(-)

-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] builtin/checkout: fix `git checkout -p HEAD...` bug
  2020-10-04 12:30 [PATCH 0/2] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
@ 2020-10-04 12:30 ` Denton Liu
  2020-10-04 18:02   ` Junio C Hamano
  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
  2 siblings, 1 reply; 9+ messages in thread
From: Denton Liu @ 2020-10-04 12:30 UTC (permalink / raw)
  To: Git Mailing List

Running `git checkout -p` with a merge-base rev results in an error:

	$ git checkout -p HEAD...
	usage: git diff-index [-m] [--cached] [<common-diff-options>] <tree-ish> [<path>...]
	common diff options:
	  -z            output diff-raw with lines terminated with NUL.
	  -p            output patch format.
	  -u            synonym for -p.
	  --patch-with-raw
			output both a patch and the diff-raw format.
	  --stat        show diffstat instead of patch.
	  --numstat     show numeric diffstat instead of patch.
	  --patch-with-stat
			output a patch and prepend its diffstat.
	  --name-only   show only names of changed files.
	  --name-status show names and status of changed files.
	  --full-index  show full object name on index lines.
	  --abbrev=<n>  abbreviate object names in diff-tree header and diff-raw.
	  -R            swap input file pairs.
	  -B            detect complete rewrites.
	  -M            detect renames.
	  -C            detect copies.
	  --find-copies-harder
			try unchanged files as candidate for copy detection.
	  -l<n>         limit rename attempts up to <n> paths.
	  -O<file>      reorder diffs according to the <file>.
	  -S<string>    find filepair whose only one side contains the string.
	  --pickaxe-all
			show all files diff when -S is used and hit is found.
	  -a  --text    treat all files as text.

	Cannot close git diff-index --cached --numstat --summary HEAD... -- () at <redacted>/libexec/git-core/git-add--interactive line 183.

This happens because checkout passes the literal argument (in the
example, `HEAD...`) to diff-index which does not recognise merge-base
revs.

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/checkout.c        | 14 +++++++++++++-
 t/t2016-checkout-patch.sh |  7 +++++++
 t/t2071-restore-patch.sh  |  8 ++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0951f8fee5..95122e16ed 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -471,6 +471,18 @@ static int checkout_paths(const struct checkout_opts *opts,
 
 	if (opts->patch_mode) {
 		const char *patch_mode;
+		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);
 
 		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 &&
-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] Doc: document "A...B" form for <tree-ish> in checkout and switch
  2020-10-04 12:30 [PATCH 0/2] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
  2020-10-04 12:30 ` [PATCH 1/2] builtin/checkout: " Denton Liu
@ 2020-10-04 12:30 ` Denton Liu
  2020-10-07  7:56 ` [PATCH v2 0/4] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
  2 siblings, 0 replies; 9+ messages in thread
From: Denton Liu @ 2020-10-04 12:30 UTC (permalink / raw)
  To: Git Mailing List

Using "A...B" has been supported for the <tree-ish> argument for a
while. However, its support has never been explicitly documented.

Explicitly document it so that users know that it is available.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-checkout.txt | 4 ++++
 Documentation/git-restore.txt  | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index afa5c11fd3..6ae8bf8dee 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -351,6 +351,10 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 <tree-ish>::
 	Tree to checkout from (when paths are given). If not specified,
 	the index will be used.
++
+As a special case, you may use `"A...B"` as a shortcut for the
+merge base of `A` and `B` if there is exactly one merge base. You can
+leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
 \--::
 	Do not interpret any more arguments as options.
diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
index 84c6c40010..55bde91ef9 100644
--- a/Documentation/git-restore.txt
+++ b/Documentation/git-restore.txt
@@ -40,6 +40,10 @@ OPTIONS
 +
 If not specified, the contents are restored from `HEAD` if `--staged` is
 given, otherwise from the index.
++
+As a special case, you may use `"A...B"` as a shortcut for the
+merge base of `A` and `B` if there is exactly one merge base. You can
+leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
 -p::
 --patch::
-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] builtin/checkout: fix `git checkout -p HEAD...` bug
  2020-10-04 12:30 ` [PATCH 1/2] builtin/checkout: " Denton Liu
@ 2020-10-04 18:02   ` Junio C Hamano
  0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2020-10-04 18:02 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 0/4] checkout/restore: fix `git checkout -p HEAD...` bug
  2020-10-04 12:30 [PATCH 0/2] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
  2020-10-04 12:30 ` [PATCH 1/2] builtin/checkout: " Denton Liu
  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 ` Denton Liu
  2020-10-07  7:56   ` [PATCH v2 1/4] builtin/checkout: " Denton Liu
                     ` (3 more replies)
  2 siblings, 4 replies; 9+ messages in thread
From: Denton Liu @ 2020-10-07  7:56 UTC (permalink / raw)
  To: Git Mailing List

As reported earlier[0], `git checkout -p HEAD...` results in the following error:

	$ git checkout -p HEAD...
	usage: git diff-index [-m] [--cached] [<common-diff-options>] <tree-ish> [<path>...]
	common diff options:
	  -z            output diff-raw with lines terminated with NUL.
	  -p            output patch format.
	  -u            synonym for -p.
	  --patch-with-raw
			output both a patch and the diff-raw format.
	  --stat        show diffstat instead of patch.
	  --numstat     show numeric diffstat instead of patch.
	  --patch-with-stat
			output a patch and prepend its diffstat.
	  --name-only   show only names of changed files.
	  --name-status show names and status of changed files.
	  --full-index  show full object name on index lines.
	  --abbrev=<n>  abbreviate object names in diff-tree header and diff-raw.
	  -R            swap input file pairs.
	  -B            detect complete rewrites.
	  -M            detect renames.
	  -C            detect copies.
	  --find-copies-harder
			try unchanged files as candidate for copy detection.
	  -l<n>         limit rename attempts up to <n> paths.
	  -O<file>      reorder diffs according to the <file>.
	  -S<string>    find filepair whose only one side contains the string.
	  --pickaxe-all
			show all files diff when -S is used and hit is found.
	  -a  --text    treat all files as text.

	Cannot close git diff-index --cached --numstat --summary HEAD... -- () at <redacted>/libexec/git-core/git-add--interactive line 183.

Since it was decided that <tree-ish> should also officially support the
merge-base revs, fix `git checkout -p <rev>...` and officially document the
support.

Changes since v1:

* Add two NEEDSWORK commits

* Be more precise in the inline code comment

[0]: https://lore.kernel.org/git/20200929061830.GA40759@generichostname/

Denton Liu (4):
  builtin/checkout: fix `git checkout -p HEAD...` bug
  Doc: document "A...B" form for <tree-ish> in checkout and switch
  add-patch: add NEEDSWORK about comparing commits
  t2016: add a NEEDSWORK about the PERL prerequisite

 Documentation/git-checkout.txt |  4 ++++
 Documentation/git-restore.txt  |  4 ++++
 add-patch.c                    |  8 ++++++++
 builtin/checkout.c             | 15 ++++++++++++++-
 git-add--interactive.perl      |  7 +++++++
 t/t2016-checkout-patch.sh      | 11 +++++++++++
 t/t2071-restore-patch.sh       |  8 ++++++++
 7 files changed, 56 insertions(+), 1 deletion(-)

Range-diff against v1:
1:  54f221411f ! 1:  723d74febe builtin/checkout: fix `git checkout -p HEAD...` bug
    @@ builtin/checkout.c: static int checkout_paths(const struct checkout_opts *opts,
     +		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.
    ++		 * Since rev can be in the form of `<a>...<b>` (which is not
    ++		 * recognized by diff-index), we will always replace the name
    ++		 * with the hex of the commit (whether it's in `...` form or
    ++		 * not) 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);
2:  75920c1a25 = 2:  a0ed2f313d Doc: document "A...B" form for <tree-ish> in checkout and switch
-:  ---------- > 3:  44ee78450e add-patch: add NEEDSWORK about comparing commits
-:  ---------- > 4:  a20544f18a t2016: add a NEEDSWORK about the PERL prerequisite
-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/4] builtin/checkout: fix `git checkout -p HEAD...` bug
  2020-10-07  7:56 ` [PATCH v2 0/4] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
@ 2020-10-07  7:56   ` 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
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Denton Liu @ 2020-10-07  7:56 UTC (permalink / raw)
  To: Git Mailing List

Running `git checkout -p` with a merge-base rev results in an error:

	$ git checkout -p HEAD...
	usage: git diff-index [-m] [--cached] [<common-diff-options>] <tree-ish> [<path>...]
	common diff options:
	  -z            output diff-raw with lines terminated with NUL.
	  -p            output patch format.
	  -u            synonym for -p.
	  --patch-with-raw
			output both a patch and the diff-raw format.
	  --stat        show diffstat instead of patch.
	  --numstat     show numeric diffstat instead of patch.
	  --patch-with-stat
			output a patch and prepend its diffstat.
	  --name-only   show only names of changed files.
	  --name-status show names and status of changed files.
	  --full-index  show full object name on index lines.
	  --abbrev=<n>  abbreviate object names in diff-tree header and diff-raw.
	  -R            swap input file pairs.
	  -B            detect complete rewrites.
	  -M            detect renames.
	  -C            detect copies.
	  --find-copies-harder
			try unchanged files as candidate for copy detection.
	  -l<n>         limit rename attempts up to <n> paths.
	  -O<file>      reorder diffs according to the <file>.
	  -S<string>    find filepair whose only one side contains the string.
	  --pickaxe-all
			show all files diff when -S is used and hit is found.
	  -a  --text    treat all files as text.

	Cannot close git diff-index --cached --numstat --summary HEAD... -- () at <redacted>/libexec/git-core/git-add--interactive line 183.

This happens because checkout passes the literal argument (in the
example, `HEAD...`) to diff-index which does not recognise merge-base
revs.

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/checkout.c        | 15 ++++++++++++++-
 t/t2016-checkout-patch.sh |  7 +++++++
 t/t2071-restore-patch.sh  |  8 ++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0951f8fee5..2e69ed24f2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -471,6 +471,19 @@ static int checkout_paths(const struct checkout_opts *opts,
 
 	if (opts->patch_mode) {
 		const char *patch_mode;
+		const char *rev = new_branch_info->name;
+		char rev_oid[GIT_MAX_HEXSZ + 1];
+
+		/*
+		 * Since rev can be in the form of `<a>...<b>` (which is not
+		 * recognized by diff-index), we will always replace the name
+		 * with the hex of the commit (whether it's in `...` form or
+		 * not) 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);
 
 		if (opts->checkout_index && opts->checkout_worktree)
 			patch_mode = "--patch=checkout";
@@ -481,7 +494,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 &&
-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/4] Doc: document "A...B" form for <tree-ish> in checkout and switch
  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   ` 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
  3 siblings, 0 replies; 9+ messages in thread
From: Denton Liu @ 2020-10-07  7:56 UTC (permalink / raw)
  To: Git Mailing List

Using "A...B" has been supported for the <tree-ish> argument for a
while. However, its support has never been explicitly documented.

Explicitly document it so that users know that it is available.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-checkout.txt | 4 ++++
 Documentation/git-restore.txt  | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index afa5c11fd3..6ae8bf8dee 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -351,6 +351,10 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 <tree-ish>::
 	Tree to checkout from (when paths are given). If not specified,
 	the index will be used.
++
+As a special case, you may use `"A...B"` as a shortcut for the
+merge base of `A` and `B` if there is exactly one merge base. You can
+leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
 \--::
 	Do not interpret any more arguments as options.
diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt
index 84c6c40010..55bde91ef9 100644
--- a/Documentation/git-restore.txt
+++ b/Documentation/git-restore.txt
@@ -40,6 +40,10 @@ OPTIONS
 +
 If not specified, the contents are restored from `HEAD` if `--staged` is
 given, otherwise from the index.
++
+As a special case, you may use `"A...B"` as a shortcut for the
+merge base of `A` and `B` if there is exactly one merge base. You can
+leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
 -p::
 --patch::
-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/4] add-patch: add NEEDSWORK about comparing commits
  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   ` Denton Liu
  2020-10-07  7:56   ` [PATCH v2 4/4] t2016: add a NEEDSWORK about the PERL prerequisite Denton Liu
  3 siblings, 0 replies; 9+ messages in thread
From: Denton Liu @ 2020-10-07  7:56 UTC (permalink / raw)
  To: Git Mailing List

The two versions of add-patch has special-casing for the literal
revision "HEAD". However, we want to handle other ways of saying "HEAD"
in the same way.[0] Add a NEEDSWORK to the add-patch code that does this
so that it can be addressed later.

[0]: https://lore.kernel.org/git/xmqqsgat7ttf.fsf@gitster.c.googlers.com/

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 add-patch.c               | 8 ++++++++
 git-add--interactive.perl | 7 +++++++
 2 files changed, 15 insertions(+)

diff --git a/add-patch.c b/add-patch.c
index bd94bd3a7c..be4cf6e9e5 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1695,6 +1695,14 @@ 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
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8a72018712..e713fe3d02 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1830,6 +1830,13 @@ sub process_args {
 				$arg = shift @ARGV or die __("missing --");
 				if ($arg ne '--') {
 					$patch_mode_revision = $arg;
+
+					# 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.
 					$patch_mode = ($arg eq 'HEAD' ?
 						       'reset_head' : 'reset_nothead');
 					$arg = shift @ARGV or die __("missing --");
-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 4/4] t2016: add a NEEDSWORK about the PERL prerequisite
  2020-10-07  7:56 ` [PATCH v2 0/4] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
                     ` (2 preceding siblings ...)
  2020-10-07  7:56   ` [PATCH v2 3/4] add-patch: add NEEDSWORK about comparing commits Denton Liu
@ 2020-10-07  7:56   ` Denton Liu
  3 siblings, 0 replies; 9+ messages in thread
From: Denton Liu @ 2020-10-07  7:56 UTC (permalink / raw)
  To: Git Mailing List

Since the builtin add-p is used when $GIT_TEST_ADD_I_USE_BUILTIN is
given, we should replace the PERL prerequisite with an ADD_I
prerequisite which first checks if $GIT_TEST_ADD_I_USE_BUILTIN is
defined before checking PERL.[0] Mark this in a NEEDSWORK so that it can
be addressed at a later time.

[0]: https://lore.kernel.org/git/xmqqsgat7ttf.fsf@gitster.c.googlers.com/

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2016-checkout-patch.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 999620e507..d91a329eb3 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -18,6 +18,10 @@ test_expect_success PERL 'setup' '
 
 # note: bar sorts before dir/foo, so the first 'n' is always to skip 'bar'
 
+# NEEDSWORK: Since the builtin add-p is used when $GIT_TEST_ADD_I_USE_BUILTIN
+# is given, we should replace the PERL prerequisite with an ADD_I prerequisite
+# which first checks if $GIT_TEST_ADD_I_USE_BUILTIN is defined before checking
+# PERL.
 test_expect_success PERL 'saying "n" does nothing' '
 	set_and_save_state dir/foo work head &&
 	test_write_lines n n | git checkout -p &&
-- 
2.28.0.942.g77c4c6094c


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-10-07  7:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-04 12:30 [PATCH 0/2] checkout/restore: fix `git checkout -p HEAD...` bug Denton Liu
2020-10-04 12:30 ` [PATCH 1/2] builtin/checkout: " Denton Liu
2020-10-04 18:02   ` Junio C Hamano
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

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).