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

[nguyenhu@xxxxxxxxxxxxxxx]

Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> 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.

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