git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Emily Shaffer <emilyshaffer@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 1/3] dir: change the scope of function 'directory_exists_in_index()'
Date: Wed, 18 Nov 2020 15:25:57 -0800	[thread overview]
Message-ID: <20201118232557.GA3698950@google.com> (raw)
In-Reply-To: <20201007074538.25891-2-shouryashukla.oo@gmail.com>

Hi,

On Wed, Oct 07, 2020 at 01:15:36PM +0530, Shourya Shukla wrote:
> 
> Change the scope of the function 'directory_exists_in_index()' as well
> as declare it in 'dir.h'.
> 
> Since the return type of the function is the enumerator 'exist_status',
> change its scope as well and declare it in 'dir.h'.

I don't have comments about the diff itself beyond what Junio mentioned
- it's very simple. But I do think this commit message needs a rewrite.

Your commit message summarizes the diff - which isn't useful, because
the diff itself is very simple. But what it fails to do is what I'm a
lot more interested in, reading this change: *why* do you want to make
this function and enum reusable? I think you mention it in the cover
letter, but it's not explained at all here.

Explaining the motivation in the cover letter also would help us
understand whether it is better to make the enum public, like your diff
proposes, or to wrap or change the function and avoid exposing the enum,
like you suggested in reply to Junio's comment.

Lastly, saying something like "This change is needed so that git commit
can sort ducks by feather length" helps avoid
https://en.wikipedia.org/wiki/XY_problem - that is, maybe we already
have another tool which is more appropriate, and which you missed; and
knowing your motivation, someone can point you in that direction
instead.

The same comment holds true for your patch 3, as well.

Thanks for your effort on this series.

 - Emily

  parent reply	other threads:[~2020-11-18 23:28 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-07  7:45 [PATCH v2 0/3] submodule: port subcommand add from shell to C 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 [this message]
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 ` [PATCH v2 0/3] submodule: port subcommand add from shell to C Josh Steadmon

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=20201118232557.GA3698950@google.com \
    --to=emilyshaffer@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 \
    --subject='Re: [PATCH v2 1/3] dir: change the scope of function '\''directory_exists_in_index()'\''' \
    /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

Code repositories for project(s) associated with this 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).