| Message ID | 54f221411f4ec60a88521c376a1c77fa0a3e7553.1601814601.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] builtin/checkout: fix `git checkout -p HEAD...` bug | expand |
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.
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 &&
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(-)