| Message ID | 20181209200449.16342-8-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: > > Currently when 'git checkout -- <pathspec>...' is invoked with > multiple pathspecs, where one or more of the pathspecs don't match > anything, checkout errors out. > > This can be inconvenient in some cases, such as when using git > checkout from a script. Wait, should scripts go with read-tree, checkout-index or other plumbing commands instead?
On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > Currently when 'git checkout -- <pathspec>...' is invoked with > multiple pathspecs, where one or more of the pathspecs don't match > anything, checkout errors out. > > This can be inconvenient in some cases, such as when using git > checkout from a script. Introduce a new --ignore-unmatched option, > which which allows us to ignore a non-matching pathspec instead of > erroring out. > > In a subsequent commit we're going to start using 'git checkout' in > 'git stash' and are going to make use of this feature. This makes sense, but seems incomplete. But to explain it, I think there's another bug I need to demonstrate first because it's related on builds on it. First, the setup: $ echo foo >subdir/newfile $ git add subdir/newfile $ echo bar >>subdir/newfile $ git status On branch A Changes to be committed: (use "git reset HEAD <file>..." to unstage) new file: subdir/newfile Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory) modified: subdir/newfile Now, does it do what we expect? $ git checkout HEAD -- subdir/newfile error: pathspec 'subdir/newfile' did not match any file(s) known to git This is the old overlay behavior; kinda lame, but you made no claims about fixing the default behavior. What about with your new option? $ git checkout --no-overlay HEAD -- subdir $ git status On branch A nothing to commit, working tree clean Yes, the feature seems to work as advertised. However, let's try again with a different variant: $ echo foo >subdir/newfile $ git checkout --no-overlay HEAD -- subdir $ git status On branch A Untracked files: (use "git add <file>..." to include in what will be committed) subdir/newfile Why is the file ignored and left there? Also: $ git checkout --no-overlay HEAD -- subdir/newfile error: pathspec 'subdir/newfile' did not match any file(s) known to git That seems wrong to me. The point of no-overlay is to make it match HEAD, and while subdir/newfile doesn't exist in HEAD or the index it does match in the working tree so the intent is clear. But let's say that the user did go ahead and specify your new flag: $ git checkout --no-overlay --ignore-unmatch HEAD -- subdir/newfile $ git status On branch A Untracked files: (use "git add <file>..." to include in what will be committed) subdir/newfile nothing added to commit but untracked files present (use "git add" to track) So now it avoids erroring out when the user does more work than necessary, but it still misses appropriately cleaning up the file.
On 12/10, Duy Nguyen wrote: > On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > > > Currently when 'git checkout -- <pathspec>...' is invoked with > > multiple pathspecs, where one or more of the pathspecs don't match > > anything, checkout errors out. > > > > This can be inconvenient in some cases, such as when using git > > checkout from a script. > > Wait, should scripts go with read-tree, checkout-index or other > plumbing commands instead? Possibly. As mentioned in an other email, we do seem to have some scripts in git.git that are using 'git checkout' already, but they are using it in the checkout branch mode, rather than the checkout paths mode that I would like to use it in git-stash. But with the rewrite of 'git stash' in C, maybe this step is moot anyway, and we can just call the checkout_paths function internally without using the run_command API at all. We could then have an internal mode for ignoring unmatched pathspecs that we wouldn't need to expose to users. > -- > Duy
On 12/10, Elijah Newren wrote: > On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > > > Currently when 'git checkout -- <pathspec>...' is invoked with > > multiple pathspecs, where one or more of the pathspecs don't match > > anything, checkout errors out. > > > > This can be inconvenient in some cases, such as when using git > > checkout from a script. Introduce a new --ignore-unmatched option, > > which which allows us to ignore a non-matching pathspec instead of > > erroring out. > > > > In a subsequent commit we're going to start using 'git checkout' in > > 'git stash' and are going to make use of this feature. > > This makes sense, but seems incomplete. But to explain it, I think > there's another bug I need to demonstrate first because it's related > on builds on it. First, the setup: > > $ echo foo >subdir/newfile > $ git add subdir/newfile > $ echo bar >>subdir/newfile > $ git status > On branch A > Changes to be committed: > (use "git reset HEAD <file>..." to unstage) > > new file: subdir/newfile > > Changes not staged for commit: > (use "git add <file>..." to update what will be committed) > (use "git checkout -- <file>..." to discard changes in working directory) > > modified: subdir/newfile > > Now, does it do what we expect? > > $ git checkout HEAD -- subdir/newfile > error: pathspec 'subdir/newfile' did not match any file(s) known to git > > This is the old overlay behavior; kinda lame, but you made no claims > about fixing the default behavior. What about with your new option? > > $ git checkout --no-overlay HEAD -- subdir > $ git status > On branch A > nothing to commit, working tree clean > > Yes, the feature seems to work as advertised. However, let's try > again with a different variant: > > $ echo foo >subdir/newfile > $ git checkout --no-overlay HEAD -- subdir > $ git status > On branch A > Untracked files: > (use "git add <file>..." to include in what will be committed) > > subdir/newfile > > Why is the file ignored and left there? Also: > > $ git checkout --no-overlay HEAD -- subdir/newfile > error: pathspec 'subdir/newfile' did not match any file(s) known to git > > That seems wrong to me. Ah interesting, this is a case I didn't consider. I'm a bit torn on this one. My intention for the no overlay mode was that it would work similar to what I'd expect 'git reset --hard -- <pathspec>' to work if it existed, which means not removing untracked files if they exist. While I think in the example you have above removing subdir/newfile may be the right behaviour I'm not so sure in the case of 'git checkout --no-overlay HEAD -- .' or ''git checkout --no-overlay HEAD -- t/*' for example. I don't think that should remove all untracked files in the repository or in the t/ directory. Removing untracked files in that case would probably surprise users more than your case above would. I think it's okay to keep considering untracked files as special with respect to how they are treated by 'git checkout --no-overlay'. > The point of no-overlay is to make it match > HEAD, and while subdir/newfile doesn't exist in HEAD or the index it > does match in the working tree so the intent is clear. But let's say > that the user did go ahead and specify your new flag: > > > $ git checkout --no-overlay --ignore-unmatch HEAD -- subdir/newfile > $ git status > On branch A > Untracked files: > (use "git add <file>..." to include in what will be committed) > > subdir/newfile > > nothing added to commit but untracked files present (use "git add" to track) > > So now it avoids erroring out when the user does more work than > necessary, but it still misses appropriately cleaning up the file. Yeah this is a good point, this could be more confusing to the user than the previous case in my opinion. Maybe I'll just drop this patch for now (and the next one, as it's better to hold of until stash in C lands anyway), and then try to do all this in-core for 'git stash'.
diff --git a/builtin/checkout.c b/builtin/checkout.c index 6ba85e9de5..7e7b5cd1d3 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -46,6 +46,7 @@ struct checkout_opts { int show_progress; int overlay_mode; int cached; + int ignore_unmatched; /* * If new checkout options are added, skip_merge_working_tree * should be updated accordingly. @@ -358,7 +359,8 @@ static int checkout_paths(const struct checkout_opts *opts, ce->ce_flags |= CE_MATCHED; } - if (report_path_error(ps_matched, &opts->pathspec, opts->prefix)) { + if (!opts->ignore_unmatched && + report_path_error(ps_matched, &opts->pathspec, opts->prefix)) { free(ps_matched); return 1; } @@ -586,6 +588,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts, * not tested here */ + /* + * opts->ignore_unmatched 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 @@ -1320,6 +1327,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) 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_BOOL(0, "ignore-unmatched", &opts.ignore_unmatched, N_("don't error on unmatched pathspecs")), OPT_END(), }; diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh index fc3eb43b89..b44cdf7b63 100755 --- a/t/t2022-checkout-paths.sh +++ b/t/t2022-checkout-paths.sh @@ -78,4 +78,13 @@ test_expect_success 'do not touch files that are already up-to-date' ' test_cmp expect actual ' +test_expect_success 'checkout --ignore-unmatched' ' + test_commit file1 && + echo changed >file1.t && + git checkout --ignore-unmatched -- file1.t unknown-file && + echo file1 >expect && + test_cmp expect file1.t + +' + test_done diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index cbc304ace8..475debcf95 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1438,6 +1438,7 @@ test_expect_success 'double dash "git checkout"' ' --no-... Z --overlay Z --cached Z + --ignore-unmatched Z EOF '
Currently when 'git checkout -- <pathspec>...' is invoked with multiple pathspecs, where one or more of the pathspecs don't match anything, checkout errors out. This can be inconvenient in some cases, such as when using git checkout from a script. Introduce a new --ignore-unmatched option, which which allows us to ignore a non-matching pathspec instead of erroring out. In a subsequent commit we're going to start using 'git checkout' in 'git stash' and are going to make use of this feature. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- builtin/checkout.c | 10 +++++++++- t/t2022-checkout-paths.sh | 9 +++++++++ t/t9902-completion.sh | 1 + 3 files changed, 19 insertions(+), 1 deletion(-)