[01/10] bisect--helper: introduce new `write_in_file()` function
diff mbox series

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

Commit Message

Miriam Rubio Feb. 26, 2020, 10:14 a.m. UTC
Let's refactor code adding a new `write_in_file()` function
that opens a file for writing a message and closes it.

This removes some duplicated code and makes the code simpler,
clearer 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 | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Junio C Hamano Feb. 26, 2020, 7:06 p.m. UTC | #1
Miriam Rubio <mirucam@gmail.com> writes:

> Let's refactor code adding a new `write_in_file()` function
> that opens a file for writing a message and closes it.
>
> This removes some duplicated code and makes the code simpler,
> clearer and easier to understand.

I find it a bit too much self promotion ;-)  With what we see only in
this step, the code is not so much simpler nor easier to understand.
Perhaps when we see more callers in the remainder of the series, we
may start to appreciate it, but if that is the case, we should
clarify it here, e.g.

    This helper will be used in later steps and makes the code
    simpler, ...

> +static int write_in_file(const char *filepath, const char *content, int append)
> +{
> +	FILE *fp = NULL;
> +	const char *mode = append ? "a" : "w";
> +
> +	fp = fopen(filepath, mode);
> +	if (!fp)
> +		return error_errno(_("could not open the file '%s'"), filepath);
> +	if (!fprintf(fp, "%s\n", content))
> +		return error_errno(_("could not write in file '%s'"), filepath);

Use and non-use of definite article being inconstent between "open
the file X" vs "write in file X" bothers me as a reader here.

Wouldn't it be more common to say "write TO" here, not "write IN"?

> +	return fclose(fp);

We didn't use to say error_errno(_("could not close ...")) in the
original, so we stay silent and return an error (EOF, which is a
negative integer) from here.

> +}
> +
>  static int check_term_format(const char *term, const char *orig_term)
>  {
>  	int res;
> @@ -104,7 +117,7 @@ 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;
> +	char *content = xstrfmt("%s\n%s", bad, good);
>  	int res;
>  
>  	if (!strcmp(bad, good))
> @@ -113,12 +126,9 @@ 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_in_file(git_path_bisect_terms(), content, 0);
> +	free(content);
>  
> -	res = fprintf(fp, "%s\n%s\n", bad, good);

With just this one callsite, I do not think it is a good idea to
lose the "write a formatted output without an extra allcoation" 
by introducing this wrapper.  The equation may become different if
we see more (a lot more) callsites that already have strings ready
to be written that can use this helper, though.

> -	res |= fclose(fp);
>  	return (res < 0) ? -1 : 0;

The return value is a bit strange here, but it is inherited from the
original, so I'll let it pass.

>  }

Patch
diff mbox series

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c1c40b516d..ee1be630da 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -74,6 +74,19 @@  static int one_of(const char *term, ...)
 	return res;
 }
 
+static int write_in_file(const char *filepath, const char *content, int append)
+{
+	FILE *fp = NULL;
+	const char *mode = append ? "a" : "w";
+
+	fp = fopen(filepath, mode);
+	if (!fp)
+		return error_errno(_("could not open the file '%s'"), filepath);
+	if (!fprintf(fp, "%s\n", content))
+		return error_errno(_("could not write in file '%s'"), filepath);
+	return fclose(fp);
+}
+
 static int check_term_format(const char *term, const char *orig_term)
 {
 	int res;
@@ -104,7 +117,7 @@  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;
+	char *content = xstrfmt("%s\n%s", bad, good);
 	int res;
 
 	if (!strcmp(bad, good))
@@ -113,12 +126,9 @@  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_in_file(git_path_bisect_terms(), content, 0);
+	free(content);
 
-	res = fprintf(fp, "%s\n%s\n", bad, good);
-	res |= fclose(fp);
 	return (res < 0) ? -1 : 0;
 }