| Message ID | 20181209200449.16342-7-t.gummerer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce no-overlay and cached mode in git checkout | expand |
On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > Add a new --cached option to git checkout, which works only on the > index, but not the working tree, similar to what 'git reset <tree-ish> > -- <pathspec>... does. Elijah wanted another mode (and I agree) that modifies worktree but leaves the index alone. This is most useful (or least confusing) when used with <tree-ish> and would be default in restore-files. I'm not saying you have to implement it, but how do the new command line options are designed to make sense? I guess if --cached is "update index only" then --no-cached goes back to the default "update both worktree and index" and we need a another option for "worktree only"? Can we have one option with three possible values (index-only, index-and-worktree, worktree-only) maybe?
On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > Add a new --cached option to git checkout, which works only on the > index, but not the working tree, similar to what 'git reset <tree-ish> > -- <pathspec>... does. Indeed the tests are adapted from the 'git > reset' tests. > > In the longer term the idea is to potentially deprecate 'git reset > <tree-ish> -- <pathspec>...', so the 'git reset' command becomes only > about re-pointing the HEAD, and not also about copying entries from > <tree-ish> to the index. > > Note that 'git checkout' by default works in overlay mode, meaning > files that match the pathspec that don't exist in <tree-ish>, but > exist in the index would not be removed. 'git checkout --no-overlay > --cached' can be used to get the same behaviour as 'git reset > <tree-ish> -- <pathspec>'. I think this argues _even more_ that --no-overlay should be the default. Your series is valuable even if we don't push on that, I'm just being noisy about what I think would be an even better world. Also, I don't think I've mentioned it yet, but I'm really excited about this series and what you're doing. It's super cool. (Which I expected when I saw the description of the desired behavior, but I'm also liking and contemplating re-using some code...) > One thing this patch doesn't currently deal with is conflicts. > Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>' > doesn't do anything with the index, so the --cached option just > mirrors that behaviour. But given it doesn't even deal with > conflicts, the '--cached' option doesn't make much sense when no > <tree-ish> is given. As it operates only on the index, it's always a > no-op if no tree-ish is given. > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > > Maybe we can just disallow --cached without <tree-ish> given for now, > and possibly later allow it with some different behaviour for > conflicts, not sure what the best way forward here is. We can also > just make it update the index as appropriate, and have it behave > different than 'git checkout' curerntly does when handling conflicts? Huh? "git checkout -- <path>" means update <path> from the index, meaning the index is left alone (it's the source) and only the working tree is touched. When you add a flag named --cached to only update the index and not the working tree, then the index becomes the sole destination. Now we combine: no tree is specified means the index is the source of the writing, and --cached being specified means the index is the sole destination of the writing. Thus, you have a no-op. If the user specifies --cached and no tree, you should immediately exit with a message along the lines of "Nothing to do; no tree given and --cached specified." The presence of conflicts seems completely irrelevant to me here. > builtin/checkout.c | 26 ++++++++-- > t/t2016-checkout-patch.sh | 8 +++ > t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++ > t/t9902-completion.sh | 1 + > 4 files changed, 135 insertions(+), 3 deletions(-) > create mode 100755 t/t2026-checkout-cached.sh > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 0aef35bbc4..6ba85e9de5 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -45,6 +45,7 @@ struct checkout_opts { > int ignore_other_worktrees; > int show_progress; > int overlay_mode; > + int cached; > /* > * If new checkout options are added, skip_merge_working_tree > * should be updated accordingly. > @@ -288,6 +289,10 @@ static int checkout_paths(const struct checkout_opts *opts, > die(_("Cannot update paths and switch to branch '%s' at the same time."), > opts->new_branch); > > + if (opts->patch_mode && opts->cached) > + return run_add_interactive(revision, "--patch=reset", > + &opts->pathspec); > + > if (opts->patch_mode) > return run_add_interactive(revision, "--patch=checkout", > &opts->pathspec); > @@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts, > * the current index, which means that it should > * be removed. > */ > - ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE; > + ce->ce_flags |= CE_MATCHED | CE_REMOVE; > + if (!opts->cached) > + ce->ce_flags |= CE_WT_REMOVE; > continue; > } else { > /* > @@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts, > for (pos = 0; pos < active_nr; pos++) { > struct cache_entry *ce = active_cache[pos]; > if (ce->ce_flags & CE_MATCHED) { > + if (opts->cached) { > + continue; > + } > if (!ce_stage(ce)) { > errs |= checkout_entry(ce, &state, NULL); > continue; > @@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts, > * not tested here > */ > > + /* > + * opts->cached cannot be used with switching branches so is > + * not tested here > + */ > + > /* > * If we aren't creating a new branch any changes or updates will > * happen in the existing branch. Since that could only be updating > @@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts, > die(_("'%s' cannot be used with switching branches"), > "--patch"); > > - if (!opts->overlay_mode) > + if (opts->overlay_mode != -1) > + die(_("'%s' cannot be used with switching branches"), > + "--overlay/--no-overlay"); > + > + if (opts->cached) > die(_("'%s' cannot be used with switching branches"), > - "--no-overlay"); > + "--cached"); > > if (opts->writeout_stage) > die(_("'%s' cannot be used with switching branches"), > @@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater }, > OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")), > OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")), > + OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")), > OPT_END(), > }; > > diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh > index 47aeb0b167..e8774046e0 100755 > --- a/t/t2016-checkout-patch.sh > +++ b/t/t2016-checkout-patch.sh > @@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' ' > verify_state dir/foo head head > ' > > +test_expect_success PERL 'git checkout --cached -p' ' > + set_and_save_state dir/foo work work && > + test_write_lines n y | git checkout --cached -p >output && > + verify_state dir/foo work head && > + verify_saved_state bar && > + test_i18ngrep "Unstage" output > +' > + > test_expect_success PERL 'none of this moved HEAD' ' > verify_saved_head > ' > diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh > new file mode 100755 > index 0000000000..1b66192727 > --- /dev/null > +++ b/t/t2026-checkout-cached.sh > @@ -0,0 +1,103 @@ > +#!/bin/sh > + > +test_description='checkout --cached <pathspec>' > + > +. ./test-lib.sh > + > +test_expect_success 'checkout --cached <pathspec>' ' > + echo 1 >file1 && > + echo 2 >file2 && > + git add file1 file2 && > + test_tick && > + git commit -m files && > + git rm file2 && > + echo 3 >file3 && > + echo 4 >file1 && > + git add file1 file3 && > + git checkout --cached HEAD -- file1 file2 && > + test_must_fail git diff --quiet && > + > + cat >expect <<-\EOF && > + diff --git a/file1 b/file1 > + index d00491f..b8626c4 100644 > + --- a/file1 > + +++ b/file1 > + @@ -1 +1 @@ > + -1 > + +4 > + diff --git a/file2 b/file2 > + deleted file mode 100644 > + index 0cfbf08..0000000 > + --- a/file2 > + +++ /dev/null > + @@ -1 +0,0 @@ > + -2 > + EOF > + git diff >actual && > + test_cmp expect actual && > + > + cat >expect <<-\EOF && > + diff --git a/file3 b/file3 > + new file mode 100644 > + index 0000000..00750ed > + --- /dev/null > + +++ b/file3 > + @@ -0,0 +1 @@ > + +3 > + EOF > + git diff --cached >actual && > + test_cmp expect actual > +' > + > +test_expect_success 'checking out an unmodified path is a no-op' ' > + git reset --hard && > + git checkout --cached HEAD -- file1 && > + git diff-files --exit-code && > + git diff-index --cached --exit-code HEAD > +' > + > +test_expect_success 'checking out specific path that is unmerged' ' > + test_commit file3 file3 && > + git rm --cached file2 && > + echo 1234 >file2 && > + F1=$(git rev-parse HEAD:file1) && > + F2=$(git rev-parse HEAD:file2) && > + F3=$(git rev-parse HEAD:file3) && > + { > + echo "100644 $F1 1 file2" && > + echo "100644 $F2 2 file2" && > + echo "100644 $F3 3 file2" > + } | git update-index --index-info && > + git ls-files -u && > + git checkout --cached HEAD file2 && > + test_must_fail git diff --quiet && > + git diff-index --exit-code --cached HEAD > +' > + > +test_expect_success '--cached without --no-overlay does not remove entry from index' ' > + test_must_fail git checkout --cached HEAD^ file3 && > + git ls-files --error-unmatch -- file3 > +' > + > +test_expect_success 'file is removed from the index with --no-overlay' ' > + git checkout --cached --no-overlay HEAD^ file3 && > + test_path_is_file file3 && > + test_must_fail git ls-files --error-unmatch -- file3 > +' > + > +test_expect_success 'test checkout --cached --no-overlay at given paths' ' > + mkdir sub && > + >sub/file1 && > + >sub/file2 && > + git update-index --add sub/file1 sub/file2 && > + T=$(git write-tree) && > + git checkout --cached --no-overlay HEAD sub/file2 && > + test_must_fail git diff --quiet && > + U=$(git write-tree) && Do we need to worry at all about losing the exit status of write-tree in either invocation? In particular, if the second one for U fails somehow, we'd end up with $U being a blank string and we'd still probably get "$T" != "$U" below. You also had some rev-parse invocations hidden in a sub-shell in both this patch and patch 5, but subsequent commands relied on non-empty output out of those, so I figured those were fine. This one might be too, but I thought I'd at least mention it. > + echo "$T" && > + echo "$U" && > + test_must_fail git diff-index --cached --exit-code "$T" && > + test "$T" != "$U" > +' > + > +test_done > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index a3fd9a9630..cbc304ace8 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' ' > --no-quiet Z > --no-... Z > --overlay Z > + --cached Z > EOF > ' > > -- > 2.20.0.405.gbc1bbc6f85
Duy Nguyen <pclouds@gmail.com> writes: > Elijah wanted another mode (and I agree) that modifies worktree but > leaves the index alone. This is most useful (or least confusing) when > used with <tree-ish> and would be default in restore-files. I'm not > saying you have to implement it, but how do the new command line > options are designed to make sense? I'd model it after "git apply", i.e. git restore-files [--tree=<treeish>] <pathspec> would work only on the working tree files, git restore-files --tree=<treeish> --cached <pathspec> would match the entries in the index that match pathspec to the given treeish without touching the working tree, and git restore-files --tree=<treeish> --index <pathspec> would be both. I have never been happy with the phraso, the (arbitrary) distinction between --cached/--index we use, so in the very longer term (like, introducing synonym at 3.0 boundary and deprecating older ones at 4.0 boundary), it may not be a bad idea to rename "--index" to "--index-and-working-tree" and "--cached" to "--index-only". But until that happens, it would be better to use these two consistently.
On Mon, Dec 10, 2018 at 7:13 PM Junio C Hamano <gitster@pobox.com> wrote: > > Duy Nguyen <pclouds@gmail.com> writes: > > > Elijah wanted another mode (and I agree) that modifies worktree but > > leaves the index alone. This is most useful (or least confusing) when > > used with <tree-ish> and would be default in restore-files. I'm not > > saying you have to implement it, but how do the new command line > > options are designed to make sense? > > I'd model it after "git apply", i.e. > > git restore-files [--tree=<treeish>] <pathspec> > > would work only on the working tree files, > > git restore-files --tree=<treeish> --cached <pathspec> > > would match the entries in the index that match pathspec to the > given treeish without touching the working tree, and > > git restore-files --tree=<treeish> --index <pathspec> > > would be both. > > I have never been happy with the phraso, the (arbitrary) distinction > between --cached/--index we use, so in the very longer term (like, > introducing synonym at 3.0 boundary and deprecating older ones at > 4.0 boundary), it may not be a bad idea to rename "--index" to > "--index-and-working-tree" and "--cached" to "--index-only". > > But until that happens, it would be better to use these two > consistently. I agree that consistency is important, but trying to distinguish between "--cached" and "--index" is _extremely_ painful. I still can't keep the distinction straight and have to look it up every time I want to use either. I don't know how to possibly teach users the meaning. Could we introduce synonyms earlier at least, and make the synonyms more prominent than the "--cached" and "--index" terms in the documentation?
On Tue, Dec 11, 2018 at 7:12 AM Elijah Newren <newren@gmail.com> wrote: > > On Mon, Dec 10, 2018 at 7:13 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Duy Nguyen <pclouds@gmail.com> writes: > > > > > Elijah wanted another mode (and I agree) that modifies worktree but > > > leaves the index alone. This is most useful (or least confusing) when > > > used with <tree-ish> and would be default in restore-files. I'm not > > > saying you have to implement it, but how do the new command line > > > options are designed to make sense? > > > > I'd model it after "git apply", i.e. > > > > git restore-files [--tree=<treeish>] <pathspec> > > > > would work only on the working tree files, > > > > git restore-files --tree=<treeish> --cached <pathspec> > > > > would match the entries in the index that match pathspec to the > > given treeish without touching the working tree, and > > > > git restore-files --tree=<treeish> --index <pathspec> > > > > would be both. > > > > I have never been happy with the phraso, the (arbitrary) distinction > > between --cached/--index we use, so in the very longer term (like, > > introducing synonym at 3.0 boundary and deprecating older ones at > > 4.0 boundary), it may not be a bad idea to rename "--index" to > > "--index-and-working-tree" and "--cached" to "--index-only". > > > > But until that happens, it would be better to use these two > > consistently. > > I agree that consistency is important, but trying to distinguish > between "--cached" and "--index" is _extremely_ painful. I still > can't keep the distinction straight and have to look it up every time > I want to use either. I don't know how to possibly teach users the > meaning. Could we introduce synonyms earlier at least, and make the > synonyms more prominent than the "--cached" and "--index" terms in the > documentation? For git-checkout I think --index and --cached fit. For restore-files, if you come up with better names, I'll gladly use that. Otherwise I'll just use these.
On 12/10, Elijah Newren wrote: > On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > > > Add a new --cached option to git checkout, which works only on the > > index, but not the working tree, similar to what 'git reset <tree-ish> > > -- <pathspec>... does. Indeed the tests are adapted from the 'git > > reset' tests. > > > > In the longer term the idea is to potentially deprecate 'git reset > > <tree-ish> -- <pathspec>...', so the 'git reset' command becomes only > > about re-pointing the HEAD, and not also about copying entries from > > <tree-ish> to the index. > > > > Note that 'git checkout' by default works in overlay mode, meaning > > files that match the pathspec that don't exist in <tree-ish>, but > > exist in the index would not be removed. 'git checkout --no-overlay > > --cached' can be used to get the same behaviour as 'git reset > > <tree-ish> -- <pathspec>'. > > I think this argues _even more_ that --no-overlay should be the > default. Your series is valuable even if we don't push on that, I'm > just being noisy about what I think would be an even better world. I think just having that mode in 'git restore-files' Duy is working on may have to be enough for now. > Also, I don't think I've mentioned it yet, but I'm really excited > about this series and what you're doing. It's super cool. (Which I > expected when I saw the description of the desired behavior, but I'm > also liking and contemplating re-using some code...) Thanks :) > > One thing this patch doesn't currently deal with is conflicts. > > Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>' > > doesn't do anything with the index, so the --cached option just > > mirrors that behaviour. But given it doesn't even deal with > > conflicts, the '--cached' option doesn't make much sense when no > > <tree-ish> is given. As it operates only on the index, it's always a > > no-op if no tree-ish is given. > > > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > --- > > > > Maybe we can just disallow --cached without <tree-ish> given for now, > > and possibly later allow it with some different behaviour for > > conflicts, not sure what the best way forward here is. We can also > > just make it update the index as appropriate, and have it behave > > different than 'git checkout' curerntly does when handling conflicts? > > Huh? > "git checkout -- <path>" > means update <path> from the index, meaning the index is left alone > (it's the source) and only the working tree is touched. > > When you add a flag named --cached to only update the index and not > the working tree, then the index becomes the sole destination. > > Now we combine: no tree is specified means the index is the source of > the writing, and --cached being specified means the index is the sole > destination of the writing. Thus, you have a no-op. If the user > specifies --cached and no tree, you should immediately exit with a > message along the lines of "Nothing to do; no tree given and --cached > specified." The presence of conflicts seems completely irrelevant to > me here. Ah yeah you're right, thanks for a sanity check. The command I was most worried about was 'git checkout --cached --{ours,theirs} -- <pathspec>', which I thought should update the index. But as we don't give any tree-ish, I'm not sure anymore it should. Maybe just always exiting with the message you mention above is the right thing to do. > > builtin/checkout.c | 26 ++++++++-- > > t/t2016-checkout-patch.sh | 8 +++ > > t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++ > > t/t9902-completion.sh | 1 + > > 4 files changed, 135 insertions(+), 3 deletions(-) > > create mode 100755 t/t2026-checkout-cached.sh > > > > diff --git a/builtin/checkout.c b/builtin/checkout.c > > index 0aef35bbc4..6ba85e9de5 100644 > > --- a/builtin/checkout.c > > +++ b/builtin/checkout.c > > @@ -45,6 +45,7 @@ struct checkout_opts { > > int ignore_other_worktrees; > > int show_progress; > > int overlay_mode; > > + int cached; > > /* > > * If new checkout options are added, skip_merge_working_tree > > * should be updated accordingly. > > @@ -288,6 +289,10 @@ static int checkout_paths(const struct checkout_opts *opts, > > die(_("Cannot update paths and switch to branch '%s' at the same time."), > > opts->new_branch); > > > > + if (opts->patch_mode && opts->cached) > > + return run_add_interactive(revision, "--patch=reset", > > + &opts->pathspec); > > + > > if (opts->patch_mode) > > return run_add_interactive(revision, "--patch=checkout", > > &opts->pathspec); > > @@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts, > > * the current index, which means that it should > > * be removed. > > */ > > - ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE; > > + ce->ce_flags |= CE_MATCHED | CE_REMOVE; > > + if (!opts->cached) > > + ce->ce_flags |= CE_WT_REMOVE; > > continue; > > } else { > > /* > > @@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts, > > for (pos = 0; pos < active_nr; pos++) { > > struct cache_entry *ce = active_cache[pos]; > > if (ce->ce_flags & CE_MATCHED) { > > + if (opts->cached) { > > + continue; > > + } > > if (!ce_stage(ce)) { > > errs |= checkout_entry(ce, &state, NULL); > > continue; > > @@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts, > > * not tested here > > */ > > > > + /* > > + * opts->cached cannot be used with switching branches so is > > + * not tested here > > + */ > > + > > /* > > * If we aren't creating a new branch any changes or updates will > > * happen in the existing branch. Since that could only be updating > > @@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts, > > die(_("'%s' cannot be used with switching branches"), > > "--patch"); > > > > - if (!opts->overlay_mode) > > + if (opts->overlay_mode != -1) > > + die(_("'%s' cannot be used with switching branches"), > > + "--overlay/--no-overlay"); > > + > > + if (opts->cached) > > die(_("'%s' cannot be used with switching branches"), > > - "--no-overlay"); > > + "--cached"); > > > > if (opts->writeout_stage) > > die(_("'%s' cannot be used with switching branches"), > > @@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) > > PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater }, > > OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")), > > OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")), > > + OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")), > > OPT_END(), > > }; > > > > diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh > > index 47aeb0b167..e8774046e0 100755 > > --- a/t/t2016-checkout-patch.sh > > +++ b/t/t2016-checkout-patch.sh > > @@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' ' > > verify_state dir/foo head head > > ' > > > > +test_expect_success PERL 'git checkout --cached -p' ' > > + set_and_save_state dir/foo work work && > > + test_write_lines n y | git checkout --cached -p >output && > > + verify_state dir/foo work head && > > + verify_saved_state bar && > > + test_i18ngrep "Unstage" output > > +' > > + > > test_expect_success PERL 'none of this moved HEAD' ' > > verify_saved_head > > ' > > diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh > > new file mode 100755 > > index 0000000000..1b66192727 > > --- /dev/null > > +++ b/t/t2026-checkout-cached.sh > > @@ -0,0 +1,103 @@ > > +#!/bin/sh > > + > > +test_description='checkout --cached <pathspec>' > > + > > +. ./test-lib.sh > > + > > +test_expect_success 'checkout --cached <pathspec>' ' > > + echo 1 >file1 && > > + echo 2 >file2 && > > + git add file1 file2 && > > + test_tick && > > + git commit -m files && > > + git rm file2 && > > + echo 3 >file3 && > > + echo 4 >file1 && > > + git add file1 file3 && > > + git checkout --cached HEAD -- file1 file2 && > > + test_must_fail git diff --quiet && > > + > > + cat >expect <<-\EOF && > > + diff --git a/file1 b/file1 > > + index d00491f..b8626c4 100644 > > + --- a/file1 > > + +++ b/file1 > > + @@ -1 +1 @@ > > + -1 > > + +4 > > + diff --git a/file2 b/file2 > > + deleted file mode 100644 > > + index 0cfbf08..0000000 > > + --- a/file2 > > + +++ /dev/null > > + @@ -1 +0,0 @@ > > + -2 > > + EOF > > + git diff >actual && > > + test_cmp expect actual && > > + > > + cat >expect <<-\EOF && > > + diff --git a/file3 b/file3 > > + new file mode 100644 > > + index 0000000..00750ed > > + --- /dev/null > > + +++ b/file3 > > + @@ -0,0 +1 @@ > > + +3 > > + EOF > > + git diff --cached >actual && > > + test_cmp expect actual > > +' > > + > > +test_expect_success 'checking out an unmodified path is a no-op' ' > > + git reset --hard && > > + git checkout --cached HEAD -- file1 && > > + git diff-files --exit-code && > > + git diff-index --cached --exit-code HEAD > > +' > > + > > +test_expect_success 'checking out specific path that is unmerged' ' > > + test_commit file3 file3 && > > + git rm --cached file2 && > > + echo 1234 >file2 && > > + F1=$(git rev-parse HEAD:file1) && > > + F2=$(git rev-parse HEAD:file2) && > > + F3=$(git rev-parse HEAD:file3) && > > + { > > + echo "100644 $F1 1 file2" && > > + echo "100644 $F2 2 file2" && > > + echo "100644 $F3 3 file2" > > + } | git update-index --index-info && > > + git ls-files -u && > > + git checkout --cached HEAD file2 && > > + test_must_fail git diff --quiet && > > + git diff-index --exit-code --cached HEAD > > +' > > + > > +test_expect_success '--cached without --no-overlay does not remove entry from index' ' > > + test_must_fail git checkout --cached HEAD^ file3 && > > + git ls-files --error-unmatch -- file3 > > +' > > + > > +test_expect_success 'file is removed from the index with --no-overlay' ' > > + git checkout --cached --no-overlay HEAD^ file3 && > > + test_path_is_file file3 && > > + test_must_fail git ls-files --error-unmatch -- file3 > > +' > > + > > +test_expect_success 'test checkout --cached --no-overlay at given paths' ' > > + mkdir sub && > > + >sub/file1 && > > + >sub/file2 && > > + git update-index --add sub/file1 sub/file2 && > > + T=$(git write-tree) && > > + git checkout --cached --no-overlay HEAD sub/file2 && > > + test_must_fail git diff --quiet && > > + U=$(git write-tree) && > > Do we need to worry at all about losing the exit status of write-tree > in either invocation? In particular, if the second one for U fails > somehow, we'd end up with $U being a blank string and we'd still > probably get "$T" != "$U" below. Hmm this seems to be a fairly common pattern in our test suite: $ git grep -F '$(git write-tree)' t/* | wc -l 112 But maybe it's just something we used to do, but should move away from. Just writing the output to a file shouldn't be much harder either, I'll do that in the next iteration. > You also had some rev-parse invocations hidden in a sub-shell in both > this patch and patch 5, but subsequent commands relied on non-empty > output out of those, so I figured those were fine. This one might be > too, but I thought I'd at least mention it. > > > + echo "$T" && > > + echo "$U" && > > + test_must_fail git diff-index --cached --exit-code "$T" && > > + test "$T" != "$U" > > +' > > + > > +test_done > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > > index a3fd9a9630..cbc304ace8 100755 > > --- a/t/t9902-completion.sh > > +++ b/t/t9902-completion.sh > > @@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' ' > > --no-quiet Z > > --no-... Z > > --overlay Z > > + --cached Z > > EOF > > ' > > > > -- > > 2.20.0.405.gbc1bbc6f85
On Wed, Dec 12, 2018 at 2:23 AM Duy Nguyen <pclouds@gmail.com> wrote: > > On Tue, Dec 11, 2018 at 7:12 AM Elijah Newren <newren@gmail.com> wrote: > > > > On Mon, Dec 10, 2018 at 7:13 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > > > Duy Nguyen <pclouds@gmail.com> writes: > > > > > > > Elijah wanted another mode (and I agree) that modifies worktree but > > > > leaves the index alone. This is most useful (or least confusing) when > > > > used with <tree-ish> and would be default in restore-files. I'm not > > > > saying you have to implement it, but how do the new command line > > > > options are designed to make sense? > > > > > > I'd model it after "git apply", i.e. > > > > > > git restore-files [--tree=<treeish>] <pathspec> > > > > > > would work only on the working tree files, > > > > > > git restore-files --tree=<treeish> --cached <pathspec> > > > > > > would match the entries in the index that match pathspec to the > > > given treeish without touching the working tree, and > > > > > > git restore-files --tree=<treeish> --index <pathspec> > > > > > > would be both. > > > > > > I have never been happy with the phraso, the (arbitrary) distinction > > > between --cached/--index we use, so in the very longer term (like, > > > introducing synonym at 3.0 boundary and deprecating older ones at > > > 4.0 boundary), it may not be a bad idea to rename "--index" to > > > "--index-and-working-tree" and "--cached" to "--index-only". > > > > > > But until that happens, it would be better to use these two > > > consistently. > > > > I agree that consistency is important, but trying to distinguish > > between "--cached" and "--index" is _extremely_ painful. I still > > can't keep the distinction straight and have to look it up every time > > I want to use either. I don't know how to possibly teach users the > > meaning. Could we introduce synonyms earlier at least, and make the > > synonyms more prominent than the "--cached" and "--index" terms in the > > documentation? > > For git-checkout I think --index and --cached fit. For restore-files, > if you come up with better names, I'll gladly use that. Otherwise I'll > just use these. I've changed my mind. I'm not using --index and --cached for "git restore" (formerly "git restore-files"). So how about this? git restore --from=<tree> <pathspec> will update both the index and worktree. git restore --from=<tree> --keep-index <pathspec> will not update the index git restore --from=<tree> --keep-worktree <pathspec> will not update worktree And I'm thinking of adding these to "git reset" too. It will have the forth form: git reset [--keep-index] [--keep-worktree] [--keep-nothing] [<commit>] which is similar to --soft, --mixed and --hard, but with better names: --soft is --keep-worktree and --keep-index, --mixed is --keep-worktree and --hard is --keep-nothing (i.e. the only change is HEAD). Although, if I work it out, I might just ignore "git reset" and make sure "git switch" and "git restore" can do whatever "git reset" can then remove its "common command" status. This part is more thinking out loud, but "git switch" can have a new form git switch --reset-branch [--keep-worktree] [--keep-index] <start-point> which resets "HEAD" (and switch of course) but do not enter detached mode. This covers a common use case of "git reset [options] <commit>". Then either "git restore" learns about "--abort-in-progress" to abort cherry-pick/merge/revert, or those commands have a new --abort and --quit option...
Duy Nguyen <pclouds@gmail.com> writes: > I've changed my mind. I'm not using --index and --cached for "git > restore" (formerly "git restore-files"). So how about this? > > git restore --from=<tree> <pathspec> will update both the index and worktree. > > git restore --from=<tree> --keep-index <pathspec> will not update the index > > git restore --from=<tree> --keep-worktree <pathspec> will not update worktree An action to "restore" with an option to "keep" (i.e. "do not touch") smells strongly of double negation. We are restoring, i.e. grabbing something that existed in the past out of another place (like tree or index) and depositing it to the working tree to recover its previous state, oh, not but not touching the content of the working tree (or the index) intact? It would be great if you can come up with phrasing that avoids specifying what is *not* done, but instructs the command what is to be done, perhaps along the lines of "restore --index-only", "restore --worktree-only" and "restore --index-and-worktree (which would be the default)".
On Fri, Feb 1, 2019 at 2:05 AM Junio C Hamano <gitster@pobox.com> wrote: > > Duy Nguyen <pclouds@gmail.com> writes: > > > I've changed my mind. I'm not using --index and --cached for "git > > restore" (formerly "git restore-files"). So how about this? > > > > git restore --from=<tree> <pathspec> will update both the index and worktree. > > > > git restore --from=<tree> --keep-index <pathspec> will not update the index > > > > git restore --from=<tree> --keep-worktree <pathspec> will not update worktree > > An action to "restore" with an option to "keep" (i.e. "do not > touch") smells strongly of double negation. We are restoring, > i.e. grabbing something that existed in the past out of another > place (like tree or index) and depositing it to the working tree to > recover its previous state, oh, not but not touching the content of > the working tree (or the index) intact? It is negation (though not double). The thinking was, since "git restore" means restore everything, extra options will amend that to "restore everything except ..." and because the parts to keep are more important (i.e. data loss), highlighting it on the command sounds a bit better to me: "ok i'm going to need to restore this thing, but I want this part and that part to stay the same, let's type the command..." > It would be great if you can come up with phrasing that avoids > specifying what is *not* done, but instructs the command what is to > be done, perhaps along the lines of "restore --index-only", "restore > --worktree-only" and "restore --index-and-worktree (which would be > the default)". My biggest problem with --*-only is that they cannot be combined. There must be an option for every valid combination. I'm still thinking about "git reset" where there are three parts to reset/restore: HEAD, index and worktree. So at least there's another valid combination "reset HEAD but not index nor worktree", which would end becoming another option. This route does not look promising. Of course we could just do --index and --worktree, each option restores the respective part. Then it's combinable (and extensible in the future). But then "git restore" means "git restore --index --worktree" and typing "git restore --index" effectively removes the default "--worktree", which seems a bit twisted.
Duy Nguyen <pclouds@gmail.com> writes: > Of course we could just do --index and --worktree, each option > restores the respective part. Then it's combinable (and extensible in > the future). But then "git restore" means "git restore --index > --worktree" and typing "git restore --index" effectively removes the > default "--worktree", which seems a bit twisted. Or "git restore --no-worktree" (essentially, instead of saying "keep", say "no" to mean "negation"). Incidentally, "git restore --no-index" does not have a counterpart in "git checkout", but I think it is probably a good thing to add; as it has to do far more than "git cat-file blob $tree:$path >$path" these days.
On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote: > > Duy Nguyen <pclouds@gmail.com> writes: > > > Of course we could just do --index and --worktree, each option > > restores the respective part. Then it's combinable (and extensible in > > the future). But then "git restore" means "git restore --index > > --worktree" and typing "git restore --index" effectively removes the > > default "--worktree", which seems a bit twisted. > > Or "git restore --no-worktree" (essentially, instead of saying > "keep", say "no" to mean "negation"). > > Incidentally, "git restore --no-index" does not have a counterpart > in "git checkout", but I think it is probably a good thing to add; > as it has to do far more than "git cat-file blob $tree:$path >$path" > these days. Oh yes, I occasionally need that too.
On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote: > > Duy Nguyen <pclouds@gmail.com> writes: > > > Of course we could just do --index and --worktree, each option > > restores the respective part. Then it's combinable (and extensible in > > the future). But then "git restore" means "git restore --index > > --worktree" and typing "git restore --index" effectively removes the > > default "--worktree", which seems a bit twisted. > > Or "git restore --no-worktree" (essentially, instead of saying > "keep", say "no" to mean "negation"). > > Incidentally, "git restore --no-index" does not have a counterpart > in "git checkout", but I think it is probably a good thing to add; > as it has to do far more than "git cat-file blob $tree:$path >$path" > these days. OK this hopefully will be the final design (git restore) "[--worktree] <paths>" restores worktree paths from index "--index <paths>" restores the index from HEAD (aka "git reset") "--source <tree> (--index|--worktree) <paths>" restores index or worktree (or both) from <tree> I'm a bit reluctant to support "git restore --index --worktree <paths>" without --source, which should default to HEAD, since it's a bit unclear/inconsistent ("git restore --worktree <paths>" defaults to index as the source, but here we use a different default source).
Hi Duy, On Mon, Feb 18, 2019 at 8:21 PM Duy Nguyen <pclouds@gmail.com> wrote: > > On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Duy Nguyen <pclouds@gmail.com> writes: > > > > > Of course we could just do --index and --worktree, each option > > > restores the respective part. Then it's combinable (and extensible in > > > the future). But then "git restore" means "git restore --index > > > --worktree" and typing "git restore --index" effectively removes the > > > default "--worktree", which seems a bit twisted. > > > > Or "git restore --no-worktree" (essentially, instead of saying > > "keep", say "no" to mean "negation"). > > > > Incidentally, "git restore --no-index" does not have a counterpart > > in "git checkout", but I think it is probably a good thing to add; > > as it has to do far more than "git cat-file blob $tree:$path >$path" > > these days. > > OK this hopefully will be the final design > > (git restore) "[--worktree] <paths>" restores worktree paths from index > > "--index <paths>" restores the index from HEAD (aka "git reset") > > "--source <tree> (--index|--worktree) <paths>" restores index or > worktree (or both) from <tree> > > I'm a bit reluctant to support "git restore --index --worktree > <paths>" without --source, which should default to HEAD, since it's a > bit unclear/inconsistent ("git restore --worktree <paths>" defaults to > index as the source, but here we use a different default source). Sorry for going missing a while from the conversation, and thanks so much for pushing this forward. Overall this looks good, but there's just one part that confuses me. Here you seem to suggest that if you pass --source but neither --index or --worktree that both the index and working tree will be written to. Why are "restored" changes considered ready for commit? That seems confusing to me (and was one of the bugs of checkout, IMO). See also second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/ Elijah
On Tue, Feb 19, 2019 at 9:42 PM Elijah Newren <newren@gmail.com> wrote: > > OK this hopefully will be the final design > > > > (git restore) "[--worktree] <paths>" restores worktree paths from index > > > > "--index <paths>" restores the index from HEAD (aka "git reset") > > > > "--source <tree> (--index|--worktree) <paths>" restores index or > > worktree (or both) from <tree> > > > > I'm a bit reluctant to support "git restore --index --worktree > > <paths>" without --source, which should default to HEAD, since it's a > > bit unclear/inconsistent ("git restore --worktree <paths>" defaults to > > index as the source, but here we use a different default source). > > Sorry for going missing a while from the conversation, and thanks so > much for pushing this forward. > > Overall this looks good, but there's just one part that confuses me. > Here you seem to suggest that if you pass --source but neither --index > or --worktree that both the index and working tree will be written to. No no. Sorry for not being clear. If neither --index or --worktree is given, the default will be --worktree. --source changes the "source" but cannot influence target selection. So "git restore --source=HEAD foo.c" will restore foo.c on the worktree but not the index. I still remember that point you and Stefan Xenos raised ;-) Full git-restore.txt is here if you're interested. I think I'll send it out soon once git-switch part more or less settles. https://gitlab.com/pclouds/git/blob/switch-and-restore-forever/Documentation/git-restore.txt > Why are "restored" changes considered ready for commit? That seems > confusing to me (and was one of the bugs of checkout, IMO). See also > second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/ > > Elijah
Duy Nguyen <pclouds@gmail.com> writes: > On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Duy Nguyen <pclouds@gmail.com> writes: >> >> > Of course we could just do --index and --worktree, each option >> > restores the respective part. Then it's combinable (and extensible in >> > the future). But then "git restore" means "git restore --index >> > --worktree" and typing "git restore --index" effectively removes the >> > default "--worktree", which seems a bit twisted. >> >> Or "git restore --no-worktree" (essentially, instead of saying >> "keep", say "no" to mean "negation"). >> >> Incidentally, "git restore --no-index" does not have a counterpart >> in "git checkout", but I think it is probably a good thing to add; >> as it has to do far more than "git cat-file blob $tree:$path >$path" >> these days. > > OK this hopefully will be the final design > > (git restore) "[--worktree] <paths>" restores worktree paths from index > > "--index <paths>" restores the index from HEAD (aka "git reset") > > "--source <tree> (--index|--worktree) <paths>" restores index or > worktree (or both) from <tree> > > I'm a bit reluctant to support "git restore --index --worktree > <paths>" without --source, which should default to HEAD, since it's a > bit unclear/inconsistent ("git restore --worktree <paths>" defaults to > index as the source, but here we use a different default source). Ok, so we grab things from the index by default, but with --source <tree>, we can tell the command to grab things from the tree. When we are explicitly told to update the index with "--index", it would be nonsense to grab things from the index, so "--source <tree>" becomes required in that case. Makes sense. To summerize and full enumerate all the allowed variants: * --index --worktree --source <tree> <path>... Update both from a tree; --source <tree> is required. * --index --no-worktree --source <tree> <path>... Update only the index but not the working tree; --source <tree> is required. * --no-index --worktree <path>... Update only the working tree files without touching the index (new feature that cannot be done with the current Git, although "cat-file -p >path" may be close enough at times), from the index * --no-index --worktree --source <tree> <path>... Update only the working tree files without touching the index from a tree-ish. * --no-index --no-worktree <path>... Update nothing, which is a no-op that is not all that useful. * --no-index --no-worktree --source <tree> <path>... Update nothing, which is a no-op that is not all that useful. I am getting the impression that to save typing, you would want to make "--index --worktree" the default (i.e. among the above, only --no-index and --no-worktree need to be spelled explicitly), but there is one glitch. Updating from the index must be spelled explicitly with "--no-index --worktree". So perhaps the defaulting rule for the "--index" option must become a bit more tricky. Perhaps the rules are: * --worktree is the defeault; --no-worktree can be given from the command line to countermand it, and --worktree can be given from the command line to be more explicit. * when --source <tree> is given from the command line, --index is the default, and --no-index can be given to countermand it. * when --source <tree> is not given from the command line, --no-index is the only sensible choice. It can be given from the command line to be more explicit, but giving --index to countermand the --no-index default would be an error, as updating the index, whether the same update also goes to the working tree, must come from a --source <tree>. Am I following you correctly? Thanks.
Elijah Newren <newren@gmail.com> writes: > Overall this looks good, but there's just one part that confuses me. > Here you seem to suggest that if you pass --source but neither --index > or --worktree that both the index and working tree will be written to. > Why are "restored" changes considered ready for commit? That seems > confusing to me (and was one of the bugs of checkout, IMO). See also > second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/ As long as worktree-only mode does not lose track of a previously-untracked path in the index (perhaps use the i-t-a bit), I do not have a strong objection against making the worktree-only mode the default.
Junio C Hamano <gitster@pobox.com> writes: > I am getting the impression that to save typing, you would want to > make "--index --worktree" the default (i.e. among the above, only > --no-index and --no-worktree need to be spelled explicitly), but > there is one glitch. Updating from the index must be spelled > explicitly with "--no-index --worktree". And after getting reminded by Elijah, the default pair is <--no-index, --worktree>. > So perhaps the defaulting rule for the "--index" option must become > a bit more tricky. Perhaps the rules are: > > * --worktree is the default; --no-worktree can be given from the > command line to countermand it, and --worktree can be given from > the command line to be more explicit. > > * when --source <tree> is given from the command line, --index is > the default, and --no-index can be given to countermand it. Correction. * when --source <tree> is given, --no-index is the default, but --index can be given to countermand it. > > * when --source <tree> is not given from the command line, > --no-index is the only sensible choice. It can be given from the > command line to be more explicit, but giving --index to > countermand the --no-index default would be an error, as updating > the index, whether the same update also goes to the working tree, > must come from a --source <tree>. This is still correct.
On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > I am getting the impression that to save typing, you would want to > > make "--index --worktree" the default (i.e. among the above, only > > --no-index and --no-worktree need to be spelled explicitly), but > > there is one glitch. Updating from the index must be spelled > > explicitly with "--no-index --worktree". > > And after getting reminded by Elijah, the default pair is > <--no-index, --worktree>. Why would you want --no-index or --no-worktree as flags? That seems to presume a default of modifying both the index and the working tree, as these names imply undoing pieces of such a default. I'd rather have a flag like --worktree which alone only modifies the working tree and is presumed to be the default (but useful to be explicit or as mentioned later), have a flag for applying the changes to the index instead (--index?), and treat applying to both the working tree and the index as unusual and require either both flags (--worktree --index ?) or some special flag that likely has a longer name (--worktree-and-index?). I _think_ Duy does the latter reading over his manpage that he linked to, but maybe I'm just reading my own biases into it.
On Tue, Feb 19, 2019 at 11:03 AM Junio C Hamano <gitster@pobox.com> wrote: > * --no-index --worktree <path>... > > Update only the working tree files without touching the index > (new feature that cannot be done with the current Git, although > "cat-file -p >path" may be close enough at times), from the index I worry slightly that wording like this might paint ourselves into a similar corner of only correctly handling files as paths, rather than also handling directories as paths, and/or which presumes the no-overlay mode that I find problematic. Maybe your "at times" just meant "when dealing with a path that is a file that existed in the given source", but I wonder if you are pushing more than that?
On Tue, Feb 19, 2019 at 11:07 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > Overall this looks good, but there's just one part that confuses me. > > Here you seem to suggest that if you pass --source but neither --index > > or --worktree that both the index and working tree will be written to. > > Why are "restored" changes considered ready for commit? That seems > > confusing to me (and was one of the bugs of checkout, IMO). See also > > second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/ > > As long as worktree-only mode does not lose track of a > previously-untracked path in the index (perhaps use the i-t-a bit), > I do not have a strong objection against making the worktree-only > mode the default. Could you unpack that for me a bit? My assumption is that worktree-only mode doesn't touch the index (other than maybe reading from it), and treating the index as read-only means by definition it can't lose anything from there -- but then you mentioned using the intent-to-add bit, and I feel like I'm missing an important puzzle piece somewhere. Trying to make sense of it, I'm wondering if you are objecting to using overlay mode in general, or are trying to connect this to the new "precious" concept being advanced, or if there's something else you are considering here.
Elijah Newren <newren@gmail.com> writes: > On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> > I am getting the impression that to save typing, you would want to >> > make "--index --worktree" the default (i.e. among the above, only >> > --no-index and --no-worktree need to be spelled explicitly), but >> > there is one glitch. Updating from the index must be spelled >> > explicitly with "--no-index --worktree". >> >> And after getting reminded by Elijah, the default pair is >> <--no-index, --worktree>. > > Why would you want --no-index or --no-worktree as flags? That seems > to presume a default of modifying both the index and the working tree, > as these names imply undoing pieces of such a default. By "flags" I think you mean "treat them as two orthogonal booleans"? It was just how I read Duy's examples (especially the "both --index and --worktree given" example where "--source <tree>" becomes mandatory). I do not have strong preference either way myself, but I tend to think that treating these as two independent booleans would be a way to make it clear that the new design departs from what we have been doing (i.e. "--index" means "both", "--cached" means "index only" and if we were to introduce the "cat-file -p >" variant that would be called "--worktree-only"; in these, there is no "two orthogonal booleans" that can be mixed---instead they come premixed). > I'd rather have a flag like --worktree which alone only modifies the > working tree and is presumed to be the default (but useful to be > explicit or as mentioned later), have a flag for applying the changes > to the index instead (--index?), and treat applying to both the > working tree and the index as unusual and require either both flags > (--worktree --index ?) or some special flag that likely has a longer > name (--worktree-and-index?). So you are in favor of pre-mixed set of options, all are mutually exclusive. Which I am perfectly fine with. I however do think "both worktree and index" is quite common when tweaking the tree to prepare for the next commit. A path with contents freshly taken out of an existing tree may not match exactly what you plan to record in your next commit, and you would not be recording it immediately in a commit as-is. But having the contents taken from an existing tree recorded as the baseline in the index would make "git diff (no tree-ish) <path>" a handy tool to review your progress toward the next commit since that baseline state, in addition to "git diff HEAD <path>" that is the usual "how does the overall change relative to the parent of the commit I am preparing for look like". So I'd hesitate to endorse "a deliberately longer and harder to type option" to populate both the index and the working tree files at the same time.
Elijah Newren <newren@gmail.com> writes: > On Tue, Feb 19, 2019 at 11:03 AM Junio C Hamano <gitster@pobox.com> wrote: >> * --no-index --worktree <path>... >> >> Update only the working tree files without touching the index >> (new feature that cannot be done with the current Git, although >> "cat-file -p >path" may be close enough at times), from the index > > I worry slightly that wording like this might paint ourselves into a > similar corner of only correctly handling files as paths, rather than > also handling directories as paths, and/or which presumes the > no-overlay mode that I find problematic. Maybe your "at times" just > meant "when dealing with a path that is a file that existed in the > given source", but I wonder if you are pushing more than that? No, I meant "when you are not doing smudge filters (including eol conversion)", aka "'cat-file -p' gives you raw blob contents, which may not be what you want in your worktree."
Elijah Newren <newren@gmail.com> writes: >> As long as worktree-only mode does not lose track of a >> previously-untracked path in the index (perhaps use the i-t-a bit), >> I do not have a strong objection against making the worktree-only >> mode the default. > > Could you unpack that for me a bit? Suppose in an ancient version v1.0 there was a file called README.txt but these days such a file does not exist (there may be README.md added though). By default, the command does not stuff the contents to the index out of the tree and instead operate only on the working tree. $ git restore-path --source v1.0 README.txt At this point, you are assuming that README.txt will become untracked and the user needs to manually add it. I was asking if it makes sense to at least make the index "aware of" it with I-T-A bit, and I am leaning towards answering "yes" to that question.
On Tue, Feb 19, 2019 at 2:36 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > >> As long as worktree-only mode does not lose track of a > >> previously-untracked path in the index (perhaps use the i-t-a bit), > >> I do not have a strong objection against making the worktree-only > >> mode the default. > > > > Could you unpack that for me a bit? > > Suppose in an ancient version v1.0 there was a file called > README.txt but these days such a file does not exist (there may be > README.md added though). By default, the command does not stuff the > contents to the index out of the tree and instead operate only on > the working tree. > > $ git restore-path --source v1.0 README.txt > > At this point, you are assuming that README.txt will become > untracked and the user needs to manually add it. I was asking if it > makes sense to at least make the index "aware of" it with I-T-A bit, > and I am leaning towards answering "yes" to that question. Ah, thanks for all the clarifications (for this and the other emails). Your suggestion here would mean that --worktree alone wouldn't actually treat the index as read-only, but I too am leaning towards the thought that it makes sense to set the i-t-a bit for such cases.
On Wed, Feb 20, 2019 at 5:36 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > >> As long as worktree-only mode does not lose track of a > >> previously-untracked path in the index (perhaps use the i-t-a bit), > >> I do not have a strong objection against making the worktree-only > >> mode the default. > > > > Could you unpack that for me a bit? > > Suppose in an ancient version v1.0 there was a file called > README.txt but these days such a file does not exist (there may be > README.md added though). By default, the command does not stuff the > contents to the index out of the tree and instead operate only on > the working tree. > > $ git restore-path --source v1.0 README.txt > > At this point, you are assuming that README.txt will become > untracked and the user needs to manually add it. I was asking if it > makes sense to at least make the index "aware of" it with I-T-A bit, > and I am leaning towards answering "yes" to that question. I completely forgot about i-t-a! Do we want the command to add i-t-a bit by default, or only when --intent-to-add is specified? So far i-t-a has been a conscious choice, both "git reset <commit>" and "git add" require --intent-to-add to use it. The first safe step could be require --intent-to-add even in git-restore, then we could introduce a config key that enables --intent-to-add in both "git restore" and "git reset" (and perhaps "git apply" later, I still need to finish that part).
On Wed, Feb 20, 2019 at 2:10 AM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > I am getting the impression that to save typing, you would want to > > make "--index --worktree" the default (i.e. among the above, only > > --no-index and --no-worktree need to be spelled explicitly), but > > there is one glitch. Updating from the index must be spelled > > explicitly with "--no-index --worktree". > > And after getting reminded by Elijah, the default pair is > <--no-index, --worktree>. > > > So perhaps the defaulting rule for the "--index" option must become > > a bit more tricky. Perhaps the rules are: > > > > * --worktree is the default; --no-worktree can be given from the > > command line to countermand it, and --worktree can be given from > > the command line to be more explicit. I originally went with that (--no-worktree to negate default settings), but after updating docs to use "git restore" it's just too much to write. My current rules are - default is --worktree - as soon as any of --[no-]worktree or --[no-]index is specified, the above line is void. There is no default, what you specify is what you get. While it's a bit more twisted than spelling out --no-worktree to countermand the default, i think it's more natural to think and write it: ok i'm going to need to restore this and that so I write --this and --that. Going with --no-worktree you need to think "i need to write this and that and counter the default which is that other thing". We essentially have two modes, the default mode and explicit mode. Too bad? > > * when --source <tree> is given from the command line, --index is > > the default, and --no-index can be given to countermand it. > > Correction. > > * when --source <tree> is given, --no-index is the default, but > --index can be given to countermand it. > > > > > * when --source <tree> is not given from the command line, > > --no-index is the only sensible choice. It can be given from the > > command line to be more explicit, but giving --index to > > countermand the --no-index default would be an error, as updating > > the index, whether the same update also goes to the working tree, > > must come from a --source <tree>. > > This is still correct. So I guess I scratch the default source for "--index --no-worktree" (or just --index in my rules above) then? It does make sense to default restoring the index from HEAD. And it makes "git status" suggestion to unstage a bit shorter ("git restore --index <paths>" instead of "git restore --source=HEAD --index <paths>")
On Wed, Feb 20, 2019 at 5:29 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >> > I am getting the impression that to save typing, you would want to > >> > make "--index --worktree" the default (i.e. among the above, only > >> > --no-index and --no-worktree need to be spelled explicitly), but > >> > there is one glitch. Updating from the index must be spelled > >> > explicitly with "--no-index --worktree". > >> > >> And after getting reminded by Elijah, the default pair is > >> <--no-index, --worktree>. > > > > Why would you want --no-index or --no-worktree as flags? That seems > > to presume a default of modifying both the index and the working tree, > > as these names imply undoing pieces of such a default. > > By "flags" I think you mean "treat them as two orthogonal booleans"? > > It was just how I read Duy's examples (especially the "both --index > and --worktree given" example where "--source <tree>" becomes > mandatory). I do not have strong preference either way myself, but > I tend to think that treating these as two independent booleans > would be a way to make it clear that the new design departs from > what we have been doing (i.e. "--index" means "both", "--cached" > means "index only" and if we were to introduce the "cat-file -p >" > variant that would be called "--worktree-only"; in these, there is > no "two orthogonal booleans" that can be mixed---instead they come > premixed). There is indeed inconsistency in "git status" advice with my patches, where it suggests both "git restore --index" and "git rm --cached", both operate only in the index but with different option name. "--index --no-worktree" is one way to deal with this. Another way may be avoid --index in git-restore and use another name like --staging-area or something? That avoids the name clash, and we could even add --worktree and --staging-area to "git rm" / "git apply" later, deprecating (but still supporting) --index and --cached there.
On Wed, Feb 20, 2019 at 5:05 AM Elijah Newren <newren@gmail.com> wrote: > > On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Junio C Hamano <gitster@pobox.com> writes: > > > > > I am getting the impression that to save typing, you would want to > > > make "--index --worktree" the default (i.e. among the above, only > > > --no-index and --no-worktree need to be spelled explicitly), but > > > there is one glitch. Updating from the index must be spelled > > > explicitly with "--no-index --worktree". > > > > And after getting reminded by Elijah, the default pair is > > <--no-index, --worktree>. > > Why would you want --no-index or --no-worktree as flags? That seems > to presume a default of modifying both the index and the working tree, > as these names imply undoing pieces of such a default. > > I'd rather have a flag like --worktree which alone only modifies the > working tree and is presumed to be the default (but useful to be > explicit or as mentioned later), have a flag for applying the changes > to the index instead (--index?), and treat applying to both the > working tree and the index as unusual and require either both flags > (--worktree --index ?) or some special flag that likely has a longer > name (--worktree-and-index?). I'd prefer separate options. I even gave --worktree and --index shortcuts so we could write "git restore -IW" if "git restore --index --worktree" is used often (and I think it could be an alternative for "git reset --hard HEAD" if --force is also specified) > I _think_ Duy does the latter reading over his manpage that he linked > to, but maybe I'm just reading my own biases into it. Nah you read it right. The "examples" section also shows it.
diff --git a/builtin/checkout.c b/builtin/checkout.c index 0aef35bbc4..6ba85e9de5 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -45,6 +45,7 @@ struct checkout_opts { int ignore_other_worktrees; int show_progress; int overlay_mode; + int cached; /* * If new checkout options are added, skip_merge_working_tree * should be updated accordingly. @@ -288,6 +289,10 @@ static int checkout_paths(const struct checkout_opts *opts, die(_("Cannot update paths and switch to branch '%s' at the same time."), opts->new_branch); + if (opts->patch_mode && opts->cached) + return run_add_interactive(revision, "--patch=reset", + &opts->pathspec); + if (opts->patch_mode) return run_add_interactive(revision, "--patch=checkout", &opts->pathspec); @@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts, * the current index, which means that it should * be removed. */ - ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE; + ce->ce_flags |= CE_MATCHED | CE_REMOVE; + if (!opts->cached) + ce->ce_flags |= CE_WT_REMOVE; continue; } else { /* @@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts, for (pos = 0; pos < active_nr; pos++) { struct cache_entry *ce = active_cache[pos]; if (ce->ce_flags & CE_MATCHED) { + if (opts->cached) { + continue; + } if (!ce_stage(ce)) { errs |= checkout_entry(ce, &state, NULL); continue; @@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts, * not tested here */ + /* + * opts->cached cannot be used with switching branches so is + * not tested here + */ + /* * If we aren't creating a new branch any changes or updates will * happen in the existing branch. Since that could only be updating @@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts, die(_("'%s' cannot be used with switching branches"), "--patch"); - if (!opts->overlay_mode) + if (opts->overlay_mode != -1) + die(_("'%s' cannot be used with switching branches"), + "--overlay/--no-overlay"); + + if (opts->cached) die(_("'%s' cannot be used with switching branches"), - "--no-overlay"); + "--cached"); if (opts->writeout_stage) die(_("'%s' cannot be used with switching branches"), @@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater }, OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")), OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")), + OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")), OPT_END(), }; diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh index 47aeb0b167..e8774046e0 100755 --- a/t/t2016-checkout-patch.sh +++ b/t/t2016-checkout-patch.sh @@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' ' verify_state dir/foo head head ' +test_expect_success PERL 'git checkout --cached -p' ' + set_and_save_state dir/foo work work && + test_write_lines n y | git checkout --cached -p >output && + verify_state dir/foo work head && + verify_saved_state bar && + test_i18ngrep "Unstage" output +' + test_expect_success PERL 'none of this moved HEAD' ' verify_saved_head ' diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh new file mode 100755 index 0000000000..1b66192727 --- /dev/null +++ b/t/t2026-checkout-cached.sh @@ -0,0 +1,103 @@ +#!/bin/sh + +test_description='checkout --cached <pathspec>' + +. ./test-lib.sh + +test_expect_success 'checkout --cached <pathspec>' ' + echo 1 >file1 && + echo 2 >file2 && + git add file1 file2 && + test_tick && + git commit -m files && + git rm file2 && + echo 3 >file3 && + echo 4 >file1 && + git add file1 file3 && + git checkout --cached HEAD -- file1 file2 && + test_must_fail git diff --quiet && + + cat >expect <<-\EOF && + diff --git a/file1 b/file1 + index d00491f..b8626c4 100644 + --- a/file1 + +++ b/file1 + @@ -1 +1 @@ + -1 + +4 + diff --git a/file2 b/file2 + deleted file mode 100644 + index 0cfbf08..0000000 + --- a/file2 + +++ /dev/null + @@ -1 +0,0 @@ + -2 + EOF + git diff >actual && + test_cmp expect actual && + + cat >expect <<-\EOF && + diff --git a/file3 b/file3 + new file mode 100644 + index 0000000..00750ed + --- /dev/null + +++ b/file3 + @@ -0,0 +1 @@ + +3 + EOF + git diff --cached >actual && + test_cmp expect actual +' + +test_expect_success 'checking out an unmodified path is a no-op' ' + git reset --hard && + git checkout --cached HEAD -- file1 && + git diff-files --exit-code && + git diff-index --cached --exit-code HEAD +' + +test_expect_success 'checking out specific path that is unmerged' ' + test_commit file3 file3 && + git rm --cached file2 && + echo 1234 >file2 && + F1=$(git rev-parse HEAD:file1) && + F2=$(git rev-parse HEAD:file2) && + F3=$(git rev-parse HEAD:file3) && + { + echo "100644 $F1 1 file2" && + echo "100644 $F2 2 file2" && + echo "100644 $F3 3 file2" + } | git update-index --index-info && + git ls-files -u && + git checkout --cached HEAD file2 && + test_must_fail git diff --quiet && + git diff-index --exit-code --cached HEAD +' + +test_expect_success '--cached without --no-overlay does not remove entry from index' ' + test_must_fail git checkout --cached HEAD^ file3 && + git ls-files --error-unmatch -- file3 +' + +test_expect_success 'file is removed from the index with --no-overlay' ' + git checkout --cached --no-overlay HEAD^ file3 && + test_path_is_file file3 && + test_must_fail git ls-files --error-unmatch -- file3 +' + +test_expect_success 'test checkout --cached --no-overlay at given paths' ' + mkdir sub && + >sub/file1 && + >sub/file2 && + git update-index --add sub/file1 sub/file2 && + T=$(git write-tree) && + git checkout --cached --no-overlay HEAD sub/file2 && + test_must_fail git diff --quiet && + U=$(git write-tree) && + echo "$T" && + echo "$U" && + test_must_fail git diff-index --cached --exit-code "$T" && + test "$T" != "$U" +' + +test_done diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index a3fd9a9630..cbc304ace8 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' ' --no-quiet Z --no-... Z --overlay Z + --cached Z EOF '
Add a new --cached option to git checkout, which works only on the index, but not the working tree, similar to what 'git reset <tree-ish> -- <pathspec>... does. Indeed the tests are adapted from the 'git reset' tests. In the longer term the idea is to potentially deprecate 'git reset <tree-ish> -- <pathspec>...', so the 'git reset' command becomes only about re-pointing the HEAD, and not also about copying entries from <tree-ish> to the index. Note that 'git checkout' by default works in overlay mode, meaning files that match the pathspec that don't exist in <tree-ish>, but exist in the index would not be removed. 'git checkout --no-overlay --cached' can be used to get the same behaviour as 'git reset <tree-ish> -- <pathspec>'. One thing this patch doesn't currently deal with is conflicts. Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>' doesn't do anything with the index, so the --cached option just mirrors that behaviour. But given it doesn't even deal with conflicts, the '--cached' option doesn't make much sense when no <tree-ish> is given. As it operates only on the index, it's always a no-op if no tree-ish is given. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- Maybe we can just disallow --cached without <tree-ish> given for now, and possibly later allow it with some different behaviour for conflicts, not sure what the best way forward here is. We can also just make it update the index as appropriate, and have it behave different than 'git checkout' curerntly does when handling conflicts? builtin/checkout.c | 26 ++++++++-- t/t2016-checkout-patch.sh | 8 +++ t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++ t/t9902-completion.sh | 1 + 4 files changed, 135 insertions(+), 3 deletions(-) create mode 100755 t/t2026-checkout-cached.sh