Re: [PATCH v4 03/13] bisect--helper: introduce new `write_in_file()` function

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

Miriam Rubio <mirucam@gmail.com> writes:

> +static int write_in_file(const char *path, const char *mode, const char *format, va_list args)
> +{
> +	FILE *fp = NULL;
> +	int res = 0;
> +
> +	if (!strcmp(mode, "a") && !strcmp(mode, "w"))
> +		return error(_("wrong writing mode '%s'"), mode);

How can a string "mode" be equal to "a" and "w" at the same time?

The only caller you have at this point passes "w" and nothing else.
It may be easier to follow the evolution of the code in the series 
if you left this as

	if (strcmp(mode, "w"))
		BUG("write-in-file does not support '%s' mode", mode);

at this step, and turned it into

	if (strcmp(mode, "w") && strcmp(mode, "a"))
		BUG("write-in-file does not support '%s' mode", mode);

in the step where a caller starts to call it for appending.

> +	fp = fopen(path, mode);
> +	if (!fp)
> +		return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
> +	res = vfprintf(fp, format, args);
> +
> +	if (!res) {

Shouldn't this check for negative return?  It is not an error to
pass format that ends up writing nothing, but any error should be
reported by returning a negative value from vfprintf(), I would
think.

    (a tip for a student/mentee)

    Any time you call a system library you are not familiar with,
    make it a habit to always consult its manual page for return
    values and its error reporting convention.

>  	if (!strcmp(bad, good))
> @@ -113,12 +144,8 @@ 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_to_file(git_path_bisect_terms(), "%s\n%s\n", bad, good);
>  
> -	res = fprintf(fp, "%s\n%s\n", bad, good);
> -	res |= fclose(fp);
>  	return (res < 0) ? -1 : 0;

Shouldn't this now be just

	return res;

as all the error codepaths that can reach here are returning error()
or error_errno() after this patch?

>  }