Re: [PATCH v1 2/6] convert.c: Remove path when not needed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 2, 2016, 21:32 UTC
- Message-ID
- <xmqqfuxare5x.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1454431990-1980-1-git-send-email-tboegi@web.de>
tboegi@web.de writes:
> Subject: Re: [PATCH v1 2/6] convert.c: Remove path when not needed
At least s/: Remove/: remove/, but I think
s/: Remove.*/: remove unused parameter 'path'/
would be easier to read.
All of these functions seem to have "path" in their name, sounding as if they meant to ask "What is the crlf-ness for THIS PATH?" Is it still sensible to have "path" in their names?
The necessary information that is specific to the path was already gathered when we prepared the 'check' thing, so they are still asking and answering their questions specific to the path by accepting 'check', but it still feels somewhat funny.
With or without "path" in their names, they are horribly misnamed (e.g. "check crlf---are we asking if the file has crlf? if the file must be written out with crlf? something else?").
Your later patches seem to refactor and reorganize this internal calling mess in some way, so let's read on and see how the whole thing improves ;-)
Show 51 quoted lines
> -static enum crlf_action git_path_check_crlf(const char *path, struct git_attr_check *check)
> +static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
> {
> const char *value = check->value;
>
> @@ -713,7 +713,7 @@ static enum crlf_action git_path_check_crlf(const char *path, struct git_attr_ch
> return CRLF_GUESS;
> }
>
> -static enum eol git_path_check_eol(const char *path, struct git_attr_check *check)
> +static enum eol git_path_check_eol(struct git_attr_check *check)
> {
> const char *value = check->value;
>
> @@ -726,8 +726,7 @@ static enum eol git_path_check_eol(const char *path, struct git_attr_check *chec
> return EOL_UNSET;
> }
>
> -static struct convert_driver *git_path_check_convert(const char *path,
> - struct git_attr_check *check)
> +static struct convert_driver *git_path_check_convert(struct git_attr_check *check)
> {
> const char *value = check->value;
> struct convert_driver *drv;
> @@ -740,7 +739,7 @@ static struct convert_driver *git_path_check_convert(const char *path,
> return NULL;
> }
>
> -static int git_path_check_ident(const char *path, struct git_attr_check *check)
> +static int git_path_check_ident(struct git_attr_check *check)
> {
> const char *value = check->value;
>
> @@ -783,12 +782,12 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
> }
>
> if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
> - ca->crlf_action = git_path_check_crlf(path, ccheck + 4);
> + ca->crlf_action = git_path_check_crlf( ccheck + 4);
> if (ca->crlf_action == CRLF_GUESS)
> - ca->crlf_action = git_path_check_crlf(path, ccheck + 0);
> - ca->ident = git_path_check_ident(path, ccheck + 1);
> - ca->drv = git_path_check_convert(path, ccheck + 2);
> - ca->eol_attr = git_path_check_eol(path, ccheck + 3);
> + ca->crlf_action = git_path_check_crlf(ccheck + 0);
> + ca->ident = git_path_check_ident(ccheck + 1);
> + ca->drv = git_path_check_convert(ccheck + 2);
> + ca->eol_attr = git_path_check_eol(ccheck + 3);
> } else {
> ca->drv = NULL;
> ca->crlf_action = CRLF_GUESS;