| Message ID | 20200701133504.18360-4-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect to C part 2 | expand |
On Wed, Jul 1, 2020 at 6:37 AM Miriam Rubio <mirucam@gmail.com> wrote: > Let's refactor code adding a new `write_in_file()` function > that opens a file for writing a message and closes it and a > wrapper for writting mode. Nit: typo, s/writting/writing/ [snippage] > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 0466b07a43..b421056546 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -74,6 +74,38 @@ static int one_of(const char *term, ...) > return res; > } > > +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); Should this maybe just be BUG()? > + 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) { This isn't quite right - vfprintf() returns a negative value on error, or the number of characters printed on success. Zero is technically OK (if the format and arguments ended up empty). [rest snipped] Chris
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? > }
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 0466b07a43..b421056546 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -74,6 +74,38 @@ static int one_of(const char *term, ...) return res; } +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); + 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) { + fclose(fp); + return error_errno(_("could not write to file '%s'"), path); + } + + return fclose(fp); +} + +static int write_to_file(const char *path, const char *format, ...) +{ + int res; + va_list args; + + va_start(args, format); + res = write_in_file(path, "w", format, args); + va_end(args); + + return res; +} + static int check_term_format(const char *term, const char *orig_term) { int res; @@ -104,7 +136,6 @@ 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; int res; 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; }
Let's refactor code adding a new `write_in_file()` function that opens a file for writing a message and closes it and a wrapper for writting mode. This helper will be used in later steps and makes the code simpler and easier to understand. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Miriam Rubio <mirucam@gmail.com> --- builtin/bisect--helper.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)