| Message ID | 20201007074538.25891-2-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: port subcommand add from shell to C | expand |
Shourya Shukla <shouryashukla.oo@gmail.com> writes: > diff --git a/dir.h b/dir.h > index a3c40dec51..e46f240528 100644 > --- a/dir.h > +++ b/dir.h > @@ -370,6 +370,15 @@ int read_directory(struct dir_struct *, struct index_state *istate, > const char *path, int len, > const struct pathspec *pathspec); > > +enum exist_status { > + index_nonexistent = 0, > + index_directory, > + index_gitdir > +}; These were adequate as private names used within the wall of dir.c, but I doubt that they are named specific enough to stand out as public symbols. Unlike say "index_state" (the name of a struct type), whose nature is quite global to any code that wants to access the in-core index, "index_directory" is *NOT* such a name. It is only of interest to those who want to see "I have a directory name---does it appear as a directory in the index?". It is not even interesting to those who want to ask similar and related questions like "I have this pathname---does it appear as anything in the index, and if so what type of entry is it?". A worse part of this is that even if such a helper function file_exists_in_index() were to be written, the "exist_status" enum won't be usable to return the answer that question, but yet the enum squats on a perfectly good name to express "status" for the whole class that it does not represent. So, NAK. We need to come up with a better name for these symbols if we were to expose them to the outside world. The only good name this patch makes public is "directory_exists_in_index()", which is specific enough. > +enum exist_status directory_exists_in_index(struct index_state *istate, > + const char *dirname, int len); > + > enum pattern_match_result { > UNDECIDED = -1, > NOT_MATCHED = 0,
On 07/10 11:05, Junio C Hamano wrote: > Shourya Shukla <shouryashukla.oo@gmail.com> writes: > > > diff --git a/dir.h b/dir.h > > index a3c40dec51..e46f240528 100644 > > --- a/dir.h > > +++ b/dir.h > > @@ -370,6 +370,15 @@ int read_directory(struct dir_struct *, struct index_state *istate, > > const char *path, int len, > > const struct pathspec *pathspec); > > > > +enum exist_status { > > + index_nonexistent = 0, > > + index_directory, > > + index_gitdir > > +}; > > These were adequate as private names used within the wall of dir.c, > but I doubt that they are named specific enough to stand out as > public symbols. > > Unlike say "index_state" (the name of a struct type), whose nature > is quite global to any code that wants to access the in-core index, > "index_directory" is *NOT* such a name. It is only of interest to > those who want to see "I have a directory name---does it appear as a > directory in the index?". It is not even interesting to those who > want to ask similar and related questions like "I have this > pathname---does it appear as anything in the index, and if so what > type of entry is it?". A worse part of this is that even if such a > helper function file_exists_in_index() were to be written, the > "exist_status" enum won't be usable to return the answer that > question, but yet the enum squats on a perfectly good name to > express "status" for the whole class that it does not represent. > > So, NAK. We need to come up with a better name for these symbols if > we were to expose them to the outside world. The only good name > this patch makes public is "directory_exists_in_index()", which is > specific enough. Understandable. So, how would it be if the function I wrote in 'submodule--helper.c' could instead be written here (in dir.c) and would help to check if the path is there in the index or not. It could be anything ranging from a filename to a SM. Since the return type will be an integer, this PATCH 1/3 can be eliminated and we won't need to change the scope of the 'directory_exists_in_index()' function and the enum can stay visible only in dir.c What are your thoughts on this?
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
diff --git a/dir.c b/dir.c index 78387110e6..e67cf52fec 100644 --- a/dir.c +++ b/dir.c @@ -1655,12 +1655,6 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir, return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len); } -enum exist_status { - index_nonexistent = 0, - index_directory, - index_gitdir -}; - /* * Do not use the alphabetically sorted index to look up * the directory name; instead, use the case insensitive @@ -1688,8 +1682,8 @@ static enum exist_status directory_exists_in_index_icase(struct index_state *ist * the files it contains) will sort with the '/' at the * end. */ -static enum exist_status directory_exists_in_index(struct index_state *istate, - const char *dirname, int len) +enum exist_status directory_exists_in_index(struct index_state *istate, + const char *dirname, int len) { int pos; diff --git a/dir.h b/dir.h index a3c40dec51..e46f240528 100644 --- a/dir.h +++ b/dir.h @@ -370,6 +370,15 @@ int read_directory(struct dir_struct *, struct index_state *istate, const char *path, int len, const struct pathspec *pathspec); +enum exist_status { + index_nonexistent = 0, + index_directory, + index_gitdir +}; + +enum exist_status directory_exists_in_index(struct index_state *istate, + const char *dirname, int len); + enum pattern_match_result { UNDECIDED = -1, NOT_MATCHED = 0,
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'. Helped-by: Christian Couder <christian.couder@gmail.com> Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- dir.c | 10 ++-------- dir.h | 9 +++++++++ 2 files changed, 11 insertions(+), 8 deletions(-)