git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Samuel Lijin <sxlijin@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 3/3] commit: fix exit code for --short/--porcelain
Date: Tue, 17 Jul 2018 10:33:10 -0700	[thread overview]
Message-ID: <xmqqh8kxpy21.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180715110807.25544-4-sxlijin@gmail.com> (Samuel Lijin's message of "Sun, 15 Jul 2018 07:08:07 -0400")

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

  reply	other threads:[~2018-07-17 17:33 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-18  3:06 [PATCH 0/2] Fix --short and --porcelain options for commit Samuel Lijin
2018-04-18  3:06 ` [PATCH 1/2] commit: fix --short and --porcelain Samuel Lijin
2018-04-18 18:38   ` Martin Ågren
     [not found]     ` <CAJZjrdW3X8eaSit85otKV2HvHmu0NDGcnnnrtxHME03q=eWW-Q@mail.gmail.com>
2018-04-19  3:55       ` Samuel Lijin
2018-04-20  7:08   ` Eric Sunshine
2018-04-18  3:06 ` [PATCH 2/2] wt-status: const-ify all printf helper methods Samuel Lijin
2018-04-26  9:25 ` [PATCH v2 0/2] Fix --short and --porcelain options for commit Samuel Lijin
2018-07-15 11:08   ` [PATCH v3 0/3] Fix --short/--porcelain options for git commit Samuel Lijin
2018-07-23  2:08     ` [PATCH v4 0/4] Rerolling patch series to fix t7501 Samuel Lijin
2018-07-30 22:15       ` Junio C Hamano
2018-07-23  2:09     ` [PATCH v4 1/4] t7501: add coverage for flags which imply dry runs Samuel Lijin
2018-07-23  2:09     ` [PATCH v4 2/4] wt-status: rename commitable to committable Samuel Lijin
2018-07-23  2:09     ` [PATCH v4 3/4] wt-status: teach wt_status_collect about merges in progress Samuel Lijin
2018-07-23  2:09     ` [PATCH v4 4/4] commit: fix exit code when doing a dry run Samuel Lijin
2018-07-15 11:08   ` [PATCH v3 1/3] t7501: add merge conflict tests for " Samuel Lijin
2018-07-17 17:05     ` Junio C Hamano
2018-07-17 17:45       ` Junio C Hamano
2018-07-15 11:08   ` [PATCH v3 2/3] wt-status: teach wt_status_collect about merges in progress Samuel Lijin
2018-07-17 17:15     ` Junio C Hamano
2018-07-15 11:08   ` [PATCH v3 3/3] commit: fix exit code for --short/--porcelain Samuel Lijin
2018-07-17 17:33     ` Junio C Hamano [this message]
2018-07-19  9:31       ` Samuel Lijin
2018-04-26  9:25 ` [PATCH v2 1/2] commit: fix --short and --porcelain options Samuel Lijin
2018-05-02  5:50   ` Junio C Hamano
2018-05-02 15:52     ` Samuel Lijin
2018-04-26  9:25 ` [PATCH v2 2/2] wt-status: const-ify all printf helper methods Samuel Lijin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh8kxpy21.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=sxlijin@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).