Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files
- From
- nguyenhu@minatec.inpg.fr <nguyenhu@minatec.inpg.fr>
- Date
- Jun 2, 2012, 15:52 UTC
- Message-ID
- <20120602175209.Horde.QpN5M3wdC4BPyjaps7w1bMA@webmail.minatec.grenoble-inp.fr>
- In-Reply-To
- <vpq7gvq9czb.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> I'd prefer having two separate patches for the config file and for the > two others. If ignore and attributes are simple enough, they may go to > the same patch, but ideally, there would be two separate patches again.
We will separate this patch in three different patches.
> No doc for core.excludesfile and core.attributesfile change :-(.
It will be done for the next patch ;)
Show 52 quoted lines
>> --- a/attr.c
>> +++ b/attr.c
>> @@ -497,6 +497,9 @@ static int git_attr_system(void)
>> static void bootstrap_attr_stack(void)
>> {
>> struct attr_stack *elem;
>> + char *xdg_attributes_file;
>> +
>> + home_config_paths(NULL, &xdg_attributes_file, "attributes");
>>
>> if (attr_stack)
>> return;
>> @@ -522,6 +525,13 @@ static void bootstrap_attr_stack(void)
>> elem->prev = attr_stack;
>> attr_stack = elem;
>> }
>> + } else if (!access(xdg_attributes_file, R_OK)) {
>> + elem = read_attr_from_file(xdg_attributes_file, 1);
>> + if (elem) {
>> + elem->origin = NULL;
>> + elem->prev = attr_stack;
>> + attr_stack = elem;
>> + }
>> }
>>
>> if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
>
> The logic seems overly complex, and you duplicate the if() uselessly.
>
> Why not just set the variable git_attributes_file before entering the
> if? Something like this:
>
> diff --git a/attr.c b/attr.c
> index 303751f..71dc472 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -515,6 +515,9 @@ static void bootstrap_attr_stack(void)
> }
> }
>
> + if (!git_attributes_file)
> + git_attributes_file = "foo";
> +
> if (git_attributes_file) {
> elem = read_attr_from_file(git_attributes_file, 1);
> if (elem) {
>
> (obviously replacing "foo" by the actual code involving
> home_config_paths(..., "attributes")).
>
> Doing so, you may even get rid of the "if (git_attributes_file)" on the
> next line.We first thought to use an "else if" in order not to pointlessly check the existence of the xdg_attributes_file (or double checking git_attributes_file) if git_attributes_file exists. BTW after checking the code more closely, we do not need to verify the existence of the xdg_attributes_file so it is indeed more clean to use your version, Thank.
Show 27 quoted lines
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1234,13 +1234,17 @@ int remove_dir_recursively(struct strbuf
>> *path, int flag)
>> void setup_standard_excludes(struct dir_struct *dir)
>> {
>> const char *path;
>> + char *xdg_path;
>>
>> dir->exclude_per_dir = ".gitignore";
>> path = git_path("info/exclude");
>> + home_config_paths(NULL, &xdg_path, "ignore");
>> if (!access(path, R_OK))
>> add_excludes_from_file(dir, path);
>> if (excludes_file && !access(excludes_file, R_OK))
>> add_excludes_from_file(dir, excludes_file);
>> + else if (!access(xdg_path, R_OK))
>> + add_excludes_from_file(dir, xdg_path);
>> }
> Same remark here. Look at the patch I sent earlier to give a default
> value:
>
> http://thread.gmane.org/gmane.comp.version-control.git/133343/focus=133415
>
> For example, you version reads from XDG file if core.excludesfile is
> set, but the file it points to doesn't exist. I don't think this is
> expected.Actually, it's the opposite. Our version only read from XDG file if core.excludesfile is not set. After checking your patch, it may be more logical to inialize the default value of excludes_file to the xdg_path as done for the attributes file.
Show 5 quoted lines
>> + echo foo >to_be_excluded && >> + git add to_be_excluded && >> + git rm --cached to_be_excluded && > > Err, why add and remove it? You just need to create it, right?
It was to check if to_be_excluded is indeed not ignored at the beginning of the test before ignoring it but that's seem a bit over-testing, I'll remove it.
Show 10 quoted lines
>> + cd .. && >> + mkdir -p .config/git/ && > > I don't like these relative references to $HOME. If you mean $HOME, why > not say > > mkdir -p $HOME/.config/git/ > echo "f attr_f" >$HOME/.config/git/ > > ?
It will be fixed but BTW where the tests are executed, $HOME has a weird behaviour:
echo $HOME and echo "$HOME"
both returns /.../t/trash directory.t1306-read-xdg-config-file
but echo foo >$HOME writes in ../t/trash
while echo foo >"$HOME" writes in t/trash directory.t1306-read-xdg-config-file
so "$HOME" is needed for the tests to work.