Re: [PATCH v3 3/3] commit: fix exit code for --short/--porcelain

>
> Samuel Lijin <sxlijin@gmail.com> 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);
> > +