Re: [PATCH v5 06/17] sparse-checkout: create 'disable' subcommand

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Nov 19, 2019, 16:15 UTC
Message-ID: <20191119161545.GH23183@szeder.dev>
In-Reply-To: <c48535cd5c00310403ffccac7507f399fca5a8a4.1571666187.git.gitgitgadget@gmail.com>

On Mon, Oct 21, 2019 at 01:56:15PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 25 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> The instructions for disabling a sparse-checkout to a full
> working directory are complicated and non-intuitive. Add a
> subcommand, 'git sparse-checkout disable', to perform those
> steps for the user.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-sparse-checkout.txt | 27 ++++++++++++---------------
>  builtin/sparse-checkout.c             | 26 +++++++++++++++++++++++++-
>  t/t1091-sparse-checkout-builtin.sh    | 15 +++++++++++++++
>  3 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index b933043b3d..f794d4797a 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -48,6 +48,10 @@ To avoid interfering with other worktrees, it first enables the
>  	working directory to match the new patterns. Enable the
>  	core.sparseCheckout config setting if it is not already enabled.
>  
> +'disable'::
> +	Remove the sparse-checkout file, set `core.sparseCheckout` to
> +	`false`, and restore the working directory to include all files.

I think it would make sense to leave the sparse-checkout file behind as-is, and only update the coonfiguration and the working tree. That way users could quickly go back to their last sparse checkout setup by simply running 'git sparse-checkout init'.

Show 20 quoted lines

> +static int sparse_checkout_disable(int argc, const char **argv)
> +{
> +	char *sparse_filename;
> +	FILE *fp;
> +
> +	if (sc_set_config(MODE_ALL_PATTERNS))
> +		die(_("failed to change config"));
> +
> +	sparse_filename = get_sparse_checkout_filename();
> +	fp = xfopen(sparse_filename, "w");
> +	fprintf(fp, "/*\n");
> +	fclose(fp);
> +
> +	if (update_working_directory())
> +		die(_("error while refreshing working directory"));
> +
> +	unlink(sparse_filename);
> +	free(sparse_filename);
> +
> +	return sc_set_config(MODE_NO_PATTERNS);

Hrm. So disabling the sparse-checkout calls the same helper function to write the configuration as initializing or setting, but with a different parameter. However, the error message in that function is:

  error(_("failed to enable core.sparseCheckout"));

So if something goes wrong while disabling the sparse-checkout, the user might get an error saying "error: failed to enable core.sparseCheckout". That doesn't look quite right, does it?

> +}