Re: [GSoC][RFC] proposal: convert git-submodule to builtin script

[Khalid Ali <khalludi123@gmail.com>]

First of all, thank you so much for the detailed feedback. I wasn't sure
how much to include in the proposal, but I see it still needs a lot of work.

> When you talk about "Convert each main task in git-submodule into a C
> function." and "If certain functionality is missing, add it to the correct
> script.", it is a good idea to back that up by concrete examples.
>
> Like, study `git-submodule.sh` and extract the list of "main tasks", and
> then mention that in your proposal. I see that you listed 9 main tasks,
> but it is not immediately clear whether you extracted that list from the
> usage text, from the manual page, or from the script itself. If the latter
> (which I think would be the best, given the goal of converting the code in
> that script), it would make a ton of sense to mention the function names
> and maybe add a permalink to the corresponding code (you could use e.g.
> GitHub's permalinks).

Yes, I actually did extract the tasks straight from git-submodule.sh. I will
definitely add the appropriate function names and permalinks to the
proposal.

> And then look at one of those main tasks, think of something that you
> believe should be covered in the test suite, describe it, then figure out
> whether it is already covered. If it is, mention that, together with the
> location, otherwise state which script would be the best location, and
> why.

Ah, alright. I'll have a look at the test suite to see what is covered and
include a section in my proposal.

> Besides, if you care to have a bit of a deeper look into the
> `git-submodule.sh` script, you will see a peculiar pattern in some of the
> subcommands, e.g. in `cmd_foreach`:
> https://github.com/git/git/blob/v2.21.0/git-submodule.sh#L320-L349
>
> Essentially, it spends two handfuls of lines on option parsing, and then
> the real business logic is performed by the `submodule--helper`, which is
> *already* a built-in.
>
> Even better: most of that business logic is implemented in a file that has
> the very file name you proposed already: `submodule.c`.
>
> So if I were you, I would add a section to your proposal (which in the end
> would no doubt dwarf the existing sections) that has as subsections each
> of those commands in `git-submodule.sh` that do *not* yet follow this
> pattern "parse options then hand off to submodule--helper".
>
> I would then study the commit history of the ones that *do* use the
> `submodule--helper` to see how they were converted, what conventions were
> used, whether there were recurring patterns, etc.
>
> In each of those subsections, I would then discuss what the
> still-to-be-converted commands do, try to find the closest command that
> already uses the `submodule--helper`, and then assess what it would take
> to convert them, how much code it would probably need, whether it could
> reuse parts that are already in `submodule.c`, etc.

I definitely noticed the option parsing in multiple parts of the function, but
the pattern didn't click until you mentioned it. I'll do as you recommended
and take a look at submodule.c to see how the code and functionality in
git-submodule.sh can be merged.

> Judging from past projects to convert scripts to C, I would say that the
> most successful strategy was to chomp off manageable parts and move them
> from the script to C. I am sure that you will find tons of good examples
> for this strategy by looking at the commit history of `git-submodule.sh`
> and then searching for the corresponding patches in the Git mailing list
> archive (e.g. https://public-inbox.org/git/).
>
> Do not expect those "chomped off" parts to hit `master` very quickly,
> though. Most likely, you would work on one patch series (very closely with
> your mentor at first, to avoid unnecessary blocks and to get a better feel
> for the way the Git community works right from the start), then, when that
> patch series is robust and solid and ready to be contributed, you would
> send it to the Git mailing list and immediately start working on the next
> patch series, all the while the reviews will trickle in. Those reviews
> will help you to improve the patch series, and it is a good idea to
> incorporate the good suggestions, and to discuss the ones you think are
> not necessary, for a few days before sending the next patch series
> iteration.
>
> Essentially, you will work in parallel on a few patch series at all times.
> Those patch series stack on top of each other, and they should, one after
> the other, make it into `pu` first, then, when they are considered ready
> for testing into `next`, and eventually to `master`. Whenever you
> contribute a new patch series iteration, you then rebase the remaining
> patch series on top. Ideally it will look a bit like a fern, with the
> first patch series being along the farthest, and each subsequent patch
> series at an earlier stage than its predecessor.

Ok, so I'd be doing each of the portions and submitting them to the mailing
list as I finish to let other coders take a look and give feedback.

> Phew. Long mail. Hopefully this amount of information does not scare you.
> And maybe some of it will help you with the proposal and/or the project.

Once again, thank you for the detailed feedback. This really gave me a good
idea of the project as a whole and what I need to include in my proposal.

Sincerely,

-Khalid