Some left-over add-on for bw/config-h
cover Some left-over add-on for bw/config-h
Back when bw/config-h was developed (and backported to Git for Windows), I came up with this patch. It seems to not be strictly necessary, but I like the safety of falling back to the Git directory when no common directory is configured (for whatever reason).
Johannes Schindelin (1):
do_git_config_sequence(): fall back to git_dir if commondir is NULL
config.c | 2 ++ 1 file changed, 2 insertions(+)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-78%2Fdscho%2Fbw%2Fconfig-h-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-78/dscho/bw/config-h-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/78
Signature · 1 line
gitgitgadget
PATCH 1/1 do_git_config_sequence(): fall back to git_dir if commondir is NULL
From: Johannes Schindelin <[email protected]>
Just some defensive programming.
--- config.c | 2 ++ 1 file changed, 2 insertions(+)
config.c +2 -0
@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct config_options *opts,if (opts->commondir) repo_config =mkpathdup ("%s/config" , opts->commondir); +else if (opts->git_dir) + repo_config =mkpathdup ("%s/config" , opts->git_dir);else repo_config =NULL ;
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
Back when bw/config-h was developed (and backported to Git for Windows), I came up with this patch. It seems to not be strictly necessary, but I like the safety of falling back to the Git directory when no common directory is configured (for whatever reason).
Shouldn't that be diagnosed as an error with BUG()? That is, if we know there is the current repository, and if commondir is not set, isn't it a bug that we want to look into in the start-up sequence?
The phrase "for whatever reason" makes me wonder if this is truly "defensive programming", rather than just sweeping the bug under the rug.
FWIW, with this toy patch applied on top of this patch, all tests that I usually run seem to pass.
config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
config.c +1 -1
@@ -1669,7 +1669,7 @@ static int do_git_config_sequence(const struct config_options *opts,if (opts->commondir) repo_config =mkpathdup ("%s/config" , opts->commondir);else if (opts->git_dir) - repo_config =mkpathdup ("%s/config" , opts->git_dir); +BUG ("git-dir exists but no commondir?" );else repo_config =NULL ;
Quoted 12 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
Back when bw/config-h was developed (and backported to Git for Windows), I came up with this patch. It seems to not be strictly necessary, but I like the safety of falling back to the Git directory when no common directory is configured (for whatever reason).
Shouldn't that be diagnosed as an error with
BUG()? That is, if we know there is the current repository, and if commondir is not set, isn't it a bug that we want to look into in the start-up sequence?
Quoted 6 lines
The phrase "for whatever reason" makes me wonder if this is truly "defensive programming", rather than just sweeping the bug under the rug.
FWIW, with this toy patch applied on top of this patch, all tests that I usually run seem to pass.
Heh, I independently made the same change after reading the patch and came here to report similar results.
I think the key thing here is that git_dir is never meant to be used as a source for config. It is there to let the "includeIf gitdir" directive work. So it would indeed be surprising and a bug if we had one but not the other.
-Peff
v2 cover Some left-over add-on for bw/config-h
In reply to Johannes Schindelin via GitGitGadgetBack when bw/config-h was developed (and backported to Git for Windows), I came up with a patch to use git_dir if commondir is NULL, and contributed that as v1 of this patch. However, it was deemed a bug if that happens, so let's instead detect that condition and report it.
Change since v1:
* Be loud about this bug instead of papering over it.
Johannes Schindelin (1):
config: report a bug if git_dir exists without commondir
config.c | 2 ++ 1 file changed, 2 insertions(+)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-78%2Fdscho%2Fbw%2Fconfig-h-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-78/dscho/bw/config-h-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/78
Range-diff vs v1:
1: a3854e4ed8 ! 1: 0767f98378 do_git_config_sequence(): fall back to git_dir if commondir is NULL
@@ -1,8 +1,9 @@
Author: Johannes Schindelin <[email protected]>
- do_git_config_sequence(): fall back to git_dir if commondir is NULL
+ config: report a bug if git_dir exists without commondir
- Just some defensive programming.
+ This did happen at some stage, and was fixed relatively quickly. Make
+ sure that we detect very quickly, too, should that happen again.
Signed-off-by: Johannes Schindelin <[email protected]>
@@ -14,7 +15,7 @@
if (opts->commondir)
repo_config = mkpathdup("%s/config", opts->commondir);
+ else if (opts->git_dir)
-+ repo_config = mkpathdup("%s/config", opts->git_dir);
++ BUG("git_dir without commondir");
else
repo_config = NULL;Signature · 1 line
gitgitgadget
PATCH v2 1/1 config: report a bug if git_dir exists without commondir
From: Johannes Schindelin <[email protected]>
This did happen at some stage, and was fixed relatively quickly. Make sure that we detect very quickly, too, should that happen again.
--- config.c | 2 ++ 1 file changed, 2 insertions(+)
config.c +2 -0
@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct config_options *opts,if (opts->commondir) repo_config =mkpathdup ("%s/config" , opts->commondir); +else if (opts->git_dir) +BUG ("git_dir without commondir" );else repo_config =NULL ;
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/1] config: report a bug if git_dir exists without commondir
Quoted 15 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
This did happen at some stage, and was fixed relatively quickly. Make sure that we detect very quickly, too, should that happen again.
Signed-off-by Johannes Schindelin <[email protected]>--- config.c | 2 ++ 1 file changed, 2 insertions(+)config.c +0
-0
config.c static int do_git_config_sequence(const struct config_options *opts, · line 1680
Quoted 5 lines
config.c +2
-0@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct config_options *opts,if (opts->commondir) repo_config =mkpathdup ("%s/config" , opts->commondir); +else if (opts->git_dir) +BUG ("git_dir without commondir" );
Yeah, I think this is the right thing to do. Thanks!
-Peff