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
