Samuel Lijin <sxlijin@xxxxxxxxx> writes:
> diff --git a/wt-status.c b/wt-status.c
> index 75d389944..4ba657978 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -718,6 +718,39 @@ static void wt_status_collect_untracked(struct wt_status *s)
> s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
> }
>
> +static int has_unmerged(const struct wt_status *s)
> +{
> + int i;
> +
> + for (i = 0; i < s->change.nr; i++) {
> + struct wt_status_change_data *d;
> + d = s->change.items[i].util;
> + if (d->stagemask)
> + return 1;
> + }
> + return 0;
> +}
> +
> +static void wt_status_mark_committable(
> + struct wt_status *s, const struct wt_status_state *state)
> +{
> + int i;
> +
> + if (state->merge_in_progress && !has_unmerged(s)) {
> + s->committable = 1;
> + return;
> + }
Is this trying to say:
During/after a merge, if there is no higher stage entry in
the index, we can commit.
I am wondering if we also should say:
During/after a merge, if there is any unresolved conflict in
the index, we cannot commit.
in which case the above becomes more like this:
if (state->merge_in_progress) {
s->committable = !has_unmerged(s);
return;
}
But with your patch, with no remaining conflict in the index during
a merge, the control comes here and goes into the next loop.
> + for (i = 0; i < s->change.nr; i++) {
> + struct wt_status_change_data *d = (s->change.items[i]).util;
> +
> + if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
> + s->committable = 1;
> + return;
> + }
> + }
The loop seems to say "As long as there is one entry in the index
that is not in conflict and is different from the HEAD, then we can
commit". Is that correct?
Imagine there are two paths A and B in the branches involved in a
merge, and A cleanly resolves (say, we take their version because
our history did not touch it since we diverged) while B has
conflict. We'll come to this loop (because we are in a merge but
have some unmerged paths) and we find that A is different from HEAD,
happily set committable bit and return.
I _think_ with the change to "what happens during merge" above that
I suggested, this loop automatically becomes correct, but I didn't
think it through. If there are ways other than .merge_in_progress
that place conflicted entries in the index, then this loop is still
incorrect and would want to be more like:
for (i = 0; i < s->change.nr; i++) {
struct wt_status_change_data *d = (s->change.items[i]).util;
if (d->index_status == DIFF_STATUS_UNMERGED) {
s->committable = 0;
return;
}
if (d->index_status)
s->committable = 1;
}
i.e. we declare "not ready to commit" if there is *any* conflicted
entry, but otherwise set committable to 1 if we see any entry that
is different from HEAD (to declare succcess once we successfully
loop through to the last entry without seeing any conflict).
> void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
> {
> wt_status_collect_changes_worktree(s);
> @@ -728,6 +761,8 @@ void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
> wt_status_collect_changes_index(s);
>
> wt_status_collect_untracked(s);
> +
> + wt_status_mark_committable(s, state);
> }
>
> static void wt_longstatus_print_unmerged(const struct wt_status *s)
> @@ -753,28 +788,28 @@ static void wt_longstatus_print_unmerged(const struct wt_status *s)
>
> }
>
> -static void wt_longstatus_print_updated(struct wt_status *s)
> +static void wt_longstatus_print_updated(const struct wt_status *s)
> {
> - int shown_header = 0;
> int i;
>
> + if (!s->committable) {
> + return;
> + }
No need to have {} around a single statement. Especially when you
know you won't be touching the line (e.g. to later add more
statements in the block) in this last patch in a series.
> + wt_longstatus_print_cached_header(s);
> +
