| Message ID | 20240319183722.211300-1-ignacio@iencinas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add hostname condition to includeIf | expand |
On Tue, Mar 19, 2024 at 2:38 PM Ignacio Encinas <ignacio@iencinas.com> wrote: > It was pointed out that it wasn't particularly obvious what it was meant by > > "If the current hostname matches the pattern, the include condition is met." > > which is definitely true. Despite this, to my knowledge, there isn't a > way to precisely define what we mean by "hostname" other than saying > that we mean whatever is returned by gethostname(2). > > I still think the documentation isn't great, but I don't see a way to > improve it further. Peff provided the answer when he suggested[1] implementing `git config --show-hostname-for-includes`. [1]: https://lore.kernel.org/git/20240318081722.GA602575@coredump.intra.peff.net/ > 1: cf175154109e ! 2: dec622c38916 config: learn the "hostname:" includeIf condition > @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards > +`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. > ++ hostname (output of gethostname(2)) matches the > ++ pattern, the include condition is met. This is still unnecessarily user-hostile, especially to users who are not programmers, but also to programmers who don't want to waste time writing a little test program to determine what gethostname(2) returns on each platform they use. That's not a great situation. Peff felt that adding `git config --show-hostname-for-includes` was probably overkill, but I'd argue that it is necessary to enable users to deterministically figure out the value to use in their configuration rather than having to grope around in the dark via guesswork and trial-and-error to figure out exactly what works. And the option name doesn't necessarily have to be so verbose; a shorter name, such as `git config --show-hostname` may be good enough. Implementing this option would also obviate the need to implement `test-tool xgethostname` (though, I agree with Junio that `test-tool gethostname` would have been a better, less implementation-revealing name).
Eric Sunshine <sunshine@sunshineco.com> writes: > Peff felt that adding `git config --show-hostname-for-includes` was > probably overkill, but I'd argue that it is necessary to enable users > to deterministically figure out the value to use in their > configuration rather than having to grope around in the dark via > guesswork and trial-and-error to figure out exactly what works. > > And the option name doesn't necessarily have to be so verbose; a > shorter name, such as `git config --show-hostname` may be good enough. > Implementing this option would also obviate the need to implement > `test-tool xgethostname` (though, I agree with Junio that `test-tool > gethostname` would have been a better, less implementation-revealing > name). Yeah, I like that show-hostname thing (which I do not know if "config" is a good home for, though).
On Tue, Mar 19, 2024 at 5:12 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Peff felt that adding `git config --show-hostname-for-includes` was > > probably overkill, but I'd argue that it is necessary to enable users > > to deterministically figure out the value to use in their > > configuration rather than having to grope around in the dark via > > guesswork and trial-and-error to figure out exactly what works. > > > > And the option name doesn't necessarily have to be so verbose; a > > shorter name, such as `git config --show-hostname` may be good enough. > > Implementing this option would also obviate the need to implement > > `test-tool xgethostname` (though, I agree with Junio that `test-tool > > gethostname` would have been a better, less implementation-revealing > > name). > > Yeah, I like that show-hostname thing (which I do not know if "config" > is a good home for, though). The other possibility which came to mind was adding a GIT_HOSTNAME variable to the output of `git var -l`.
On 19/3/24 21:55, Eric Sunshine wrote: > On Tue, Mar 19, 2024 at 2:38 PM Ignacio Encinas <ignacio@iencinas.com> wrote: >> It was pointed out that it wasn't particularly obvious what it was meant by >> >> "If the current hostname matches the pattern, the include condition is met." >> >> which is definitely true. Despite this, to my knowledge, there isn't a >> way to precisely define what we mean by "hostname" other than saying >> that we mean whatever is returned by gethostname(2). >> >> I still think the documentation isn't great, but I don't see a way to >> improve it further. > > Peff provided the answer when he suggested[1] implementing `git config > --show-hostname-for-includes`. > > [1]: https://lore.kernel.org/git/20240318081722.GA602575@coredump.intra.peff.net/ Sorry if it sounded like I disregarded the opinion. I did see it and liked the idea, but I guessed something like that would face a lot of resistance. My bad. >> 1: cf175154109e ! 2: dec622c38916 config: learn the "hostname:" includeIf condition >> @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards >> +`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. >> ++ hostname (output of gethostname(2)) matches the >> ++ pattern, the include condition is met. > > This is still unnecessarily user-hostile, especially to users who are > not programmers, but also to programmers who don't want to waste time > writing a little test program to determine what gethostname(2) returns > on each platform they use. That's not a great situation. > > Peff felt that adding `git config --show-hostname-for-includes` was > probably overkill, but I'd argue that it is necessary to enable users > to deterministically figure out the value to use in their > configuration rather than having to grope around in the dark via > guesswork and trial-and-error to figure out exactly what works. > > And the option name doesn't necessarily have to be so verbose; a > shorter name, such as `git config --show-hostname` may be good enough. > Implementing this option would also obviate the need to implement > `test-tool xgethostname` (though, I agree with Junio that `test-tool > gethostname` would have been a better, less implementation-revealing > name). Lets find this a good "home" then [1]. Thanks! [1] https://lore.kernel.org/git/CAPig+cTFRAmzBGiJv2F-k1XWvGSbT8UeAG57T+XpB-1w66HRkQ@mail.gmail.com/
Junio C Hamano <gitster@pobox.com> writes: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> Peff felt that adding `git config --show-hostname-for-includes` was >> probably overkill, but I'd argue that it is necessary to enable users >> to deterministically figure out the value to use in their >> configuration rather than having to grope around in the dark via >> guesswork and trial-and-error to figure out exactly what works. >> >> And the option name doesn't necessarily have to be so verbose; a >> shorter name, such as `git config --show-hostname` may be good enough. >> Implementing this option would also obviate the need to implement >> `test-tool xgethostname` (though, I agree with Junio that `test-tool >> gethostname` would have been a better, less implementation-revealing >> name). > > Yeah, I like that show-hostname thing (which I do not know if "config" > is a good home for, though). A thought when I was reading this: wouldn't it be enough to document that `uname -n` can be used to get the hostname that should be used? As far as I know this should be POSIX-compliant and uses gethostname(2). Dirk
On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote: >Junio C Hamano <gitster@pobox.com> writes: > >> Eric Sunshine <sunshine@sunshineco.com> writes: >> >>> Peff felt that adding `git config --show-hostname-for-includes` was >>> probably overkill, but I'd argue that it is necessary to enable users >>> to deterministically figure out the value to use in their >>> configuration rather than having to grope around in the dark via >>> guesswork and trial-and-error to figure out exactly what works. >>> >>> And the option name doesn't necessarily have to be so verbose; a >>> shorter name, such as `git config --show-hostname` may be good enough. >>> Implementing this option would also obviate the need to implement >>> `test-tool xgethostname` (though, I agree with Junio that `test-tool >>> gethostname` would have been a better, less implementation-revealing >>> name). >> >> Yeah, I like that show-hostname thing (which I do not know if "config" >> is a good home for, though). > >A thought when I was reading this: wouldn't it be enough to document that `uname -n` can be used to get the hostname that should >be used? > >As far as I know this should be POSIX-compliant and uses gethostname(2). As previously pointed out, uname -n and gethostname(2) are not equivalent. uname -n does not (depending on implementation) go to DNS while gethostname(2) goes to DNS first (although apparently glibc may not). This is particularly important in a multi-home situation where more than one IP adapter has a different IP address on the same host, and where DNS does not consider the different addresses to be equivalent (which otherwise could cause problems for reverse lookups). --Randall
<rsbecker@nexbridge.com> writes: > On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote: >>Junio C Hamano <gitster@pobox.com> writes: >> >>> Eric Sunshine <sunshine@sunshineco.com> writes: >>> >>>> Peff felt that adding `git config --show-hostname-for-includes` was >>>> probably overkill, but I'd argue that it is necessary to enable users >>>> to deterministically figure out the value to use in their >>>> configuration rather than having to grope around in the dark via >>>> guesswork and trial-and-error to figure out exactly what works. >>>> >>>> And the option name doesn't necessarily have to be so verbose; a >>>> shorter name, such as `git config --show-hostname` may be good enough. >>>> Implementing this option would also obviate the need to implement >>>> `test-tool xgethostname` (though, I agree with Junio that `test-tool >>>> gethostname` would have been a better, less implementation-revealing >>>> name). >>> >>> Yeah, I like that show-hostname thing (which I do not know if "config" >>> is a good home for, though). >> >>A thought when I was reading this: wouldn't it be enough to document that > `uname -n` can be used to get the hostname that should >>be used? >> >>As far as I know this should be POSIX-compliant and uses gethostname(2). > > As previously pointed out, uname -n and gethostname(2) are not equivalent. > uname -n does not (depending on implementation) go to DNS while > gethostname(2) goes to DNS first (although apparently glibc may not). This > is particularly important in a multi-home situation where more than one IP > adapter has a different IP address on the same host, and where DNS does not > consider the different addresses to be equivalent (which otherwise could > cause problems for reverse lookups). Thanks for the explanation, I did not notice this has already been discussed. Interestingly, I strace(1)'ed uname -n here on Linux and noticed it uses uname(2) (what else?) and not gethostname(2), so it seems I was completely wrong. Sorry for disturbing the discussion. Dirk
On Tuesday, March 19, 2024 6:27 PM, Dirk Gouders wrote: ><rsbecker@nexbridge.com> writes: > >> On Tuesday, March 19, 2024 5:37 PM, Dirk Gouders wrote: >>>Junio C Hamano <gitster@pobox.com> writes: >>> >>>> Eric Sunshine <sunshine@sunshineco.com> writes: >>>> >>>>> Peff felt that adding `git config --show-hostname-for-includes` was >>>>> probably overkill, but I'd argue that it is necessary to enable >>>>> users to deterministically figure out the value to use in their >>>>> configuration rather than having to grope around in the dark via >>>>> guesswork and trial-and-error to figure out exactly what works. >>>>> >>>>> And the option name doesn't necessarily have to be so verbose; a >>>>> shorter name, such as `git config --show-hostname` may be good enough. >>>>> Implementing this option would also obviate the need to implement >>>>> `test-tool xgethostname` (though, I agree with Junio that >>>>> `test-tool gethostname` would have been a better, less >>>>> implementation-revealing name). >>>> >>>> Yeah, I like that show-hostname thing (which I do not know if "config" >>>> is a good home for, though). >>> >>>A thought when I was reading this: wouldn't it be enough to document >>>that >> `uname -n` can be used to get the hostname that should >>>be used? >>> >>>As far as I know this should be POSIX-compliant and uses gethostname(2). >> >> As previously pointed out, uname -n and gethostname(2) are not equivalent. >> uname -n does not (depending on implementation) go to DNS while >> gethostname(2) goes to DNS first (although apparently glibc may not). >> This is particularly important in a multi-home situation where more >> than one IP adapter has a different IP address on the same host, and >> where DNS does not consider the different addresses to be equivalent >> (which otherwise could cause problems for reverse lookups). > >Thanks for the explanation, I did not notice this has already been discussed. > >Interestingly, I strace(1)'ed uname -n here on Linux and noticed it uses >uname(2) (what else?) and not gethostname(2), so it seems I was completely >wrong. > >Sorry for disturbing the discussion. No worries. I only know this point because I was rather deeply in a related code base back in 1994. I did not know that glibc varied from an old UNIX (I think that's where the code was from) code base prior to this thread. Learning is good and never a problem. Regards, Randall
<rsbecker@nexbridge.com> writes: > No worries. I only know this point because I was rather deeply in a related > code base back in 1994. I did not know that glibc varied from an old UNIX (I > think that's where the code was from) code base prior to this thread. > Learning is good and never a problem. It is not surprising that you were doing gethostname in 1994, but it is very surprising that you still remember such details ;-)
On Tue, Mar 19, 2024 at 05:13:47PM -0400, Eric Sunshine wrote: > On Tue, Mar 19, 2024 at 5:12 PM Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > Peff felt that adding `git config --show-hostname-for-includes` was > > > probably overkill, but I'd argue that it is necessary to enable users > > > to deterministically figure out the value to use in their > > > configuration rather than having to grope around in the dark via > > > guesswork and trial-and-error to figure out exactly what works. > > > > > > And the option name doesn't necessarily have to be so verbose; a > > > shorter name, such as `git config --show-hostname` may be good enough. > > > Implementing this option would also obviate the need to implement > > > `test-tool xgethostname` (though, I agree with Junio that `test-tool > > > gethostname` would have been a better, less implementation-revealing > > > name). > > > > Yeah, I like that show-hostname thing (which I do not know if "config" > > is a good home for, though). > > The other possibility which came to mind was adding a GIT_HOSTNAME > variable to the output of `git var -l`. That strikes me as a more appropriate spot than an option to git-config. Even if config is the only thing _now_ which cares about the hostname, it may be something that other parts of the system care about in the future. Some care may need to be taken for error handling, though. For "git var GIT_HOSTNAME" it is OK to exit non-zero, but "git var -l" should not bail on a system where gethostname() doesn't work (it is still not clear to me if that is a real case to worry about or not). -Peff
On Tue, Mar 19, 2024 at 8:19 PM Jeff King <peff@peff.net> wrote: > On Tue, Mar 19, 2024 at 05:13:47PM -0400, Eric Sunshine wrote: > > The other possibility which came to mind was adding a GIT_HOSTNAME > > variable to the output of `git var -l`. > > That strikes me as a more appropriate spot than an option to git-config. > Even if config is the only thing _now_ which cares about the hostname, > it may be something that other parts of the system care about in the > future. Also, taking into consideration Patrick's proposed revamp[1] of git-config to give it a subcommand API, then git-config becomes an even less welcome place for a standalone --show-hostname option which, by itself, doesn't really fit into the subcommand paradigm, and it probably doesn't make sense to add a new subcommand ("info" or whatever) just for that. [1]: https://lore.kernel.org/git/cover.1710198711.git.ps@pks.im/T/#u > Some care may need to be taken for error handling, though. For "git var > GIT_HOSTNAME" it is OK to exit non-zero, but "git var -l" should not > bail on a system where gethostname() doesn't work (it is still not clear > to me if that is a real case to worry about or not). Ports to oddball platforms should probably be providing a gethostname() in "compat/" anyhow, just as is done for Windows in "compat/mingw.c".
On Tue, Mar 19, 2024 at 10:49 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Tue, Mar 19, 2024 at 8:19 PM Jeff King <peff@peff.net> wrote: > > Some care may need to be taken for error handling, though. For "git var > > GIT_HOSTNAME" it is OK to exit non-zero, but "git var -l" should not > > bail on a system where gethostname() doesn't work (it is still not clear > > to me if that is a real case to worry about or not). > > Ports to oddball platforms should probably be providing a > gethostname() in "compat/" anyhow, just as is done for Windows in > "compat/mingw.c". Ignore my mumbo jumbo response. You are, of course, correct that the implementation of `git var -l` needs to be done with care so that it doesn't bail if gethostname() fails; that's true regardless of whether or not a platform-specific "compat/" implementation is part of the mix.
The suggestion for having `git var` list GIT_HOSTNAME gave me an idea: perhaps instead of, or in addition to, a `hostname` condition in the `includeif` code, we could: * have an `includeif:env:...` condition that tests an env variable against a pattern; and/or * use $GIT_HOSTNAME as the variable. We'd then set `GIT_HOSTNAME` to the gethostname() result *unless* it's already set. This gives users much more flexibility, because: * they can use the hostname and/or arbitrary-env-var condition; * they can then *set* GIT_HOSTNAME to the short or full hostname at their discretion if the default is not suitable for some reason; and of course * they can, as noted, use `git var` to find the default setting. Chris
On Wed, Mar 20, 2024 at 10:35 AM Chris Torek <chris.torek@gmail.com> wrote: > The suggestion for having `git var` list GIT_HOSTNAME gave me > an idea: perhaps instead of, or in addition to, a `hostname` > condition in the `includeif` code, we could: > > * have an `includeif:env:...` condition that tests an env > variable against a pattern; and/or > * use $GIT_HOSTNAME as the variable. > > We'd then set `GIT_HOSTNAME` to the gethostname() result *unless* > it's already set. > > This gives users much more flexibility, because: > > * they can use the hostname and/or arbitrary-env-var condition; > * they can then *set* GIT_HOSTNAME to the short or full > hostname at their discretion if the default is not suitable > for some reason; and of course > * they can, as noted, use `git var` to find the default setting. This certainly is a much more generic approach, and simplifies the implementation considerably since it obviates the need for GIT_HOSTNAME (or --show-hostname) since the choice of variable name and value is fully under the user's control. I have some vague feeling that this idea of using an environment variable as a condition may have been discussed before and possibly rejected due to potential security concerns, but I don't use `includeif` myself and haven't really followed past discussions, so I could be wrong about that. Peff would probably have better recollection.
Chris Torek <chris.torek@gmail.com> writes: > The suggestion for having `git var` list GIT_HOSTNAME gave me > an idea: perhaps instead of, or in addition to, a `hostname` > condition in the `includeif` code, we could: > > * have an `includeif:env:...` condition that tests an env > variable against a pattern; and/or > * use $GIT_HOSTNAME as the variable. Nice. > We'd then set `GIT_HOSTNAME` to the gethostname() result *unless* > it's already set. > > This gives users much more flexibility, because: > > * they can use the hostname and/or arbitrary-env-var condition; > * they can then *set* GIT_HOSTNAME to the short or full > hostname at their discretion if the default is not suitable > for some reason; and of course > * they can, as noted, use `git var` to find the default setting. > > Chris
On Wed, Mar 20, 2024 at 12:37:22PM -0400, Eric Sunshine wrote: > I have some vague feeling that this idea of using an environment > variable as a condition may have been discussed before and possibly > rejected due to potential security concerns, but I don't use > `includeif` myself and haven't really followed past discussions, so I > could be wrong about that. Peff would probably have better > recollection. I can't think of any security concerns; if you can control the environment you can already set GIT_CONFIG_PARAMETERS to do whatever you like. In fact, I think I've suggested includeIf.env before. ;) Ævar even wrote a patch, but I think we got bogged down in issues of syntax: https://lore.kernel.org/git/patch-1.1-1fe6f60d2bf-20210924T005553Z-avarab@gmail.com/ -Peff
Extend includeIf to take hostname into account. Motivating request can be found here [1]. [1] https://github.com/gitgitgadget/git/issues/1665 A lot of feedback was given to the v2 of this patch. It was pointed out that it wasn't particularly obvious what it was meant by "If the current hostname matches the pattern, the include condition is met." which is definitely true. Despite this, to my knowledge, there isn't a way to precisely define what we mean by "hostname" other than saying that we mean whatever is returned by gethostname(2). In my opinion, terms like "short hostname" can be confusing in some cases, and I'm not even sure we can rely on $hostname to agree with gethostname(2) in every platform. I still think the documentation isn't great, but I don't see a way to improve it further. Thanks everyone for the feedback! Changes since v2: * Expose the result of xgethostname through test-tool * Rewrite test to rely on test-tool xgethostname rather than using $hostname * Clarify documentation, specifying that by "hostname" we mean output of gethostname(2) Changes since v1: * Add blank line between declarations and code in `include_by_branch`. * Rewrite "echo"s used in tests to make them more readable. Ignacio Encinas (2): t: add a test helper for getting hostname config: learn the "hostname:" includeIf condition Documentation/config.txt | 10 +++++++++ Makefile | 1 + config.c | 17 +++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/helper/test-xgethostname.c | 12 +++++++++++ t/t1305-config-include.sh | 42 ++++++++++++++++++++++++++++++++++++ t/t6500-gc.sh | 3 +-- 8 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 t/helper/test-xgethostname.c Range-diff against v2: -: ------------ > 1: ee1f9b1da037 t: add a test helper for getting hostname 1: cf175154109e ! 2: dec622c38916 config: learn the "hostname:" includeIf condition @@ Documentation/config.txt: As for the naming of this keyword, it is for forwards +`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. ++ hostname (output of gethostname(2)) matches the ++ pattern, the include condition is met. + A few more notes on matching via `gitdir` and `gitdir/i`: @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' ' +test_expect_success 'conditional include, hostname' ' + cat >>.git/config <<-EOF && -+ [includeIf "hostname:$(hostname)a"] ++ [includeIf "hostname:$(test-tool xgethostname)a"] + path = bar12 + EOF + cat >>.git/bar12 <<-EOF && @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' ' + test_must_fail git config test.twelve && + + cat >>.git/config <<-EOF && -+ [includeIf "hostname:$(hostname)"] ++ [includeIf "hostname:$(test-tool xgethostname)"] + path = bar12 + EOF + echo 12 >expect && @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' ' + +test_expect_success 'conditional include, hostname, wildcard' ' + cat >>.git/config <<-EOF && -+ [includeIf "hostname:$(hostname)a*"] ++ [includeIf "hostname:$(test-tool xgethostname)a*"] + path = bar13 + EOF + cat >>.git/bar13 <<-EOF && @@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' ' + test_must_fail git config test.thirteen && + + cat >>.git/config <<-EOF && -+ [includeIf "hostname:$(hostname)*"] ++ [includeIf "hostname:$(test-tool xgethostname)*"] + path = bar13 + EOF + echo 13 >expect && base-commit: e09f1254c54329773904fe25d7c545a1fb4fa920