Denton Liu <liu.denton@xxxxxxxxx> 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.
