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

[Samuel Lijin <sxlijin@gmail.com>]

Thanks for the review.

On Tue, Jul 17, 2018 at 10:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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'll be honest: when I wrote this, I didn't think too much about what
the code was actually doing, semantically speaking: I was assuming
that the behavior that set the commitable bit in the call tree of
wt_longstatus_print() was correct, and that it was just a matter of
mechanically copying that logic over to the --short/--porcelain call
paths.

Looking into this more deeply, I think you're right, but more
problematically, this is technically a bug with the current Git code
that seems to be cancelled out by another bug: wt_status_state
apparently does not correctly reflect the state of the index when it
reaches wt_longstatus_print_updated(). Working from master
(f55ff46c9), I modified the last test in t7501 to look like this:

→.echo "Initial contents, unimportant" | tee test-file1 test-file2 &&
→.git add test-file1 test-file2 &&
→.git commit -m "Initial commit" &&
→.echo "commit-1-state" | tee test-file1 test-file2 &&
→.git commit -m "commit 1" -i test-file1 test-file2 &&
→.git tag commit-1 &&
→.git checkout -b branch-2 HEAD^1 &&
→.echo "commit-2-state" | tee test-file1 test-file2 &&
→.git commit -m "commit 2" -i test-file1 test-file2 &&
→.! $(git merge --no-commit commit-1) &&
→.echo "commit-2-state" | tee test-file1 &&
→.git add test-file1 &&
→.git commit --dry-run &&
→.git commit -m "conflicts fixed from merge."

And once inside gdb did this:

(gdb) b wt-status.c:766
Breakpoint 1 at 0x205d73: file wt-status.c, line 766.
(gdb) r
Starting program: /home/pockets/git/git commit --dry-run
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
On branch branch-2
You have unmerged paths.
  (fix conflicts and run "git commit")
  (use "git merge --abort" to abort the merge)


Breakpoint 1, wt_longstatus_print_updated (s=0x555555a29960 <s>) at
wt-status.c:766
warning: Source file is more recent than executable.
760             for (i = 0; i < s->change.nr; i++) {
(gdb) print s->change.nr
$1 = 1

Can you confirm I'm not crazy, and am analyzing this correctly?

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