Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files

[nguyenhu@minatec.inpg.fr]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=";"; format=flowed	DelSp=Yes, Size: 4453 bytes --]



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 ;)

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

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

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

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