Hi Atharva,
On 11/04/21 3:10 pm, Atharva Raykar wrote:
On 10-Apr-2021, at 18:29, Christian Couder <christian.couder@xxxxxxxxx> wrote:
On Thu, Apr 8, 2021 at 12:19 PM Atharva Raykar <raykar.ath@xxxxxxxxx> wrote:
Here's my updated draft. Changes since v1:
- Elaborated more on example porting strategy, stating how the patches
could be broken up.
- Made language at the end of section 6 less ambiguous.
- Updated status of microproject.
- s/git/Git in several places.
Thanks for this summary of the changes since the previous version!
Yeah. Summaries are really helpful :)
[ ... ]
This
would perform the work being done by the shell script past the flags
being parsed and make the necessary call to `update_clone()', which
returns information about the cloned modules.
How does it return information?
For each cloned module,
it will find out the update mode through `module_update_mode()', and
run the appropriate operation according to that mode (like a rebase,
if that was the update mode).
One possible way this work can be broken up into multiple patches, is
by moving over the shell code into C in a bottom-up manner.
For example: The shell part which retrieves the latest revision in the
remote (if --remote is specified) can be wrapped into a command of
`submodule--helper.c'.
Could you give an example of how the command would be named, what
arguments it would take and how it could be used?
Then we can move the part where we run the
update method (ie the `case' on line 611 onwards) into a C function.
Do you mean the code that does something like:
case "$update_module" in
checkout)
...
rebase)
...
merge)
...
!*)
...
*)
...
esac
if (sanitize_submodule_env; cd "$sm_path" &&
$command "$sha1")
then
say "$say_msg"
elif test -n "$must_die_on_failure"
then
die_with_status 2 "$die_msg"
else
err="${err};$die_msg"
continue
fi
?
Could you also give an example of how the command would be named, what
arguments it would take and how it could be used?
I could add more detail about the exact arguments each converted part
would take, but I feel a little hesitant because I will most likely
change my mind on a lot of those kind of lower-level decisions as I
understand the codebase better. The point I was trying to convey is
that the high-level workflow I would follow while converting would look
like this:
1. Identify parts in git-submodule.sh that have cohesive functionality
2. Rewrite that functionality in C, which can be invoked from
`git submodule--helper <function name> <args>`
3. Remove the shell code and replace it with the above invocation
4. Once the shell code is reduced to only a bunch of calls to
submodule--helper, wrap all of that into one call that looks like
`git submodule--helper update <flags>` that encapsulates all the
functionality done by the other helper function calls.
(In other words: I will cluster the functionality in a bottom-up way.
Maybe I should mention the above four points in my proposal?)
That sounds like a good idea which wouldn't result in one huge patch and
thus avoids reviewer fatigue.
The example I gave for how to handle the presence of the remote flag
and the function that performs the module updation method (ie, the `case`
on line 611) was just to illustrate the above workflow, rather than say
that this is how I will exactly do it.
I also would like to know what level of granularity is ideal for the
proposal. For now I have tried to keep it at "whatever I will surely
follow through when I work on the project", which at the moment is the
covered by the four points I mentioned above.
If I go too much into detail about the functions and arguments
of every helper in my example, I will feel compelled to do the same for
the `git submodule add` example. I also will have to reason more carefully
because I do not want to end up in a situation where I do not actually
stick to my proposal all that much, because I realise in my investigation
phase that there is a different, much better way.
Do let me know what is preferred.
It makes sense that you don't want to go into too much detail in your
proposal. I think Christian wasn't expecting it either. As far as I
understand, he was just trying to make your proposal clear to the person
who reads it. Just mentioning something like,
This would perform the work being done by the shell script past the
flags being parsed and make the necessary call to `update_clone()',
which returns information about the cloned modules.
is not clear as it doesn't say how you're "thinking" the function would
return information. Mention this would be helpful for the reader to know
what your expectations are and if they need any correction. So, it is
better to mention such related information to make your proposal
complete. The high-level flow looks good to me.
Also, I believe Christian would correct me in case I got anything
wrong :)
--
Sivaraam
