git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: git <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Heba Waly <heba.waly@gmail.com>, Elijah Newren <newren@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [GSoC][RFC][Proposal v4] Convert submodule to builtin
Date: Tue, 24 Mar 2020 10:08:37 +0100	[thread overview]
Message-ID: <CAP8UFD3BMEqt1+gGgGPHdMQ+BzY6Q84rGcp2UNK=WLusYQuy4g@mail.gmail.com> (raw)
In-Reply-To: <20200323171554.5254-1-shouryashukla.oo@gmail.com>

Hi,

On Mon, Mar 23, 2020 at 6:16 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:

> Prathamesh implemented and improved the subcommands status[https://lore.kernel.org/git/20170713200538.25806-4-pc44800@gmail.com/], sync[https://lore.kernel.org/git/20180114211529.6391-2-pc44800@gmail.com/], deinit[https://lore.kernel.org/git/20180114211529.6391-3-pc44800@gmail.com/] and some more. The relevancy of this to my project is that some helper functions(located in submodule.c) such as print_submodule_summary(),prepare_submodule_summary(), etc. have been implemented beforehand. In the case of subcommand summary, use these functions, integrate them with the basic scaffolding(mentioned in the table below) and implement the module_summary() frontend function.

The last sentence above is not very clear to me. Do you mean "In the
case of subcommand summary, I should use these functions, integrate
them ..."? Or maybe  "In the case of subcommand summary, the things
left to do are to use these functions, integrate them ..."?

> He also ported various helper functions such as set_name_rev()[https://lore.kernel.org/git/20170619215025.10086-3-pc44800@gmail.com/]. He kept offering improvements to his conversions till around January of 2018.

Nice!

> Stefan Beller finished the implementation of the subcommand init[https://lore.kernel.org/git/1453418323-29587-1-git-send-email-sbeller@google.com/] as well as laid its foundation[https://lore.kernel.org/git/1453255396-31942-3-git-send-email-sbeller@google.com/]. He implemented foreach[https://lore.kernel.org/git/20180509002952.172347-5-sbeller@google.com/] and improved deinit[https://lore.kernel.org/git/20180327232824.112539-1-sbeller@google.com/] & update[https://lore.kernel.org/git/1444960333-16003-6-git-send-email-sbeller@google.com/] as well. He also ported various helper functions such as resolve_relative_url()[https://lore.kernel.org/git/1460767813-25916-2-git-send-email-sbeller@google.com/].

Nice!

> #### Current Status of the subcommand and future vision
>
> The current status of the conversion as well as the direction I will take for the conversion of the subcommands are as follows:
>
>         add: pending conversion, full code needs to be written for the same. Need to implement callback macros and structures, i.e. struct add_cb,
>              ADD_CB_INIT, as well as frontend function module_add(). Other helper functions may be needed in the process as well. Compare with shell
>              script and try to “translate” it into C. I guesstimate around 400-500 lines of code for this(including helper functions).

Happy to see a guesstimation!

>         set-branch: pending conversion, full code needs to be written for the same. Need to implement macros and structures, i.e. struct setbranch,
>                     SETBRANCH_CB_INIT, as well as frontend function module_setbranch(). Other helper functions(such as remote_submodule_branch() &
>                     get_default_remote() which are already implemented may prove helpful later) may be needed in the process as well. Compare with shell
>                     script and try to “translate” it into C. This subcommand may take about 200 lines of C code to implement(including helper functions).

As it looks like it requires less lines of code, do you think it might
be better to do this one before the 'add' sub-command?

>         set-url: pending conversion, full code needs to be written for the same. Need to implement macros and structures, i.e. struct seturl,
>                  SETURL_CB_INIT, as well as frontend function module_seturl(). Other helper functions(such as remote_url() & resolve_resolve_url() which
>                  are already implemented may prove helpful later) may be needed in the process as well. Compare with shell script and try to “translate” it
>                  into C. It will have a similar implementation to set-branch because they are “setter” functions. This subcommand may take about 200 lines
>                  of C code to implement(including helper functions).
>
>         summary: pending conversion, work in progress; callback structures, functions and macros have been created, also, basic scaffolding of the command
>                  is done, i.e., functions module_summary(), summary_submodule(), summary_submodule_cb(). As this is a prototype, some functions may be scrapped
>                  or added later. Other functions to complement the subcommand have already been created; learn from Prathamesh's mistakes and implement a better code.
>                  After discussions with Junio C Hamano[https://lore.kernel.org/git/20200318163234.21628-1-shouryashukla.oo@gmail.com/T/#ma3912b761b6deda937691a19c1a070e5e9b34bd7],
>                  I intend to add a “--recursive” option as well for summary so as to obtain summaries of nested submodules as well.
>                  I estimate about 400 lines of code for this subcommand(excluding the  “--recursive” option, yet including the helper functions)

Nice!

> I aim to follow the same approach as Stefan and Prathamesh as mentioned above.

I am not sure that you talked much about their approach above.

> Though, there is about a 3 year gap between their work and mine, the model for porting seems to be consistent even if coding style may vary and might even give out improvements over previous implementations.



> #### Contribution process and interaction with the mentors
>
> I will keep committing changes on my GitHub fork[https://github.com/periperidip/git] and finally post a bundled patch on the Mailing List.

By "bundled patch" do you mean a patch series?

> I will make sure to keep interacting with the community as well as the mentors regularly.    I aim to write weekly “progress report” blogs, which I will post on my website as well as the List. Apart from that, I will document anything new I learn as well as my journey in the GSoC program on my blogs and maybe as self-answered questions on StackOverflow with the aim that they will help me as well as others in case of reference.
>
>
> #### Project Timeline
>
> I have been studying the code of 'submodule.c', 'submodule--helper.c' and 'git-submodule.sh'
> since the submission of my microproject. After studying the codes, I tried to devise an effective
> conversion strategy for 'submodule'. I noticed that 'submodule.c' contains various helper functions
> for 'submodule--helper.c' whereas the latter houses the main "converted" command as of now.
>
> The subcommands ‘set-branch’ and ‘set-url’ will provide easy conversion due to the vast array of helper functions already available for them.

Nice that you found this!

> Hence, I have coupled them with other commands and tasks such as the conversion of ‘add’ and improving CLI parsing.

Hmm.

> In spite of the easiness of the aforementioned conversions, I have decided to convert ‘summary’ first because of the large variety of resources available for it

It is not very clear which resources you are talking about.

> as well as a direction of conversion available(as done by Prathamesh)

Do you mean that he already sent patches or wrote commits in his repo
about that?

> which will aid me in learning from the mistakes committed before and thus help me offer an even more improved version of the subcommand.

Thanks,
Christian.

      reply	other threads:[~2020-03-24  9:08 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-23 17:15 [GSoC][RFC][Proposal v4] Convert submodule to builtin Shourya Shukla
2020-03-24  9:08 ` Christian Couder [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP8UFD3BMEqt1+gGgGPHdMQ+BzY6Q84rGcp2UNK=WLusYQuy4g@mail.gmail.com' \
    --to=christian.couder@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=heba.waly@gmail.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=shouryashukla.oo@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).