Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

Hi Ævar,

On Thu, 19 Nov 2020, Ævar Arnfjörð Bjarmason wrote:

>
> On Thu, Nov 19 2020, Jonathan Tan wrote:
>
> >> Whew.
> >>
> >> This was way too big to be reviewed in a single sitting.  I do not
> >> know offhand if there is a better way to structure the changes into
> >> a more digestible pieces to help prevent reviewers from overlooking
> >> potential mistakes, though.
> >>
> >> Thanks.
> >
> > I just took a look at this, and one thing that would have helped is if
> > you ported the end of the function first in a commit, and work your way
> > backwards (in one or more commits).
> >
> > After reading through the whole thing, I saw that this is mostly a
> > straightforward start-to-finish port (besides factoring out code into
> > functions), but it would be much easier for reviewers to conceptualize
> > and discuss the different parts if they were already divided.
>
> Having done some minor changes to git-submodule.sh recently, I wondered
> if we weren't at the point where it would be a nice approach to invert
> the C/sh helper relationship.
>
> I.e. write git-submodule.c, which would be the small entry point, it
> would then mostly dispatch to a submodule--helper, which would in turn
> mostly dispatch to a new submodule--helper-sh (containing most of the
> current git-submodule.sh code), which in turn would re-dispatch to the C
> submodule--helper (which as an aside, then sometimes calls itself via
> process invocation).
>
> It's quite a bit of spaghetti code, but means that there's a straighter
> path to porting some of the setup code such as the "--check-writeable",
> is_absolute_path() etc. being changed at the start of the change here to
> git-submodule.sh.

Looking at
https://github.com/gitgitgadget/git/blob/ss/submodule-add-in-c/git-submodule.sh,
I see that while there are still 794 lines, most of it is just mostly
redundant option parsing. The only function left to convert is
`cmd_update()`, and the first half was already converted to C long ago,
via the `git submodule--helper update-clone` subcommand:
https://github.com/gitgitgadget/git/blob/ss/submodule-add-in-c/git-submodule.sh#L269-L530

At this stage I suspect that having a little more patience would make more
sense. After `cmd_update` is converted, we can simply finish the
conversion, without having to keep the shell script around.

Ciao,
Dscho