Re: [PATCH 01/10] bisect--helper: introduce new `write_in_file()` function

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2020, 19:06 UTC
Message-ID: <xmqq4kvdjhhc.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200226101429.81327-2-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

Show 5 quoted lines

> Let's refactor code adding a new `write_in_file()` function
> that opens a file for writing a message and closes it.
>
> This removes some duplicated code and makes the code simpler,
> clearer and easier to understand.

I find it a bit too much self promotion ;-) With what we see only in this step, the code is not so much simpler nor easier to understand. Perhaps when we see more callers in the remainder of the series, we may start to appreciate it, but if that is the case, we should clarify it here, e.g.

    This helper will be used in later steps and makes the code
    simpler, ...

Show 10 quoted lines

> +static int write_in_file(const char *filepath, const char *content, int append)
> +{
> +	FILE *fp = NULL;
> +	const char *mode = append ? "a" : "w";
> +
> +	fp = fopen(filepath, mode);
> +	if (!fp)
> +		return error_errno(_("could not open the file '%s'"), filepath);
> +	if (!fprintf(fp, "%s\n", content))
> +		return error_errno(_("could not write in file '%s'"), filepath);

Use and non-use of definite article being inconstent between "open the file X" vs "write in file X" bothers me as a reader here.

Wouldn't it be more common to say "write TO" here, not "write IN"?

> +	return fclose(fp);

We didn't use to say error_errno(_("could not close ...")) in the original, so we stay silent and return an error (EOF, which is a negative integer) from here.

Show 25 quoted lines

> +}
> +
>  static int check_term_format(const char *term, const char *orig_term)
>  {
>  	int res;
> @@ -104,7 +117,7 @@ static int check_term_format(const char *term, const char *orig_term)
>  
>  static int write_terms(const char *bad, const char *good)
>  {
> -	FILE *fp = NULL;
> +	char *content = xstrfmt("%s\n%s", bad, good);
>  	int res;
>  
>  	if (!strcmp(bad, good))
> @@ -113,12 +126,9 @@ static int write_terms(const char *bad, const char *good)
>  	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
>  		return -1;
>  
> -	fp = fopen(git_path_bisect_terms(), "w");
> -	if (!fp)
> -		return error_errno(_("could not open the file BISECT_TERMS"));
> +	res = write_in_file(git_path_bisect_terms(), content, 0);
> +	free(content);
>  
> -	res = fprintf(fp, "%s\n%s\n", bad, good);

With just this one callsite, I do not think it is a good idea to lose the "write a formatted output without an extra allcoation" by introducing this wrapper. The equation may become different if we see more (a lot more) callsites that already have strings ready to be written that can use this helper, though.

> -	res |= fclose(fp);
>  	return (res < 0) ? -1 : 0;

The return value is a bit strange here, but it is inherited from the original, so I'll let it pass.

>  }