[v4,03/13] bisect--helper: introduce new `write_in_file()` function
diff mbox series

Message ID 20200701133504.18360-4-mirucam@gmail.com
State New
Headers show
Series
  • Finish converting git bisect to C part 2
Related show

Commit Message

Miriam Rubio July 1, 2020, 1:34 p.m. UTC
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(-)

Comments

Chris Torek July 1, 2020, 2:56 p.m. UTC | #1
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
Junio C Hamano July 8, 2020, 7:52 p.m. UTC | #2
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?

>  }

Patch
diff mbox series

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;
 }