Re: [PATCH v5 04/17] sparse-checkout: 'set' subcommand

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Nov 21, 2019, 13:36 UTC
Message-ID: <20191121133628.GT23183@szeder.dev>
In-Reply-To: <64b230d4-d36f-5d92-7570-6d965db471a5@gmail.com>

On Thu, Nov 21, 2019 at 08:04:35AM -0500, Derrick Stolee wrote:

Show 29 quoted lines

> On 11/19/2019 10:46 AM, SZEDER Gábor wrote:
> > On Mon, Oct 21, 2019 at 01:56:13PM +0000, Derrick Stolee via GitGitGadget wrote:
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> The 'git sparse-checkout set' subcommand takes a list of patterns
> >> as arguments and writes them to the sparse-checkout file. Then, it
> >> updates the working directory using 'git read-tree -mu HEAD'.
> >>
> >> The 'set' subcommand will replace the entire contents of the
> >> sparse-checkout file. The write_patterns_and_update() method is
> >> extracted from cmd_sparse_checkout() to make it easier to implement
> >> 'add' and/or 'remove' subcommands in the future.
> > 
> > Yeah, an 'add' subcommand would be great, because 'set' throwing away
> > all the existing patterns can lead to some frustration:
> 
> I plan to extend this feature when this series lands.
> 
> > Having said that, being forced to use 'git sparse-checkout set' and
> > specify all patterns at once does have its own benefits: I needed like
> > 6 subdirectories to build that subset of a big project that I was
> > interested in, and now all the necessary patterns are saved in my Bash
> > history, and I will be able to easily dig out the complete command in
> > the future.  That wouldn't work with using the 'add' subcommand to add
> > one pattern at a time.
> 
> In general, this "set" command is created first because tools can interact
> with it more easily than "add" and "remove". Users would probably prefer the
> "add" and "remove" way to interact.

Makes sense.

However, I'd like to add that in the meantime I got to like the 'set' subcommand more and more. I enabled-disabled sparse checkout a lot of times while testing and trying to poke holes, and to be able to set up everything with only one single command that I can easily recall from the shell's history was a great help.

Show 26 quoted lines

> >> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> >> index cb74715ca6..bf2dc55bb1 100755
> >> --- a/t/t1091-sparse-checkout-builtin.sh
> >> +++ b/t/t1091-sparse-checkout-builtin.sh
> >> @@ -96,4 +96,36 @@ test_expect_success 'clone --sparse' '
> >>  	test_cmp expect dir
> >>  '
> >>  
> >> +test_expect_success 'set enables config' '
> >> +	git init empty-config &&
> >> +	(
> >> +		cd empty-config &&
> >> +		test_commit test file &&
> >> +		test_path_is_missing .git/config.worktree &&
> >> +		test_must_fail git sparse-checkout set nothing &&
> > 
> > This command prints the same error message twice:
> > 
> >   + test_must_fail git sparse-checkout set nothing
> >   error: Sparse checkout leaves no entry on working directory
> >   error: Sparse checkout leaves no entry on working directory
> 
> At this commit, I do not see two identical lines, but instead the second
> line says
> 
> 	error: failed to update index with new sparse-checkout paths

You're right, I must have looked at and copied the results from a test run of the full patch series or even from 'next'. The error message changes with commit efd9c53b6d (sparse-checkout: update working directory in-process, 2019-10-21) to what I shown above.

Interestingly, at one point in my testing I managed to get a different error three times:

  $ ~/src/git/git sparse-checkout set foo
  error: Entry 'dir/bar' not uptodate. Cannot update sparse checkout.
  error: Entry 'dir/bar' not uptodate. Cannot update sparse checkout.
  error: Entry 'dir/bar' not uptodate. Cannot update sparse checkout.

However, when I later re-run the same sequence of commands that lead to that three errors I got that error only twice, and couldn't reproduce it since.

Style nit, seeing "error: Sparse..." and "error: Entry...": error messages should start with a lower-case letter.

> (that "paths" should probably be "patterns")

That would be better indeed, but that error message goes away by the end of the series anyway...