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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2020, 19:52 UTC
Message-ID: <xmqqwo3d23a4.fsf@gitster.c.googlers.com>
In-Reply-To: <20200701133504.18360-4-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

Show 7 quoted lines

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

Show 6 quoted lines

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

Show 13 quoted lines

>  	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?

>  }