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

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