Re: [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()'

From: Emily Shaffer <emilyshaffer@google.com>
Date: Nov 18, 2020, 23:25 UTC
Message-ID: <20201118232557.GA3698950@google.com>
In-Reply-To: <20201007074538.25891-2-shouryashukla.oo@gmail.com>

Hi,

On Wed, Oct 07, 2020 at 01:15:36PM +0530, Shourya Shukla wrote:

Show 6 quoted lines

> 
> 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