Hi René,
On Wed, 3 May 2017, René Scharfe wrote:
> Am 02.05.2017 um 18:02 schrieb Johannes Schindelin:
> > Reported via Coverity.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> > ---
> > wt-status.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/wt-status.c b/wt-status.c
> > index 0a6e16dbe0f..1f3f6bcb980 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status
> > *s)
> > 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"))
> > + !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))
> >
>
> The return line could be replaced by "; else" to achieve the same
> result, without duplicating the free calls.
True. It is much harder to explain it that way, though: the context looks
like this:
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"))
+ !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;
}
if (!s->amend && !s->nowarn && !s->workdir_dirty)
split_in_progress = 0;
free(head);
free(orig_head);
free(rebase_amend);
free(rebase_orig_head);
return split_in_progress;
}
So as you see, if we make the change you suggested, the next if() is hit
which possibly sets split_in_progress = 0.
The thing is: this variable has been initialized to 0 in the beginning! So
this conditional assignment would be a noop, unless any of the code paths
before this conditional modified split_in_progress.
After you fully wrapped your head around this code, I am sure you agree
that the code is unnecessarily confusing to begin with: why do we go out
of our way to allocate and read all those strings, compare them to figure
out whether there is a split in progress, just to look at bits in the
wt_status struct (that we had available from the beginning) to potentially
undo all of that.
What's worse, I cannot even find a logical explanation why the code is so
confusing, as it has been added as it is today in commit 2d1ccebae41
(status: better advices when splitting a commit (during rebase -i),
2012-06-10).
So I think this function really wants to be fixed more fully (although I
feel bad for inserting such a non-trivial fix into a patch series
otherwise populated by trivial memory/resource leak plugs):
-- snipsnap --
Subject: [PATCH] squash! split_commit_in_progress(): fix memory leak
split_commit_in_progress(): simplify & fix memory leak
This function did a whole lot of unnecessary work, such as reading in
four files just to figure out that, oh, hey, we do not need to look at
them after all because the HEAD is not detached.
Simplify the entire function to return early when possible, to read in
the files only when necessary, and to release the allocated memory
always (there was a leak, reported via Coverity, where we failed to
release the allocated strings if the HEAD is not detached).
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
---
wt-status.c | 39 +++++++++++++++++----------------------
1 file changed, 17 insertions(+), 22 deletions(-)
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");
+
+ if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+ ; /* fall through, no split in progress */
+ 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);
+
return split_in_progress;
}
--
2.12.2.windows.2.800.gede8f145e06