Thanks for the review.
On Tue, Jul 17, 2018 at 10:33 AM Junio C Hamano <[email protected]> wrote:
>
> Samuel Lijin <[email protected]> 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);
> > +
