Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3160 bytes --]

Hi René,

On Sat, 6 May 2017, René Scharfe wrote:

> Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:
>
> > diff --git a/wt-status.c b/wt-status.c
> > index 1f3f6bcb980..117ac8cfb01 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1082,34 +1082,29 @@ static char *read_line_from_git_path(const char
> > *filename)
> >   static int split_commit_in_progress(struct wt_status *s)
> >   {
> >   	int split_in_progress = 0;
> > -	char *head = read_line_from_git_path("HEAD");
> > -	char *orig_head = read_line_from_git_path("ORIG_HEAD");
> > -	char *rebase_amend = read_line_from_git_path("rebase-merge/amend");
> > -	char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> > -
> > -	if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
> > -	    !s->branch || strcmp(s->branch, "HEAD")) {
> > -		free(head);
> > -		free(orig_head);
> > -		free(rebase_amend);
> > -		free(rebase_orig_head);
> > -		return split_in_progress;
> > -	}
> > -
> > -	if (!strcmp(rebase_amend, rebase_orig_head)) {
> > -		if (strcmp(head, rebase_amend))
> > -			split_in_progress = 1;
> > -	} else if (strcmp(orig_head, rebase_orig_head)) {
> > -		split_in_progress = 1;
> > -	}
> > +	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> > +
> > +	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> > +	    !s->branch || strcmp(s->branch, "HEAD"))
> > +		return 0;
> >   -	if (!s->amend && !s->nowarn && !s->workdir_dirty)
> > -		split_in_progress = 0;
> > +	head = read_line_from_git_path("HEAD");
> > +	orig_head = read_line_from_git_path("ORIG_HEAD");
> > +	rebase_amend = read_line_from_git_path("rebase-merge/amend");
> > +	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> 
> Further improvement idea (for a later series): Use rebase_path_amend()
> and rebase_path_orig_head() somehow, to build each path only once.
> 
> Accessing the files HEAD and ORIG_HEAD directly seems odd.
> 
> The second part of the function should probably be moved to sequencer.c.

Sure. On all four accounts.

> > +
> > +	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> > +		; /* fall through, no split in progress */
> 
> You could set split_in_progress to zero here.  Would save a comment
> without losing readability.

But that would confuse e.g. myself, 6 months down the road: why assign
that variable when it already has been assigned? (And we have to assign it
because there is still a code path entering none of these if/else if's
arms).

> > +	else if (!strcmp(rebase_amend, rebase_orig_head))
> > +		split_in_progress = !!strcmp(head, rebase_amend);
> > +	else if (strcmp(orig_head, rebase_orig_head))
> > +		split_in_progress = 1;
> >   
> >    free(head);
> >    free(orig_head);
> >    free(rebase_amend);
> >    free(rebase_orig_head);
> > +
> 
> Isn't the patch big enough already without rearranging the else blocks
> and adding that blank line? :)

The else blocks are not really rearranged; apart from the early return,
the rest of the logic has been painstakingly kept in the same order.

Ciao,
Dscho