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

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Nov 16, 2010, 03:08 UTC
Message-ID: <AANLkTim3LMhQiO0RmPTW3dLNQa0FxkyX3m7Jjd_rPN7C@mail.gmail.com>
In-Reply-To: <20101115211636.GH16385@burratino>

2010/11/16 Jonathan Nieder <jrnieder@gmail.com>:

Show 24 quoted lines

>>  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.

Yep. --edit-sparse-checkout.

Show 13 quoted lines

>> @@ -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:

That was copied from git-commit.txt (I think). Maybe that file should be updated too.

Show 12 quoted lines

> [...]
>> --- 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()?

Yup.

Show 6 quoted lines

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

This code is actually duplicated in unpack_trees(). It's time to create free_exclude() function.

Show 6 quoted lines

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

OK

Show 22 quoted lines

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

I'll go with the former.

-- 
Duy