Re: [PATCH 07/10] checkout: add -S to update sparse checkout

[Jonathan Nieder <jrnieder@gmail.com>]

Nguyễn Thái Ngọc Duy wrote:

> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -12,6 +12,7 @@ SYNOPSIS
>  'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
>  'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
>  'git checkout' --patch [<tree-ish>] [--] [<paths>...]
> +'git checkout' -S
>  
>  DESCRIPTION
>  -----------
> @@ -176,6 +177,13 @@ the conflicted merge in the specified paths.
>  This means that you can use `git checkout -p` to selectively discard
>  edits from your current working tree.
>  
> +-S::
> +--update-sparse-checkout::
> +	An editor is invoked to let you update your sparse checkout
> +	patterns. The updated patterns will be saved in
> +	$GIT_DIR/info/sparse-checkout. The working directory is also
> +	updated. An empty file will abort the process.

Wording nit: this doesn't make the worktree more up-to-date.  How
about:

 --edit-sparse-checkout
 --define-work-area
 --narrow-worktree

Hmph.

--edit-sparse-checkout seems best for consistency of the choices I can
think of.

[...]
> @@ -316,6 +324,14 @@ $ git add frotz
>  ------------
>  
>  
> +ENVIRONMENT AND CONFIGURATION VARIABLES
> +---------------------------------------
> +The editor used to edit the commit log message will be chosen from the
                                 ^
  patterns defining what subset of the tracked tree to work on

> +GIT_EDITOR environment variable, the core.editor configuration variable, the

Might be simpler to say:

 CONFIGURATION
 -------------
 --edit-sparse-checkout uses the configured editor.  See git-var(1) for
 details.

[...]
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -675,6 +675,31 @@ static const char *unique_tracking_name(const char *name)
>  	return NULL;
>  }
>  
> +static void edit_info_sparse_checkout()
> +{
> +	char *tmpfile = xstrdup(git_path("sparse-checkout"));

git_pathdup()?

> +	struct exclude_list el;
> +	int i;
> +
> +	copy_file(tmpfile, git_path("info/sparse-checkout"), 0666);
> +
> +	if (launch_editor(tmpfile, NULL, NULL))
> +		exit(1);
> +
> +	memset(&el, 0, sizeof(el));
> +	if (add_excludes_from_file_to_list(tmpfile, "", 0, NULL, &el, 0) < 0 ||
> +	    el.nr == 0)
> +		die("No valid sparse patterns. Abort.");

Perhaps the error message should mention the temp file.

> +	for (i = 0; i < el.nr; i++)
> +		free(el.excludes[i]);
> +	free(el.excludes);

Neat.

> +
> +	if (rename(tmpfile, git_path("info/sparse-checkout")) < 0)
> +		die_errno("Can't update %s", git_path("info/sparse-checkout"));

Wouldn't git_path() clobber errno?  Probably worth keeping a temporary
with the result from git_path before the rename.

> +
> +	free(tmpfile);
> +}
> +
>  int cmd_checkout(int argc, const char **argv, const char *prefix)
>  {
>  	struct checkout_opts opts;
> @@ -685,6 +710,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	char *conflict_style = NULL;
>  	int patch_mode = 0;
>  	int dwim_new_local_branch = 1;
> +	int update_sparse_checkout = 0;
>  	struct option options[] = {
>  		OPT__QUIET(&opts.quiet),
>  		OPT_STRING('b', NULL, &opts.new_branch, "branch",
> @@ -704,6 +730,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  		OPT_STRING(0, "conflict", &conflict_style, "style",
>  			   "conflict style (merge or diff3)"),
>  		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
> +		OPT_BOOLEAN('S', "update-sparse-checkout", &update_sparse_checkout,
> +			    "open up editor to edit $GIT_DIR/info/sparse-checkout" ),
>  		{ OPTION_BOOLEAN, 0, "guess", &dwim_new_local_branch, NULL,
>  		  "second guess 'git checkout no-such-branch'",
>  		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
> @@ -722,6 +750,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, options, checkout_usage,
>  			     PARSE_OPT_KEEP_DASHDASH);
>  
> +	if (update_sparse_checkout && !core_apply_sparse_checkout)
> +		die("core.sparseCheckout needs to be turned on");

--%s requires core.sparsecheckout to be turned out

> +
>  	/* we can assume from now on new_branch = !new_branch_force */
>  	if (opts.new_branch && opts.new_branch_force)
>  		die("-B cannot be used with -b");
> @@ -874,6 +905,9 @@ no_reference:
>  		if (!pathspec)
>  			die("invalid path specification");
>  
> +		if (update_sparse_checkout)
> +			die("git checkout: update paths is incompatible with updating sparse checkout.");
> +

"updating paths is incompatible", I think.

Maybe an area for future work. :)

> @@ -892,8 +926,11 @@ no_reference:
>  		return checkout_paths(source_tree, pathspec, &opts);
>  	}
>  
> -	if (patch_mode)
> +	if (patch_mode) {
> +		if (update_sparse_checkout)
> +			die("git checkout: interactive checkout is incompatible with updating sparse checkout.");

Hmm, I'd put

	if (patch_mode && update_sparse_checkout)
		die(...

earlier as part of option validation.

[...]
> --- a/t/t1011-read-tree-sparse-checkout.sh
> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -184,4 +184,22 @@ test_expect_success 'read-tree --reset removes outside worktree' '
>  	test_cmp empty result
>  '
>  
> +test_expect_success 'git checkout -S fails to launch editor' '
> +	GIT_EDITOR=/non-existent test_must_fail git checkout -S &&

Exporting envvars via a function is non-portable.  The usual workaround:

	(
		GIT_EDITOR=... &&
		export  GIT_EDITOR &&
		test_must_fail ...
	) &&

Maybe it's time to introduce

	eval_must_fail GIT_EDITOR=/non-existent git checkout -S

?

[...]
> +test_expect_success 'git checkout -S' '
> +	git checkout -f top &&
> +	cat >editor.sh <<\EOF &&
> +#!/bin/sh
> +echo sub > "$1"
> +EOF

Style: the git test scripts usually omit the space after >.

> --- a/templates/info--sparse-checkout
> +++ b/templates/info--sparse-checkout
> @@ -1,3 +1,4 @@
>  # Specify what files are checked out in working directory
>  # Run "git checkout" after updating this file to update working directory
> +# If this is opened by "git checkout -S", empty this file will abort it.
>  *

Usage nits: s/if/when/; s/opened/edited/; s/will/to/; s/it//.

 # When editing with "git checkout -S", empty this file to abort.