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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2010/11/16 Jonathan Nieder <jrnieder@xxxxxxxxx>:
>> Â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.

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

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

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

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

>> --- 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]