Re: [PATCHv4] Read (but not write) from XDG configuration, XDG attributes and XDG ignore files
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 2, 2012, 11:20 UTC
- Message-ID
- <vpq7gvq9czb.fsf@bauges.imag.fr>
- In-Reply-To
- <1338585788-9764-1-git-send-email-Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Huynh Khoi Nguyen NGUYEN <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr> writes:
> Git will not be able to write in this new configuration file.
The use of the future is ambiguous, and since another patch will allow writing, I'd rather rephrase it as e.g.
Git currently does not write to this new configuration file.
> If core.excludesfile is not define, Git will read the global exclude
s/define/defined/
> files in $XDG_CONFIG_HOME/git/ignore. Same goes for > core.attributesfile in $XDG_CONFIG_HOME/git/attributes. If > $XDG_CONFIG_HOME is either not set or empty, $HOME/.config will be > used.
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.
> --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt
No doc for core.excludesfile and core.attributesfile change :-(.
Show 26 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. Then, the logic is really "core.excludesfile defaults to the XDG file", not "if such variable is set then do that else do something else". > --- 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. > + 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? > + 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/ ? (you don't even need to "cd")
-- Matthieu Moy http://www-verimag.imag.fr/~moy/