[GSoC][PATCH 0/4] submodule: port 'summary' from Shell to C
- From
Shourya Shukla <shouryashukla.oo@gmail.com>
- Date
- Jul 2, 2020, 19:24 UTC
- Message-ID
- <20200702192409.21865-1-shouryashukla.oo@gmail.com>
Hello all!
This is third subcommand in line to be converted as a part of my GSoC project. The other two were 'set-url' and 'set-branch' both of which have been merged into Git. This port has been taken off from what Prathamesh Chavan left as a part of his GSoC project in 2017 under Stefan Beller and Christian Couder. As written in my proposal and advised by my mentors Kaartic and Christian, it was decided to re-use PC's work for multiple reasons.
Here is the link to the v1 of his patch: https://lore.kernel.org/git/20170731205621.24305-9-pc44800@gmail.com/
Here is the link to the v2 of his patch: https://lore.kernel.org/git/20170807211900.15001-9-pc44800@gmail.com/
Even though this patch series draws on Prathamesh' work, a lot extra features had to be added along with some remodelling. A summary of the 4 commits in this patch are as follows: the patch consists of 3 preparatory patches preceding the main commit porting 'git submodule summary'.
Being a bit more verbose about the prepatory patches:
1. a8dcc59a6a amends an extra linefeed between callback struct(s)
macro(s) of subcommands 'init', 'status' and 'sync' so as to make
the whole format of subcommands a bit more uniform and
consistent. 2. 5f17ca37e0 renames the helper functions
'show_submodule_summary()', 'prepare_submodule_summary()' and
'print_submodule_summary()' used by the 'builtin_diff()' function
of diff.c This was done so as to avoid any ambiguity later on
when the helper functions used by 'summary' will be defined in
the upcoming port. The functions are renamed to
'*_diff_submodule_summary()' so as to classify them as the one
being used by the diff machinery. 3. 5945ab4113 changes the scope of the 'count_lines()' function
defined in diff.c so as to make it usable by other entities of
the code. This was originally a patch authored by PC with a small
change that the function instead of being defined as an 'extern
int', it is now defined as 'int' since f758a7f8ac4 by Nguyễn Thái
Ngọc Duy removed extern from the function declarations in diff.hNow moving on to the main patch 4/4 that will port summary.
17738e9df4 ports the subcommand summary from Shell to C. I will not
be repetitive by stating how all the functions communicate since
this is covered in the commit message. What I will focus on is the
bits I had to tweak along with any extra functions added. Originally
the functions were: module_summary(),
compute_summary_submodule_list(), submodule_summary_callback(),
prepare_submodule_summary(), generate_submodule_summary(),
print_submodule_summary() and verify_submodule_object_name. I had to
add another function called get_diff_cmd() which fetches a const
char * (a diff-index or a diff-files) in response to the enum provided
to it and bugs out in case of an unexpected enum value. I also had
to tweak the enum declared by PC by NOT keeping it static and
tweaking its definition just a little. These were suggested by
Christian on PC's patch at the time. There were also some changes to function parameters such as those
of index_fd() by passing the the_index parameter to it or the case
of submodule_from_path() where it was needed to add another parameter
of the_repository. This all must have been due to change of function
defintions in the past 3 years. Even after all this, there were two problems: t7418 had been failing
(something which I feel might have been overlooked/not tested by PC
during his time) and the CI build failed. First let's cover the test
failure. The main test for testing summary is t7401. t7418 is another test
which involves sparse checkouts and submodules. The test creates a
sparse checkout clone excluding the .gitmodules file to test the
functionality of the '--for-status' option. After quite a thorough
investigation me, Kaartic and Christian figured out that this port
fails to handle cases where a gitfile exists instead of a .git dir
since all the tests in t7401 did not any submodules which were
cloned in any way but rather just exist in a nearby directory and
are used in our tests (this is something I feel should be fixed and
hence I am saving this for later). But anyway, we decided that it
will be excellent to check for a non-bare repo instead of checking
for a gitdir therefore replacing the function is_git_directory()
with is_nonbare_repository() in the first non-nested if-check in
generate_submodule_summary(). Regarding the CI failure. The build was failing due to the
function oidcmp() in the first if-check of
generate_submodule_summary(). The CI build suggested that we use
oideq() instead so that the build passes and on doing the change, it
did! There were some other cosmetic changes such as wrapping of
columns upto 80 chars (to follow the CodingGuidelines) and
improving some error messages and option descriptions,Finally, since it was originally PC's work, I have kept him as the author for the final 2 commits and kept the date as-is (from 2017). This patch is not perfect and hence needs the feedback from all of you :)
Here is a 'git log --oneline' of the commits:
17738e9df4 submodule: port submodule subcommand 'summary' from shell to C
5945ab4113 diff: change scope of the function count_lines()
5f17ca37e0 submodule: rename helper functions to avoid ambiguity
a8dcc59a6a submodule: amend extra line feed between callback struct and macroThanks, Shourya Shukla
Prathamesh Chavan (2): diff: change scope of the function count_lines() submodule: port submodule subcommand 'summary' from shell to C
Shourya Shukla (2): submodule: amend extra line feed between callback struct and macro submodule: rename helper functions to avoid ambiguity
builtin/submodule--helper.c | 454 +++++++++++++++++++++++++++++++++++- diff.c | 4 +- diff.h | 1 + git-submodule.sh | 186 +-------------- submodule.c | 10 +- submodule.h | 2 +- 6 files changed, 461 insertions(+), 196 deletions(-)
-- 2.27.0