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

From: SZEDER Gábor <[email-masked]>
Date: Tue, 19 Nov 2019 17:15:45 +0100

On Mon, Oct 21, 2019 at 01:56:15PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email-masked]>
> 
> 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 <[email-masked]>
> ---
>  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'.

> +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?

> +}