threads / patch / 37331

patchmailsplit.c: remove dead code

Subject: [PATCH] mailsplit.c: remove dead code

## tl;dr

9 messages between Aug 11, 2014 and Aug 17, 2014. Diffs are folded; open one to read it.

replies: 8people: 6as markdown or json

Stefan Beller· Aug 11, 2014, 21:11 UTC · lore
This was found by coverity. (Id: 290001)

the variable 'output' is only assigned to a value inequal to NUL, after all gotos to the corrupt label. Therefore we can conclude the two removed lines are actually dead code.

Signed-off-by: Stefan Beller <stefanbeller@gmail.com>
---
 builtin/mailsplit.c | 2 --
 1 file changed, 2 deletions(-)
Show changes to builtin/mailsplit.c +0 −2
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 06296d4..b499014 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -93,8 +93,6 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)
 	return status;
 
  corrupt:
-	if (output)
-		fclose(output);
 	unlink(name);
 	fprintf(stderr, "corrupt mailbox\n");
 	exit(1);

-- 
2.1.0.rc2
René Scharfe· Aug 12, 2014, 16:38 UTC · re: Stefan Beller · lore

Re: [PATCH] mailsplit.c: remove dead code

Am 11.08.2014 um 23:11 schrieb Stefan Beller:
Show 5 quoted lines
> This was found by coverity. (Id: 290001)
>
> the variable 'output' is only assigned to a value inequal to NUL,
> after all gotos to the corrupt label.
> Therefore we can conclude the two removed lines are actually dead code.

After reading the above for the first time I thought it meant the opposite of what's actually going on. Perhaps it's the placement of "only", the comma or a flawed understanding of grammar on my part?

In any case, there is only one way to reach the label named corrupt, and the variable named output is always NULL if that branch is taken. That means the removed code was a no-op. With those two lines gone you also don't need to initialize output anymore, by the way.

And since there is only a single goto, you could move the three remaining error handling lines up to the if statement. Keeping condition and dependent code together would be an improvement, I think.

Show 19 quoted lines
> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>
> ---
>   builtin/mailsplit.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 06296d4..b499014 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -93,8 +93,6 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)
>   	return status;
>
>    corrupt:
> -	if (output)
> -		fclose(output);
>   	unlink(name);
>   	fprintf(stderr, "corrupt mailbox\n");
>   	exit(1);
>
Stefan Beller· Aug 12, 2014, 21:21 UTC · re: René Scharfe · lore
This was found by coverity. (Id: 290001)

The variable 'output' is assigned to a value after all gotos to the corrupt label.

Remove the goto by moving the errorhandling code to the condition, which detects the error.

Signed-off-by: Stefan Beller <stefanbeller@gmail.com>
Helped-by: René Scharfe <l.s.r@web.de>
---
 builtin/mailsplit.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)
If I understood correctly, this is what you had in mind?

Thanks, Stefan

Show changes to builtin/mailsplit.c +6 −11
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 06296d4..763cda0 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -53,14 +53,16 @@ static int keep_cr;
  */
 static int split_one(FILE *mbox, const char *name, int allow_bare)
 {
-	FILE *output = NULL;
+	FILE *output;
 	int fd;
 	int status = 0;
 	int is_bare = !is_from_line(buf.buf, buf.len);
 
-	if (is_bare && !allow_bare)
-		goto corrupt;
-
+	if (is_bare && !allow_bare) {
+		unlink(name);
+		fprintf(stderr, "corrupt mailbox\n");
+		exit(1);
+	}
 	fd = open(name, O_WRONLY | O_CREAT | O_EXCL, 0666);
 	if (fd < 0)
 		die_errno("cannot open output file '%s'", name);
@@ -91,13 +93,6 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)
 	}
 	fclose(output);
 	return status;
-
- corrupt:
-	if (output)
-		fclose(output);
-	unlink(name);
-	fprintf(stderr, "corrupt mailbox\n");
-	exit(1);
 }
 
 static int populate_maildir_list(struct string_list *list, const char *path)

-- 
2.1.0.rc2
Matthieu Moy· Aug 12, 2014, 21:37 UTC · re: Stefan Beller · lore

Re: [PATCH] mailsplit.c: remove dead code

Stefan Beller <stefanbeller@gmail.com> writes:
Show 5 quoted lines
> +	if (is_bare && !allow_bare) {
> +		unlink(name);
> +		fprintf(stderr, "corrupt mailbox\n");
> +		exit(1);
> +	}
Not directly related to your patch, but shouldn't this be using
die(_("corrupt mailbox"));
instead?
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Aug 12, 2014, 21:38 UTC · re: Matthieu Moy · lore

Re: [PATCH] mailsplit.c: remove dead code

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 13 quoted lines
> Stefan Beller <stefanbeller@gmail.com> writes:
>
>> +	if (is_bare && !allow_bare) {
>> +		unlink(name);
>> +		fprintf(stderr, "corrupt mailbox\n");
>> +		exit(1);
>> +	}
>
> Not directly related to your patch, but shouldn't this be using
>
> die(_("corrupt mailbox"));
>
> instead?

Doesn't the exit status of mailsplit matter to its existing invokers?

Junio C Hamano· Aug 12, 2014, 21:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] mailsplit.c: remove dead code

... answering to myself, the only invoker does not seem to care, so I do not mind if the fprintf/exit gets turned into die() in a follow-up patch.

Thanks.
On Tue, Aug 12, 2014 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Stefan Beller <stefanbeller@gmail.com> writes:
>>
>>> +    if (is_bare && !allow_bare) {
>>> +            unlink(name);
>>> +            fprintf(stderr, "corrupt mailbox\n");
>>> +            exit(1);
>>> +    }
>>
>> Not directly related to your patch, but shouldn't this be using
>>
>> die(_("corrupt mailbox"));
>>
>> instead?
>
> Doesn't the exit status of mailsplit matter to its existing
> invokers?
Matthieu Moy· Aug 12, 2014, 21:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] mailsplit.c: remove dead code

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Stefan Beller <stefanbeller@gmail.com> writes:
>>
>>> +	if (is_bare && !allow_bare) {
>>> +		unlink(name);
>>> +		fprintf(stderr, "corrupt mailbox\n");
>>> +		exit(1);
>>> +	}
>>
>> Not directly related to your patch, but shouldn't this be using
>>
>> die(_("corrupt mailbox"));
>>
>> instead?
>
> Doesn't the exit status of mailsplit matter to its existing
> invokers?

Not within Git. I doubt anybody checks if the exit status is 1 and relies on that for corrupt mailboxes, but OTOH, changing the code may not be worth the trouble.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jonathan Nieder· Aug 12, 2014, 22:58 UTC · re: Stefan Beller · lore

Re: [PATCH] mailsplit.c: remove dead code

Stefan Beller wrote:
> This was found by coverity. (Id: 290001)
[...]
Show 5 quoted lines
> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>
> Helped-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/mailsplit.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)

The idea is to simplify error handling (removing cleanup of the 'output' var) by making it more obvious that there is only one kind of error, right?

For what it's worth, it looks good to me (and much clearer than the last version). It's always possible to reintroduce goto-based error handling later if another kind of error is introduced, and in the meantime this is simpler and does not change behavior.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Jeff King· Aug 17, 2014, 08:14 UTC · re: René Scharfe · lore

Re: [PATCH] mailsplit.c: remove dead code

On Tue, Aug 12, 2014 at 06:38:38PM +0200, René Scharfe wrote:
Show 19 quoted lines
> Am 11.08.2014 um 23:11 schrieb Stefan Beller:
> >This was found by coverity. (Id: 290001)
> >
> >the variable 'output' is only assigned to a value inequal to NUL,
> >after all gotos to the corrupt label.
> >Therefore we can conclude the two removed lines are actually dead code.
> 
> After reading the above for the first time I thought it meant the opposite
> of what's actually going on.  Perhaps it's the placement of "only", the
> comma or a flawed understanding of grammar on my part?
> 
> In any case, there is only one way to reach the label named corrupt, and the
> variable named output is always NULL if that branch is taken.  That means
> the removed code was a no-op.  With those two lines gone you also don't need
> to initialize output anymore, by the way.
> 
> And since there is only a single goto, you could move the three remaining
> error handling lines up to the if statement.  Keeping condition and
> dependent code together would be an improvement, I think.

I think that would be a correct refactoring of the current code, but I have to wonder why the other die cases are not using "goto corrupt" in the first place.

The other thing this code path does is unlink the file "name". In the current code, this is _also_ a noop. We "goto corrupt" before we actually open the output file. So like the fclose(output), it is cleaning up an operation that was never started. It can just go away.

But the bigger question is: should the other code paths be cleaning up the file? It probably doesn't matter, as mailsplit is typically run in a temporary directory in the first place, so it is up to the caller to clean up any half-formed cruft. And if we did want to clean up cruft, we should probably do it with an atexit/signal handler to catch more cases.

Given that we are not cleaning up now and nobody has complained, I'd be inclined to say we should not. And the unlink can just go away, and all errors can just call die().

-Peff

← back to recent threads