Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:
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.schinde...@gmx.de>
---
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.
Right. It could have been used as a quick two-line fix.
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.
The only function with a higher cyclomatic complexity in this file is
wt_longstatus_print(), which spans more than 100 lines. Amazing.
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).
Repeatedly I get the impression that defects or shortcomings tend to
cluster. It pays to take a good look at the code surrounding a find of
an automatic checker for other possible improvements.
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):
It could be done in two steps. Doing it in one is probably OK as well.
-- 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.schinde...@gmx.de>
---
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");
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.
+
+ 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.
+ 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? :)
return split_in_progress;
}
