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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 17, 2018, 17:33 UTC
Message-ID: <xmqqh8kxpy21.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180715110807.25544-4-sxlijin@gmail.com>

Samuel Lijin <sxlijin@gmail.com> writes:

Show 30 quoted lines

> 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.

Show 8 quoted lines

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

Show 25 quoted lines

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