| Message ID | d10cb03dd8ce00cb6033c61ff9b9b30bbf5f9c89.1592058281.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | checkout: teach --worktree | expand |
On Sat, Jun 13, 2020 at 10:25 AM Denton Liu <liu.denton@gmail.com> wrote: > [...] > Teach `git checkout --worktree`, allowing users to checkout files > directly into the worktree without affecting the index. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt > @@ -264,6 +266,12 @@ When switching branches with `--merge`, staged changes may be lost. > +-W:: > +--worktree:: > + When writing contents, only modify files in the worktree. Do not > + modify the index. This option is essentially a no-op when used > + without a `<tree-ish>`. Why a no-op rather than actually diagnosing that --worktree makes no sense in that case and erroring out? > diff --git a/t/t2028-checkout-worktree.sh b/t/t2028-checkout-worktree.sh > @@ -0,0 +1,51 @@ > +test_expect_success 'checkout --worktree on a commit' ' > + test_when_finished "git reset --hard tip" && > + git diff HEAD HEAD~ >expect && > + git checkout --worktree HEAD~ file1 && > + git diff >actual && > + test_cmp expect actual && > + git diff --cached --exit-code && Would the intent be clearer if you used 'test_expect_code' here? test_expect_code 0 git diff --cached --exit-code && Same question for remaining tests.
Hi Eric, On Sat, Jun 13, 2020 at 10:51:47PM -0400, Eric Sunshine wrote: > On Sat, Jun 13, 2020 at 10:25 AM Denton Liu <liu.denton@gmail.com> wrote: > > [...] > > Teach `git checkout --worktree`, allowing users to checkout files > > directly into the worktree without affecting the index. > > > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > > --- > > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt > > @@ -264,6 +266,12 @@ When switching branches with `--merge`, staged changes may be lost. > > +-W:: > > +--worktree:: > > + When writing contents, only modify files in the worktree. Do not > > + modify the index. This option is essentially a no-op when used > > + without a `<tree-ish>`. > > Why a no-op rather than actually diagnosing that --worktree makes no > sense in that case and erroring out? I decided on this behaviour because I assumed that an empty `git checkout` has `git restore` behaviour but I guess I was mistaken. I'll change it to error out. > > diff --git a/t/t2028-checkout-worktree.sh b/t/t2028-checkout-worktree.sh > > @@ -0,0 +1,51 @@ > > +test_expect_success 'checkout --worktree on a commit' ' > > + test_when_finished "git reset --hard tip" && > > + git diff HEAD HEAD~ >expect && > > + git checkout --worktree HEAD~ file1 && > > + git diff >actual && > > + test_cmp expect actual && > > + git diff --cached --exit-code && > > Would the intent be clearer if you used 'test_expect_code' here? > > test_expect_code 0 git diff --cached --exit-code && > > Same question for remaining tests. I'm not really sure that this adds anything. When I read through the tests, I already expect each command to be successful, i.e. return 0. I don't see how explicitly documenting for this one command would make that more clear. Looking through the test suite, I only see 15 results of `test_expect_code 0 git diff --exit-code` and all of those are in t4035. Meanwhile, I see at least 234 instances without the `test_expect_code`. I believe that we should leave this as-is. Thanks, Denton
On Sun, Jun 14, 2020 at 03:44:39AM -0400, Denton Liu wrote: > Hi Eric, > > On Sat, Jun 13, 2020 at 10:51:47PM -0400, Eric Sunshine wrote: > > On Sat, Jun 13, 2020 at 10:25 AM Denton Liu <liu.denton@gmail.com> wrote: > > > [...] > > > Teach `git checkout --worktree`, allowing users to checkout files > > > directly into the worktree without affecting the index. > > > > > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > > > --- > > > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt > > > @@ -264,6 +266,12 @@ When switching branches with `--merge`, staged changes may be lost. > > > +-W:: > > > +--worktree:: > > > + When writing contents, only modify files in the worktree. Do not > > > + modify the index. This option is essentially a no-op when used > > > + without a `<tree-ish>`. > > > > Why a no-op rather than actually diagnosing that --worktree makes no > > sense in that case and erroring out? > > I decided on this behaviour because I assumed that an empty > `git checkout` has `git restore` behaviour but I guess I was mistaken. > I'll change it to error out. ...Disregard the above. I misread your comments. I thought about it some more and I think that the real bug is in how I phrased it in the documentation. I meant that --worktree itself was essentially no-op, not the whole checkout operation. I think that it makes sense to allow this behaviour. The documentation states that we only modify files in the worktree. So if we do `git checkout --worktree <path>`, we should overwrite the worktree with the index. This should be exactly the same as running `git checkout <path>`. However, one additonal behaviour I should implement is running `git checkout --worktree` should behave like running `git checkout .`. So this would make --worktree not a no-op without a tree-ish.
Eric Sunshine <sunshine@sunshineco.com> writes: >> + When writing contents, only modify files in the worktree. Do not >> + modify the index. This option is essentially a no-op when used >> + without a `<tree-ish>`. > > Why a no-op rather than actually diagnosing that --worktree makes no > sense in that case and erroring out? Should it be a no-op? If checking paths out of the index, with or without the new --worktree option, the files in the working tree will be affected and the contents in the index won't change.
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt index 5b697eee1b..ba9c0a900c 100644 --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -12,9 +12,9 @@ SYNOPSIS 'git checkout' [-q] [-f] [-m] --detach [<branch>] 'git checkout' [-q] [-f] [-m] [--detach] <commit> 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>] -'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>... -'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul] -'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...] +'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [--worktree] [<tree-ish>] [--] <pathspec>... +'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [--worktree] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul] +'git checkout' (-p|--patch) [--worktree] [<tree-ish>] [--] [<pathspec>...] DESCRIPTION ----------- @@ -79,14 +79,16 @@ be used to detach `HEAD` at the tip of the branch (`git checkout + Omitting `<branch>` detaches `HEAD` at the tip of the current branch. -'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...:: -'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]:: +'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [--worktree] [<tree-ish>] [--] <pathspec>...:: +'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [--worktree] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]:: Overwrite the contents of the files that match the pathspec. When the `<tree-ish>` (most often a commit) is not given, overwrite working tree with the contents in the index. When the `<tree-ish>` is given, overwrite both the index and - the working tree with the contents at the `<tree-ish>`. + the working tree with the contents at the `<tree-ish>` unless + `--worktree` is given in which case _only_ the working tree is + overwritten. + The index may contain unmerged entries because of a previous failed merge. By default, if you try to check out such an entry from the index, the @@ -96,7 +98,7 @@ specific side of the merge can be checked out of the index by using `--ours` or `--theirs`. With `-m`, changes made to the working tree file can be discarded to re-create the original conflicted merge result. -'git checkout' (-p|--patch) [<tree-ish>] [--] [<pathspec>...]:: +'git checkout' (-p|--patch) [--worktree] [<tree-ish>] [--] [<pathspec>...]:: This is similar to the previous mode, but lets you use the interactive interface to show the "diff" output and choose which hunks to use in the result. See below for the description of @@ -264,6 +266,12 @@ When switching branches with `--merge`, staged changes may be lost. "merge" (default) and "diff3" (in addition to what is shown by "merge" style, shows the original contents). +-W:: +--worktree:: + When writing contents, only modify files in the worktree. Do not + modify the index. This option is essentially a no-op when used + without a `<tree-ish>`. + -p:: --patch:: Interactively select hunks in the difference between the diff --git a/builtin/checkout.c b/builtin/checkout.c index af849c644f..a2ade85ad5 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1741,6 +1741,20 @@ static int checkout_main(int argc, const char **argv, const char *prefix, return checkout_branch(opts, &new_branch_info); } +static int handle_worktree_opt(const struct option *opt, const char *arg, int unset) +{ + struct checkout_opts *opts = opt->value; + + BUG_ON_OPT_NEG(unset); + BUG_ON_OPT_ARG(arg); + + opts->checkout_index = 0; + opts->checkout_worktree = 1; + + return 0; +} + + int cmd_checkout(int argc, const char **argv, const char *prefix) { struct checkout_opts opts; @@ -1750,6 +1764,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) N_("create and checkout a new branch")), OPT_STRING('B', NULL, &opts.new_branch_force, N_("branch"), N_("create/reset and checkout a branch")), + OPT_CALLBACK_F('W', "worktree", &opts, NULL, + N_("restore the working tree (default)"), + PARSE_OPT_NOARG | PARSE_OPT_NONEG, handle_worktree_opt), OPT_BOOL('l', NULL, &opts.new_branch_log, N_("create reflog for new branch")), OPT_BOOL(0, "guess", &opts.dwim_new_local_branch, N_("second guess 'git checkout <no-such-branch>' (default)")), diff --git a/t/t2028-checkout-worktree.sh b/t/t2028-checkout-worktree.sh new file mode 100755 index 0000000000..7ba36277c9 --- /dev/null +++ b/t/t2028-checkout-worktree.sh @@ -0,0 +1,51 @@ +#!/bin/sh + +test_description='checkout --worktree' + +. ./test-lib.sh + +test_expect_success setup ' + echo first >file1 && + echo file2 >file2 && + git add file1 file2 && + git commit -m first && + + echo second >file1 && + git commit -am second && + git tag tip +' + +test_expect_success 'checkout --worktree on a commit' ' + test_when_finished "git reset --hard tip" && + git diff HEAD HEAD~ >expect && + git checkout --worktree HEAD~ file1 && + git diff >actual && + test_cmp expect actual && + git diff --cached --exit-code && + test_cmp_rev HEAD tip +' + +test_expect_success 'checkout --worktree with no commit' ' + test_when_finished "git reset --hard tip" && + echo worktree >file1 && + git checkout --worktree file1 && + git diff --exit-code && + test_cmp_rev HEAD tip +' + +test_expect_success 'checkout --no-worktree fails' ' + test_must_fail git checkout --no-worktree +' + +test_expect_success PERL 'git checkout -p --worktree' ' + test_when_finished "git reset --hard tip" && + echo changed >file2 && + git diff -R --src-prefix=b/ --dst-prefix=a/ >expect && + git commit -am file12 && + test_write_lines n y | git checkout --worktree -p HEAD~2 && + git diff >actual && + test_cmp expect actual && + git diff --cached --exit-code +' + +test_done diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 3c44af6940..1db0bb3a31 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1481,6 +1481,7 @@ test_expect_success 'double dash "git checkout"' ' --quiet Z --detach Z --track Z + --worktree Z --orphan=Z --ours Z --theirs Z
A complaint that has come up frequently in the past is that it is not possible to checkout files directly into a worktree without modifying the index. Even though this could be worked around by redirecting the output of `git show` to overwrite files, this was not feasible if one wanted to use patch mode. Since `git restore` was implemented, this has since been possible using the `--worktree` option. However, some long-time users of Git still prefer to use `git checkout` over `git restore` and would like to see the functionality ported over. Teach `git checkout --worktree`, allowing users to checkout files directly into the worktree without affecting the index. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Documentation/git-checkout.txt | 22 ++++++++++----- builtin/checkout.c | 17 ++++++++++++ t/t2028-checkout-worktree.sh | 51 ++++++++++++++++++++++++++++++++++ t/t9902-completion.sh | 1 + 4 files changed, 84 insertions(+), 7 deletions(-) create mode 100755 t/t2028-checkout-worktree.sh