| Message ID | 20200825113020.71801-4-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: fixup to summary-v3 | expand |
On 25-08-2020 17:00, Shourya Shukla wrote: > The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on > Windows due to a different error message > > error: cannot spawn git: No such file or directory > > instead of > > fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory > > Tighten up the check to compute '{src,dst}_abbrev' by guarding the The change only affects `src_abbrev`. So, it's misleading to mention `dst_abbrev` here. > 'verify_submodule_committish()' call using `p->status !='D'`, so that > the former isn't called in case of non-existent submodule directory, > consequently, there is no such error message on any execution > environment. > > Therefore, eliminate the 'grep' check in t7421. Instead, verify the > absence of an error message by doing a 'test_must_be_empty' on the > file containing the error. > > Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > builtin/submodule--helper.c | 7 ++++--- > t/t7421-submodule-summary-add.sh | 2 +- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 93d0700891..f1951680f7 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg, > static void generate_submodule_summary(struct summary_cb *info, > struct module_cb *p) > { > - char *displaypath, *src_abbrev, *dst_abbrev; > + char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL; Unlike `src_abbrev`, I don't think we need to initilialize `dst_abbrev` to NULL here as it would be assigned in all code paths. > int missing_src = 0, missing_dst = 0; > char *errmsg = NULL; > int total_commits = -1; > @@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info, > } > > if (S_ISGITLINK(p->mod_src)) { > - src_abbrev = verify_submodule_committish(p->sm_path, > - oid_to_hex(&p->oid_src)); > + if (p->status != 'D') > + src_abbrev = verify_submodule_committish(p->sm_path, > + oid_to_hex(&p->oid_src)); > if (!src_abbrev) { > missing_src = 1; > /*
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: >> @@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info, >> } >> >> if (S_ISGITLINK(p->mod_src)) { >> - src_abbrev = verify_submodule_committish(p->sm_path, >> - oid_to_hex(&p->oid_src)); >> + if (p->status != 'D') >> + src_abbrev = verify_submodule_committish(p->sm_path, >> + oid_to_hex(&p->oid_src)); >> if (!src_abbrev) { >> missing_src = 1; >> /* Interesting. There is a mirroring if-else cascade that begins with "if (S_ISGITLINK(p->mod_dst))" immediately after the if-else cascade started here, and in there, the same verify_submodule_committish() is called for oid_dst unconditionally. Should the asymmetry bother readers of the code, or is the source side somehow special and needs extra care?
On 25/08 08:03, Kaartic Sivaraam wrote: > On 25-08-2020 17:00, Shourya Shukla wrote: > > The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on > > Windows due to a different error message > > > > error: cannot spawn git: No such file or directory > > > > instead of > > > > fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory > > > > Tighten up the check to compute '{src,dst}_abbrev' by guarding the > > The change only affects `src_abbrev`. So, it's misleading to mention > `dst_abbrev` here. I forgot to change that. Thank you for pointing this out. > > 'verify_submodule_committish()' call using `p->status !='D'`, so that > > the former isn't called in case of non-existent submodule directory, > > consequently, there is no such error message on any execution > > environment. > > > > Therefore, eliminate the 'grep' check in t7421. Instead, verify the > > absence of an error message by doing a 'test_must_be_empty' on the > > file containing the error. > > > > Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > > --- > > builtin/submodule--helper.c | 7 ++++--- > > t/t7421-submodule-summary-add.sh | 2 +- > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > > index 93d0700891..f1951680f7 100644 > > --- a/builtin/submodule--helper.c > > +++ b/builtin/submodule--helper.c > > @@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg, > > static void generate_submodule_summary(struct summary_cb *info, > > struct module_cb *p) > > { > > - char *displaypath, *src_abbrev, *dst_abbrev; > > + char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL; > > Unlike `src_abbrev`, I don't think we need to initilialize `dst_abbrev` > to NULL here as it would be assigned in all code paths. Alright. Changed!
> Interesting. There is a mirroring if-else cascade that begins with > "if (S_ISGITLINK(p->mod_dst))" immediately after the if-else cascade > started here, and in there, the same verify_submodule_committish() > is called for oid_dst unconditionally. Should the asymmetry bother > readers of the code, or is the source side somehow special and needs > extra care? I understand what you are trying to say. The thing is that the conditional `if (S_ISGITLINK(p->mod_dst))` already guards the `verify_submodule_committish` when we have a status of 'D'. So, we do not need another similar if-statement for that. It does seem a bit weird to someone who is reading this thing for the first time, hence, I will mention this in the commit message. Apologies for the late reply, I was a little busy with something.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 93d0700891..f1951680f7 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg, static void generate_submodule_summary(struct summary_cb *info, struct module_cb *p) { - char *displaypath, *src_abbrev, *dst_abbrev; + char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL; int missing_src = 0, missing_dst = 0; char *errmsg = NULL; int total_commits = -1; @@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info, } if (S_ISGITLINK(p->mod_src)) { - src_abbrev = verify_submodule_committish(p->sm_path, - oid_to_hex(&p->oid_src)); + if (p->status != 'D') + src_abbrev = verify_submodule_committish(p->sm_path, + oid_to_hex(&p->oid_src)); if (!src_abbrev) { missing_src = 1; /* diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh index 59a9b00467..b070f13714 100755 --- a/t/t7421-submodule-summary-add.sh +++ b/t/t7421-submodule-summary-add.sh @@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths' git commit -m "change submodule path" && rev=$(git -C sm rev-parse --short HEAD^) && git submodule summary HEAD^^ -- my-subm >actual 2>err && - grep "fatal:.*my-subm" err && + test_must_be_empty err && cat >expected <<-EOF && * my-subm ${rev}...0000000:
The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on Windows due to a different error message error: cannot spawn git: No such file or directory instead of fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory Tighten up the check to compute '{src,dst}_abbrev' by guarding the 'verify_submodule_committish()' call using `p->status !='D'`, so that the former isn't called in case of non-existent submodule directory, consequently, there is no such error message on any execution environment. Therefore, eliminate the 'grep' check in t7421. Instead, verify the absence of an error message by doing a 'test_must_be_empty' on the file containing the error. Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- builtin/submodule--helper.c | 7 ++++--- t/t7421-submodule-summary-add.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-)