| Message ID | 20220301002613.1459916-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | teach submodules to know they're submodules | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > diff --git a/submodule.c b/submodule.c > index 741104af8a..463e7f0c48 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -2237,6 +2237,7 @@ int get_superproject_working_tree(struct strbuf *buf) > struct strbuf sb = STRBUF_INIT; > struct strbuf one_up = STRBUF_INIT; > const char *cwd = xgetcwd(); > + int has_superproject_cfg = 0; > int ret = 0; > const char *subpath; > int code; > @@ -2250,6 +2251,17 @@ int get_superproject_working_tree(struct strbuf *buf) > */ > return 0; > > + if (git_config_get_bool("submodule.hassuperproject", &has_superproject_cfg) > + || !has_superproject_cfg) { git_config_get_bool() returns 0 when it successfully finds the variable, so the above says "If submodule.hasSuperproject is not set at all, or if it is set to false, then..." > + /* > + * If we don't have a superproject, then we're probably not a > + * submodule. If this is failing and shouldn't be, investigate > + * why the config was never set. > + */ > + error(_("Asked to find a superproject, but submodule.hasSuperproject != true")); > + return 0; But given that this thing is new, I am not sure if that is a sensible guard to use here. Shouldn't we say - If submodule.hasSuperproject is EXPLICITLY set to false then ... instead? I.e. if (!git_config_get_bool("submodule.hassuperproject", &value) && !value) { error(_("asked to ...")); return 0; }
On Mon, Feb 28, 2022 at 11:06:07PM -0800, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > diff --git a/submodule.c b/submodule.c > > index 741104af8a..463e7f0c48 100644 > > --- a/submodule.c > > +++ b/submodule.c > > @@ -2237,6 +2237,7 @@ int get_superproject_working_tree(struct strbuf *buf) > > struct strbuf sb = STRBUF_INIT; > > struct strbuf one_up = STRBUF_INIT; > > const char *cwd = xgetcwd(); > > + int has_superproject_cfg = 0; > > int ret = 0; > > const char *subpath; > > int code; > > @@ -2250,6 +2251,17 @@ int get_superproject_working_tree(struct strbuf *buf) > > */ > > return 0; > > > > + if (git_config_get_bool("submodule.hassuperproject", &has_superproject_cfg) > > + || !has_superproject_cfg) { > > git_config_get_bool() returns 0 when it successfully finds the > variable, so the above says "If submodule.hasSuperproject is not set > at all, or if it is set to false, then..." > > > + /* > > + * If we don't have a superproject, then we're probably not a > > + * submodule. If this is failing and shouldn't be, investigate > > + * why the config was never set. > > + */ > > + error(_("Asked to find a superproject, but submodule.hasSuperproject != true")); > > + return 0; > > But given that this thing is new, I am not sure if that is a > sensible guard to use here. Shouldn't we say > > - If submodule.hasSuperproject is EXPLICITLY set to false then ... Ah, interesting. I think that makes sense. I wrote this patch hoping to get an additional check for completeness of the previous patch (that is - if I can rely on that value for this other operation, and all the tests still pass without me touching them, then I seem to have wired the new config through everywhere) and I think it's served that purpose; for the real world, that's a little more dangerous, so I think relying on explicit false makes sense. Will do. > > instead? I.e. > > if (!git_config_get_bool("submodule.hassuperproject", &value) && > !value) { > error(_("asked to ...")); > return 0; > } >
diff --git a/submodule.c b/submodule.c index 741104af8a..463e7f0c48 100644 --- a/submodule.c +++ b/submodule.c @@ -2237,6 +2237,7 @@ int get_superproject_working_tree(struct strbuf *buf) struct strbuf sb = STRBUF_INIT; struct strbuf one_up = STRBUF_INIT; const char *cwd = xgetcwd(); + int has_superproject_cfg = 0; int ret = 0; const char *subpath; int code; @@ -2250,6 +2251,17 @@ int get_superproject_working_tree(struct strbuf *buf) */ return 0; + if (git_config_get_bool("submodule.hassuperproject", &has_superproject_cfg) + || !has_superproject_cfg) { + /* + * If we don't have a superproject, then we're probably not a + * submodule. If this is failing and shouldn't be, investigate + * why the config was never set. + */ + error(_("Asked to find a superproject, but submodule.hasSuperproject != true")); + return 0; + } + if (!strbuf_realpath(&one_up, "../", 0)) return 0;
In the previous commit, submodules learned a config 'submodule.hasSuperproject' to indicate whether or not we should attempt to traverse the filesystem to find their superproject. To help test that this config was added everywhere it should have been, begin using it to decide whether to exit early from 'git rev-parse --show-superproject-working-dir'. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Maybe it's actually better to warn instead of error here? Or maybe it's best not to say anything, but to set 'submodule.hasSuperproject' after we successfully find the superproject? Either way - I ran the test suite with this early exit added and everything still passed. I made this change hoping to get a little signal on whether the series achieved its goal, and in that regard I'm satisfied. --- submodule.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)