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

[Junio C Hamano <gitster@pobox.com>]

Miriam Rubio <mirucam@gmail.com> writes:

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

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

> +}
> +
>  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.

>  }