Shourya ShuklaGreetings,
This is the v2 of the previous patch series with the same title. The v1 received some comments from Junio and Kaartic. The following changes were made:
PATCH[3/3] received the comment that it had an unnecessary
'char *dst_abbrev = NULL' which had to be reverted to 'char
*dst_abbrev' since the assignment was pretty much useless.
The commit message also needed some changes in the sense that it
stated that the change of guarding the
'verify_submodule_committish()' call was made for dst_abbrev as well
which wasn't the case. Junio also suggested to clarify the reason
for not having the guard in case of 'dst_abbrev'.Another thing which came up was the cleanup of 'submodule--helper.c'. IT started with Junio commenting on PATCH[2/3] 'submodule: fix style in function definition'. He asked me to verify if there are any other similar faults regarding function or variable defintions which had a faulty asterisk placement. I did some digging and found a fault here in submodule--helper.c:
static char *compute_rev_name(const char *sub_path, const char* object_id)
As yiou may notice, the correction should be 's/static char */static char* /. I also did some further digging and found that there we some other minor faults in the option descriptions of various subcommands. For instance in module_foreach:
struct option module_foreach_options[] = {
OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
OPT_BOOL(0, "recursive", &info.recursive,
N_("Recurse into nested submodules")),
OPT_END()
};The option descriptions should start in lowercase but they start with a capital letter. This convention is mentioned in L267-270 of 'api-parse-options.txt'. There are other such small violations such as die() messages starting with a captial letter.
I will do this cleanup after some time when I am a bit free since I have some personal engagements right now. Or something even better could be to add this as a 'good first issue' on gitgitgadget/git so that a newcomer can be something relatively easy and get familiar with the way work is done at Git. Please do tell what seems more fitting to you. Also, to be clear, I am not suggesting the latter out of laziness.
I am attaching a range-diff b/w v1 and v2 below for ease of review. Feedback and reviews are appreciated.
Regards, Shourya Shukla
----- range-diff:
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit messagefatal: 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
+ Tighten up the check to compute 'src_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.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'. Therefore, eliminate the 'grep' check in t7421. Instead, verify the
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submo
dule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp
atibility
@@ Commit messagefatal: 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
:...skipping...
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit messagefatal: 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
+ Tighten up the check to compute 'src_abbrev' by guarding the
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit message
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
+ Tighten up the check to compute 'src_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.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submo
dule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp
atibility
@@ Commit message
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
:...skipping...
1: a22ffa950f = 1: 768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2: 32934998ee = 2: 35664360ac submodule: fix style in function definition
3: 82e0956cd2 ! 3: f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
@@ Commit message
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
+ Tighten up the check to compute 'src_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.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
absence of an error message by doing a 'test_must_be_empty' on the
@@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_
struct module_cb *p)
{
- char *displaypath, *src_abbrev, *dst_abbrev;
-+ char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
++ char *displaypath, *src_abbrev = NULL, *dst_abbrev;
int missing_src = 0, missing_dst = 0;
char *errmsg = NULL;
int total_commits = -1;
~
~
~
~
~
~
~
~
~ '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.
+ environment. The same need not be implemented for 'dst_abbrev' and is
+ rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
+ already guards the `verify_submodule_committish` when we have a status
+ of 'D'. Therefore, eliminate the 'grep' check in t7421. Instead, verify the
absence of an error message by doing a 'test_must_be_empty' on the
@@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_
struct module_cb *p)
{
- char *displaypath, *src_abbrev, *dst_abbrev;
-+ char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
++ char *displaypath, *src_abbrev = NULL, *dst_abbrev;
int missing_src = 0, missing_dst = 0;
char *errmsg = NULL;
int total_commits = -1;
~
~
~
~
~
~
~
~
~
-----Shourya Shukla (3): submodule: eliminate unused parameters from print_submodule_summary() submodule: fix style in function definition t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
builtin/submodule--helper.c | 17 ++++++++--------- t/t7421-submodule-summary-add.sh | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-)
-- 2.28.0