git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Josh Steadmon <steadmon@google.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com,
	christian.couder@gmail.com, kaartic.sivaraam@gmail.com,
	Johannes.Schindelin@gmx.de, liu.denton@gmail.com
Subject: Re: [PATCH v2 0/3] submodule: port subcommand add from shell to C
Date: Wed, 18 Nov 2020 16:03:33 -0800
Message-ID: <20201119000333.GI36751@google.com> (raw)
In-Reply-To: <20201007074538.25891-1-shouryashukla.oo@gmail.com>

Hi Shourya,

Thank you for this series! Please see the comments below:


On 2020.10.07 13:15, Shourya Shukla wrote:
> Hello all,
> 
> This is the v2 of the patch with the same title, delivered more than a
> month ago as a part of my GSoC. Link to v1:
> https://lore.kernel.org/git/20200824090359.403944-1-shouryashukla.oo@gmail.com/

Since GSoC has ended for the year, I wanted to point out the
git-mentoring@googlegroups.com list, where you can find additional
mentors if you like.

> The changelog is as follows:
> 
>     1. Introduce PATCH[1/3](dir: change the scope of function
>        'directory_exists_in_index()', 2020-10-06). This was done since
>        the above mentioned function will be used in the patch that
>        follows.
> 
>     2. There are multiple changes in this commit:
> 
>             A. Improve the part which checks if the 'path' given as
>                argument exists or not. Implementing Kaartic's
>                suggestions on the patch, I had to make sure that the
>                case for checking if the path has tracked contents or
>                not also works.
> 
>             B. Also, wrap the aforementioned segment in a function
>                since it became very long. The function is called
>                'check_sm_exists()'.
> 
>             C. Also, use the function 'is_nonbare_repository_dir()'
>                instead of 'is_directory()' when trying to resolve
>                gitlink.
> 
>             D. Append keyword 'fatal' in front of the expected output of
>                test t7400.6 since the command die()s out in case of
>                absence of commits in a submodule.
> 
>             E. Remove the extra `#include "dir.h"` from
>                'submodule--helper.c'.
> 
>     3. Introduce PATCH[3/3] (t7400: add test to check 'submodule add'
>        for tracked paths, 2020-10-07). Kaartic pointed out that a test
>        for path with tracked contents did not exist and hence it was
>        necessary to write one. Therefore, this commit introduces a new
>        test 't7400.18: submodule add to path with tracked contents
>        fails'.

Generally, we want to avoid describing in detail what the code does;
hopefully, the code can speak for itself. It may be a better use of the
cover letter to describe the motivation for the series as a whole.
Reviewers will not necessarily have background on what you want to
accomplish. We came up with a few factors that might have inspired this
change, but we're not sure which you intended to address:

* Increase efficiency by reducing the number of processes forked and the
  use of the shell.

* Make the submodule code easier to maintain (since the project probably
  has more C experts than shell experts).

* Improve the user experience with submodules by giving the
  submodule-add code access to C internals, and vice versa.

Knowing what you want to accomplish can make it easier for reviewers. Of
course, you'll also want to include important context in your commit
messages as well, so that it's available in the history if future
debugging is necessary.


Thanks again for the series, and please feel free to follow up if you
have any questions
-- Josh

      parent reply	other threads:[~2020-11-19  0:04 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-07  7:45 Shourya Shukla
2020-10-07  7:45 ` [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()' Shourya Shukla
2020-10-07 18:05   ` Junio C Hamano
2020-10-12 10:11     ` Shourya Shukla
2020-11-18 23:25   ` Emily Shaffer
2020-10-07  7:45 ` [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C Shourya Shukla
2020-10-07 18:37   ` Junio C Hamano
2020-10-07 22:19   ` Junio C Hamano
2020-10-08 17:19   ` Junio C Hamano
2020-10-09  5:09   ` Junio C Hamano
2020-11-18 23:13     ` Jonathan Tan
2020-11-19  7:44       ` Ævar Arnfjörð Bjarmason
2020-11-19 12:38         ` Johannes Schindelin
2020-11-19 20:37           ` Junio C Hamano
2020-10-07  7:45 ` [PATCH v2 3/3] t7400: add test to check 'submodule add' for tracked paths Shourya Shukla
2020-11-19  0:03 ` Josh Steadmon [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=20201119000333.GI36751@google.com \
    --to=steadmon@google.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git