Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path
Remi Galan Alfonsowrites: > I tried to see if the `git config` in other tests were in the > same case or not but the sheer amount made me reconsider. However > taking a look at a couple of random ones, there are some scripts > that would benefit from the conversion. Yes, that is why I said "it is a good idea to do this where necessary, but the test Dscho touched here does not need it". It is a given that there are tons of "git config" users as we have a lot more tests that were written before test_config was invented. It is good to occasionally modernise ancient tests; we usually do so when we need to make some other changes to them (e.g. I added a feature, and wanted to add a couple of new tests, but I found it unreadable and unmaintainable because it kept the ancient style, so I am updating the existing tests to more modern style first before doing my changes--and then do my changes on top of the cleaned up codebase). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path
Junio C Hamanowrites: > Remi Galan Alfonso > writes: > >> Johannes Schindelin writes: >>> Hi Rémi, >>> >>> On Tue, 16 Aug 2016, Remi Galan Alfonso wrote: >>> >>> > Johannes Schindelin writes: >>> > > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh >>> > > index 5e3fb3a..f1f9aee 100755 >>> > > --- a/t/t1350-config-hooks-path.sh >>> > > +++ b/t/t1350-config-hooks-path.sh >>> > > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of >>> > > specifying core.hooksPath work' >>> > > test_cmp expect actual >>> > > ' >>> > > >>> > > +test_expect_success 'git rev-parse --git-path hooks' ' >>> > > +git config core.hooksPath .git/custom-hooks && >>> > >>> > Any reason to not use 'test_config' here? >>> >>> Yes: consistency. The rest of the script uses `git config`, not >>> `test_config`. >> >> Fine by me, then. Sorry for the noise. > > No, thanks for reviewing. I'll take Dscho's patch as-is but once it > hits 'next', it probably is a good idea to do a separate clean-up > patch on top to use test_config where necessary. > > Having said that, this entire script is about constantly changing > the value of that single configuration variable and see how the code > performs, so any new test added after existing ones is expected to > ignore left-over values in the variable and set it to a value of its > own liking. So I suspect there is no existing "git config" call in > this script, with or without Dscho's patch, that would benefit from > getting converted to test_config. Thanks for checking the ones in this file, considering the lack of benefits it might not be worth it to change it for now. I tried to see if the `git config` in other tests were in the same case or not but the sheer amount made me reconsider. However taking a look at a couple of random ones, there are some scripts that would benefit from the conversion. For example in t3200-branch there is: test_expect_success 'git branch with column.*' ' git config column.ui column && git config column.branch "dense" && COLUMNS=80 git branch >actual && git config --unset column.branch && git config --unset column.ui && cat >expected <<\EOF && a/b/c bam foo l * mastern o/p r abc bar j/k m/m master2 o/o q EOF test_cmp expected actual ' The conversion would drop 2 lines in this particular case and would avoid bleeding the config should `git branch` fail (not sure how possible that is...). (I can make a patch for t3200 but that won't be before days/weeks, so if someone else wants to take care of it I won't mind) If I have some time to kill, I'll try looking at a few others but I won't expect that any time soon. Thanks, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path
Remi Galan Alfonsowrites: > Johannes Schindelin writes: >> Hi Rémi, >> >> On Tue, 16 Aug 2016, Remi Galan Alfonso wrote: >> >> > Johannes Schindelin writes: >> > > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh >> > > index 5e3fb3a..f1f9aee 100755 >> > > --- a/t/t1350-config-hooks-path.sh >> > > +++ b/t/t1350-config-hooks-path.sh >> > > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of >> > > specifying core.hooksPath work' >> > > test_cmp expect actual >> > > ' >> > > >> > > +test_expect_success 'git rev-parse --git-path hooks' ' >> > > +git config core.hooksPath .git/custom-hooks && >> > >> > Any reason to not use 'test_config' here? >> >> Yes: consistency. The rest of the script uses `git config`, not >> `test_config`. > > Fine by me, then. Sorry for the noise. No, thanks for reviewing. I'll take Dscho's patch as-is but once it hits 'next', it probably is a good idea to do a separate clean-up patch on top to use test_config where necessary. Having said that, this entire script is about constantly changing the value of that single configuration variable and see how the code performs, so any new test added after existing ones is expected to ignore left-over values in the variable and set it to a value of its own liking. So I suspect there is no existing "git config" call in this script, with or without Dscho's patch, that would benefit from getting converted to test_config. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path
Johannes Schindelinwrites: > Hi Rémi, > > On Tue, 16 Aug 2016, Remi Galan Alfonso wrote: > > > Johannes Schindelin writes: > > > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh > > > index 5e3fb3a..f1f9aee 100755 > > > --- a/t/t1350-config-hooks-path.sh > > > +++ b/t/t1350-config-hooks-path.sh > > > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of > > > specifying core.hooksPath work' > > > test_cmp expect actual > > > ' > > > > > > +test_expect_success 'git rev-parse --git-path hooks' ' > > > +git config core.hooksPath .git/custom-hooks && > > > > Any reason to not use 'test_config' here? > > Yes: consistency. The rest of the script uses `git config`, not > `test_config`. Fine by me, then. Sorry for the noise. > Ciao, > Dscho Ciao, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path
Hi Rémi, On Tue, 16 Aug 2016, Remi Galan Alfonso wrote: > Johannes Schindelinwrites: > > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh > > index 5e3fb3a..f1f9aee 100755 > > --- a/t/t1350-config-hooks-path.sh > > +++ b/t/t1350-config-hooks-path.sh > > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of > > specifying core.hooksPath work' > > test_cmp expect actual > > ' > > > > +test_expect_success 'git rev-parse --git-path hooks' ' > > +git config core.hooksPath .git/custom-hooks && > > Any reason to not use 'test_config' here? Yes: consistency. The rest of the script uses `git config`, not `test_config`. Ciao, Dscho
Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path
Hi Johannes, Johannes Schindelinwrites: > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh > index 5e3fb3a..f1f9aee 100755 > --- a/t/t1350-config-hooks-path.sh > +++ b/t/t1350-config-hooks-path.sh > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of > specifying core.hooksPath work' > test_cmp expect actual > ' > > +test_expect_success 'git rev-parse --git-path hooks' ' > +git config core.hooksPath .git/custom-hooks && Any reason to not use 'test_config' here? Thanks, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] rev-parse: respect core.hooksPath in --git-path
The idea of the --git-path option is not only to avoid having to prefix paths with the output of --git-dir all the time, but also to respect overrides for specific common paths inside the .git directory (e.g. `git rev-parse --git-path objects` will report the value of the environment variable GIT_OBJECT_DIRECTORY, if set). When introducing the core.hooksPath setting, we forgot to adjust git_path() accordingly. This patch fixes that. While at it, revert the special-casing of core.hooksPath in run-command.c, as it is now no longer needed. Signed-off-by: Johannes Schindelin--- Published-As: https://github.com/dscho/git/releases/tag/git-path-hooks-v2 Fetch-It-Via: git fetch https://github.com/dscho/git git-path-hooks-v2 Interdiff vs v1: diff --git a/run-command.c b/run-command.c index 33bc63a..5a4dbb6 100644 --- a/run-command.c +++ b/run-command.c @@ -824,10 +824,7 @@ const char *find_hook(const char *name) static struct strbuf path = STRBUF_INIT; strbuf_reset(); - if (git_hooks_path) - strbuf_addf(, "%s/%s", git_hooks_path, name); - else - strbuf_git_path(, "hooks/%s", name); + strbuf_git_path(, "hooks/%s", name); if (access(path.buf, X_OK) < 0) return NULL; return path.buf; path.c | 2 ++ run-command.c| 5 + t/t1350-config-hooks-path.sh | 6 ++ 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/path.c b/path.c index 17551c4..fe3c4d9 100644 --- a/path.c +++ b/path.c @@ -380,6 +380,8 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len) get_index_file(), strlen(get_index_file())); else if (git_db_env && dir_prefix(base, "objects")) replace_dir(buf, git_dir_len + 7, get_object_directory()); + else if (git_hooks_path && dir_prefix(base, "hooks")) + replace_dir(buf, git_dir_len + 5, git_hooks_path); else if (git_common_dir_env) update_common_dir(buf, git_dir_len, NULL); } diff --git a/run-command.c b/run-command.c index 33bc63a..5a4dbb6 100644 --- a/run-command.c +++ b/run-command.c @@ -824,10 +824,7 @@ const char *find_hook(const char *name) static struct strbuf path = STRBUF_INIT; strbuf_reset(); - if (git_hooks_path) - strbuf_addf(, "%s/%s", git_hooks_path, name); - else - strbuf_git_path(, "hooks/%s", name); + strbuf_git_path(, "hooks/%s", name); if (access(path.buf, X_OK) < 0) return NULL; return path.buf; diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh index 5e3fb3a..f1f9aee 100755 --- a/t/t1350-config-hooks-path.sh +++ b/t/t1350-config-hooks-path.sh @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of specifying core.hooksPath work' test_cmp expect actual ' +test_expect_success 'git rev-parse --git-path hooks' ' + git config core.hooksPath .git/custom-hooks && + git rev-parse --git-path hooks/abc >actual && + test .git/custom-hooks/abc = "$(cat actual)" +' + test_done -- 2.9.2.691.g78954f3 base-commit: 07c92928f2b782330df6e78dd9d019e984d820a7 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html