Hi René,

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

Am 04.05.2017 um 12:59 schrieb Johannes Schindelin:

quoted

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.

quoted

+
+	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).

quoted

+	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