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