| Message ID | 20240307205006.467443-2-ignacio@iencinas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add hostname condition to includeIf | expand |
Ignacio Encinas <ignacio@iencinas.com> writes: > Currently, customizing the configuration depending on the machine running > git has to be done manually. > > Add support for a new includeIf keyword "hostname:" to conditionally > include configuration files depending on the hostname. > > Signed-off-by: Ignacio Encinas <ignacio@iencinas.com> > --- > Documentation/config.txt | 9 +++++++++ > config.c | 16 ++++++++++++++++ > t/t1305-config-include.sh | 22 ++++++++++++++++++++++ > 3 files changed, 47 insertions(+) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index e3a74dd1c1..9a22fd2609 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with > a naming scheme that supports more variable-based include conditions, > but currently Git only supports the exact keyword described above. > +`hostname`:: > + The data that follows the keyword `hostname:` is taken to be a > + pattern with standard globbing wildcards. If the current > + hostname matches the pattern, the include condition is met. > + OK. This seems to copy its phrasing from the existing text for "gitdir" and "onbranch", which greatly helps the description for these features consistent. > diff --git a/config.c b/config.c > index 3cfeb3d8bd..e0611fc342 100644 > --- a/config.c > +++ b/config.c > @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len) > return ret; > } > > +static int include_by_hostname(const char *cond, size_t cond_len) > +{ > + int ret; > + char my_host[HOST_NAME_MAX + 1]; > + struct strbuf pattern = STRBUF_INIT; > + if (xgethostname(my_host, sizeof(my_host))) > + return 0; > + > + strbuf_add(&pattern, cond, cond_len); > + ret = !wildmatch(pattern.buf, my_host, 0); > + strbuf_release(&pattern); > + return ret; > +} Have a blank line between the end of the decl block (our codebase frowns upon decl-after-statement) and the first statement, i.e. before "if (xgethostname...". Otherwise this looks reasonable to me. > diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh > index 5cde79ef8c..ee78d9cade 100755 > --- a/t/t1305-config-include.sh > +++ b/t/t1305-config-include.sh > @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' ' > grep "exceeded maximum include depth" stderr > ' > > +test_expect_success 'conditional include, hostname' ' > + echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config && > + echo "[test]twelve=12" >.git/bar12 && > + test_must_fail git config test.twelve && Emulating other tests in this file that uses here document may make it a bit easier to read? E.g., cat >>.gitconfig <<-EOF && [includeIf "hostname:$(hostname)a"] path = bar12 EOF > + echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config && Ditto for the remainder of the patch. Thanks.
On 7/3/24 22:40, Junio C Hamano wrote: > Ignacio Encinas <ignacio@iencinas.com> writes: > >> Currently, customizing the configuration depending on the machine running >> git has to be done manually. >> >> Add support for a new includeIf keyword "hostname:" to conditionally >> include configuration files depending on the hostname. >> >> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com> >> --- >> Documentation/config.txt | 9 +++++++++ >> config.c | 16 ++++++++++++++++ >> t/t1305-config-include.sh | 22 ++++++++++++++++++++++ >> 3 files changed, 47 insertions(+) >> >> diff --git a/Documentation/config.txt b/Documentation/config.txt >> index e3a74dd1c1..9a22fd2609 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with >> a naming scheme that supports more variable-based include conditions, >> but currently Git only supports the exact keyword described above. > >> +`hostname`:: >> + The data that follows the keyword `hostname:` is taken to be a >> + pattern with standard globbing wildcards. If the current >> + hostname matches the pattern, the include condition is met. >> + > > OK. This seems to copy its phrasing from the existing text for > "gitdir" and "onbranch", which greatly helps the description for > these features consistent. > >> diff --git a/config.c b/config.c >> index 3cfeb3d8bd..e0611fc342 100644 >> --- a/config.c >> +++ b/config.c >> @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len) >> return ret; >> } >> >> +static int include_by_hostname(const char *cond, size_t cond_len) >> +{ >> + int ret; >> + char my_host[HOST_NAME_MAX + 1]; >> + struct strbuf pattern = STRBUF_INIT; >> + if (xgethostname(my_host, sizeof(my_host))) >> + return 0; >> + >> + strbuf_add(&pattern, cond, cond_len); >> + ret = !wildmatch(pattern.buf, my_host, 0); >> + strbuf_release(&pattern); >> + return ret; >> +} > > Have a blank line between the end of the decl block (our codebase > frowns upon decl-after-statement) and the first statement, > i.e. before "if (xgethostname...". > > Otherwise this looks reasonable to me. Got it. >> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh >> index 5cde79ef8c..ee78d9cade 100755 >> --- a/t/t1305-config-include.sh >> +++ b/t/t1305-config-include.sh >> @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' ' >> grep "exceeded maximum include depth" stderr >> ' >> >> +test_expect_success 'conditional include, hostname' ' >> + echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config && >> + echo "[test]twelve=12" >.git/bar12 && >> + test_must_fail git config test.twelve && > > Emulating other tests in this file that uses here document may make > it a bit easier to read? E.g., > > cat >>.gitconfig <<-EOF && > [includeIf "hostname:$(hostname)a"] > path = bar12 > EOF Thanks for pointing that out. I just read the last tests from that file where they used the echo "..." >> approach. Do you think it is worthwhile rewriting those tests to use the approach you suggested? By the way, before contributing, I saw there is some work on moving to unit tests. I wasn't sure how to test this particular feature there, so I went with the "old" approach as it seemed more natural. Is this ok? >> + echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config && > > Ditto for the remainder of the patch. > > Thanks. Thank you for the review.
Ignacio Encinas Rubio <ignacio@iencinas.com> writes: >>> +test_expect_success 'conditional include, hostname' ' >>> + echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config && >>> + echo "[test]twelve=12" >.git/bar12 && >>> + test_must_fail git config test.twelve && >> >> Emulating other tests in this file that uses here document may make >> it a bit easier to read? E.g., >> >> cat >>.gitconfig <<-EOF && >> [includeIf "hostname:$(hostname)a"] >> path = bar12 >> EOF > > Thanks for pointing that out. I just read the last tests from that file > where they used the echo "..." >> approach. Do you think it is > worthwhile rewriting those tests to use the approach you suggested? I had an impression that existing ones do not have ugliness of backslash-quoting and do not benefit from such a rewrite to use here-document as much as the one you added does. If that is not the case, and existing ones would concretely improve the readability with such a rewrite to use here-document, surely. If we want to do that route, we should either do one of the two. - The patch [1/2] does such a "style update" only on existing tests to improve their readability, and then patch [2/2] then does your addition to the tests, together with the code change. - Or you do this patch alone, without touching existing tests, but with your tests added in an easier-to-read style. And then after the dust settles, a separate "style udpate" patch clean the existing ones up. > By the way, before contributing, I saw there is some work on moving to > unit tests. I wasn't sure how to test this particular feature there, so > I went with the "old" approach as it seemed more natural. Is this ok? We are not really "moving to". We did not have a test framework for effective unit tests, so some tests that would have been easier to manage as unit tests were instead written as an end-to-end integration tests, which was what we had a framework for. These tests are moving to, but for a test like "the user uses the '[includeIf X:Y] path = P' construct---does the git command really shows the effect of include from P when condition X:Y holds?", the unit testing framework would not be a better fit than the end-to-end behaviour test, I would say.
diff --git a/Documentation/config.txt b/Documentation/config.txt index e3a74dd1c1..9a22fd2609 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with a naming scheme that supports more variable-based include conditions, but currently Git only supports the exact keyword described above. +`hostname`:: + The data that follows the keyword `hostname:` is taken to be a + pattern with standard globbing wildcards. If the current + hostname matches the pattern, the include condition is met. + A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching. @@ -261,6 +266,10 @@ Example path = foo.inc [remote "origin"] url = https://example.com/git + +; include only if the hostname of the machine matches some-hostname +[includeIf "hostname:some-hostname"] + path = foo.inc ---- Values diff --git a/config.c b/config.c index 3cfeb3d8bd..e0611fc342 100644 --- a/config.c +++ b/config.c @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len) return ret; } +static int include_by_hostname(const char *cond, size_t cond_len) +{ + int ret; + char my_host[HOST_NAME_MAX + 1]; + struct strbuf pattern = STRBUF_INIT; + if (xgethostname(my_host, sizeof(my_host))) + return 0; + + strbuf_add(&pattern, cond, cond_len); + ret = !wildmatch(pattern.buf, my_host, 0); + strbuf_release(&pattern); + return ret; +} + static int add_remote_url(const char *var, const char *value, const struct config_context *ctx UNUSED, void *data) { @@ -406,6 +420,8 @@ static int include_condition_is_true(const struct key_value_info *kvi, else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond, &cond_len)) return include_by_remote_url(inc, cond, cond_len); + else if (skip_prefix_mem(cond, cond_len, "hostname:", &cond, &cond_len)) + return include_by_hostname(cond, cond_len); /* unknown conditionals are always false */ return 0; diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index 5cde79ef8c..ee78d9cade 100755 --- a/t/t1305-config-include.sh +++ b/t/t1305-config-include.sh @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' ' grep "exceeded maximum include depth" stderr ' +test_expect_success 'conditional include, hostname' ' + echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config && + echo "[test]twelve=12" >.git/bar12 && + test_must_fail git config test.twelve && + + echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config && + echo 12 >expect && + git config test.twelve >actual && + test_cmp expect actual +' + +test_expect_success 'conditional include, hostname, wildcard' ' + echo "[includeIf \"hostname:$(hostname)a*\"]path=bar13" >>.git/config && + echo "[test]thirteen=13" >.git/bar13 && + test_must_fail git config test.thirteen && + + echo "[includeIf \"hostname:$(hostname)*\"]path=bar13" >>.git/config && + echo 13 >expect && + git config test.thirteen >actual && + test_cmp expect actual +' + test_done
Currently, customizing the configuration depending on the machine running git has to be done manually. Add support for a new includeIf keyword "hostname:" to conditionally include configuration files depending on the hostname. Signed-off-by: Ignacio Encinas <ignacio@iencinas.com> --- Documentation/config.txt | 9 +++++++++ config.c | 16 ++++++++++++++++ t/t1305-config-include.sh | 22 ++++++++++++++++++++++ 3 files changed, 47 insertions(+)