Re: [RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C

Pranit Bauva [off-list ref] writes:

+static int write_terms(const char *bad, const char *good)
+{
+	FILE *fp;
+	int res;
+
+	if (!strcmp(bad, good))
+		return error(_("please use two different terms"));
+
+	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 = fprintf(fp, "%s\n%s\n", bad, good);
+	res |= fclose(fp);
+	return (res < 0) ? -1 : 0;
+}

If fprintf(3) were a function that returns 0 on success and negative
on error (like fclose(3) is), the pattern to cascade the error
return with "res |= another_call()" is appropriate, but the made me
hiccup a bit while reading it.  It is not wrong per-se and it would
certainly be making it worse if we did something silly like

	res = fprintf(...) < 0 ? -1 : 0;
        res |= fclose(fp);

so I guess what you have is the most succinct way to do this.

Hey Junio,

On Tue, Aug 2, 2016 at 11:08 PM, Junio C Hamano [off-list ref] wrote:

Pranit Bauva [off-list ref] writes:

quoted

+static int write_terms(const char *bad, const char *good)
+{
+     FILE *fp;
+     int res;
+
+     if (!strcmp(bad, good))
+             return error(_("please use two different terms"));
+
+     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 = fprintf(fp, "%s\n%s\n", bad, good);
+     res |= fclose(fp);
+     return (res < 0) ? -1 : 0;
+}

If fprintf(3) were a function that returns 0 on success and negative
on error (like fclose(3) is), the pattern to cascade the error
return with "res |= another_call()" is appropriate, but the made me
hiccup a bit while reading it.  It is not wrong per-se and it would
certainly be making it worse if we did something silly like

        res = fprintf(...) < 0 ? -1 : 0;
        res |= fclose(fp);

so I guess what you have is the most succinct way to do this.

I agree with your point and your suggested code is better!

Regards,
Pranit Bauva