From: Emily Shaffer <firstname.lastname@example.org> To: Shourya Shukla <email@example.com> Cc: firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, Johannes.Schindelin@gmx.de, firstname.lastname@example.org 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: <email@example.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
next prev parent reply other threads:[~2020-11-18 23:26 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 * 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 \ --firstname.lastname@example.org \ --cc=Johannes.Schindelin@gmx.de \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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
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 NNTP newsgroup(s).