Re: [PATCH v4 03/13] bisect--helper: introduce new `write_in_file()` function
From: Junio C Hamano <hidden>
Date: 2020-07-08 19:52:13
Miriam Rubio [off-list ref] 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.
quoted hunk
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?
}