threads / patch / 58960

patchbisect: remove check for fp and move the goto label after the fclose

Subject: [PATCH] bisect: remove check for fp and move the goto label after the fclose

## tl;dr

2 messages between Dec 16, 2022 and Dec 16, 2022. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Rose via GitGitGadget· Dec 16, 2022, 00:43 UTC · lore
From: Seija Kijin <doremylover123@gmail.com>
The fp check in these function are only relevant if goto has been taken.
Because of this, we should move the label so that fclose is not called.
This is because the goto is taken when fp is 0 anyway.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
    bisect: remove check for fp and move the goto label after the fclose
    
    The fp check in these function are only relevant if goto has been taken.
    
    Because of this, we should move the label so that fclose is not called.
    
    This is because the goto is taken when fp is 0 anyway.
    
    Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1397%2FAtariDreams%2Ffclose-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1397/AtariDreams/fclose-v1
Pull-Request: https://github.com/git/git/pull/1397
 builtin/bisect.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to builtin/bisect.c +4 −4
diff --git a/builtin/bisect.c b/builtin/bisect.c
index cc9483e8515..2e5dbc8be3b 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -316,9 +316,9 @@ static int bisect_write(const char *state, const char *rev,
 	if (!nolog)
 		fprintf(fp, "git bisect %s %s\n", state, rev);
 
+	fclose(fp);
+
 finish:
-	if (fp)
-		fclose(fp);
 	strbuf_release(&tag);
 	return res;
 }
@@ -484,9 +484,9 @@ static int get_terms(struct bisect_terms *terms)
 	strbuf_getline_lf(&str, fp);
 	terms->term_good = strbuf_detach(&str, NULL);
 
+	fclose(fp);
+
 finish:
-	if (fp)
-		fclose(fp);
 	strbuf_release(&str);
 	return res;
 }

base-commit: 57e2c6ebbe7108b35ba30184dcbcb6c34c929ad8
-- 
gitgitgadget
Junio C Hamano· Dec 16, 2022, 01:03 UTC · re: Rose via GitGitGadget · lore

Re: [PATCH] bisect: remove check for fp and move the goto label after the fclose

"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> From: Seija Kijin <doremylover123@gmail.com>
>
> The fp check in these function are only relevant if goto has been taken.
>
> Because of this, we should move the label so that fclose is not called.
>
> This is because the goto is taken when fp is 0 anyway.

It may happen to be so with today's code, but for the health of the code in the longer term, I do not think this patch is a good idea. We may add more "check error and jump to the label to clean it up" exit paths to these functions, and change to add such exit paths would essentially need to revert this change.

By the way, I do not know if this is something GGG end-users can fix, or requires a fix in GGG itself, but somebody keeps adding an e-mail address in the @users.noreply.github.com domain to the Cc: field of these patches. For folks whose work is GitHub only, it might be fine to declare "this user only has e-mail-address-looking string but does not receive e-mail at all", but as working with GGG is all about following e-mail based workflow, such an address should be added to patch e-mails sent to the list this say.

Thanks.

← back to recent threads