From: Emily Shaffer <firstname.lastname@example.org>
To: Shourya Shukla <email@example.com>
Cc: firstname.lastname@example.org, email@example.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)
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
The same comment holds true for your patch 3, as well.
Thanks for your effort on this series.
next prev 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
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:
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 \
* 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
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).