[7/8] checkout: allow ignoring unmatched pathspec
diff mbox series

Message ID 20181209200449.16342-8-t.gummerer@gmail.com
State New
Headers show
Series
  • introduce no-overlay and cached mode in git checkout
Related show

Commit Message

Thomas Gummerer Dec. 9, 2018, 8:04 p.m. UTC
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(-)

Comments

Duy Nguyen Dec. 10, 2018, 4:51 p.m. UTC | #1
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?
Elijah Newren Dec. 10, 2018, 8:25 p.m. UTC | #2
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.
Thomas Gummerer Dec. 11, 2018, 10:23 p.m. UTC | #3
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
Thomas Gummerer Dec. 11, 2018, 10:36 p.m. UTC | #4
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'.

Patch
diff mbox series

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
 '