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 v2 1/2] commit: fix --short and --porcelain options
Date: Wed, 02 May 2018 14:50:08 +0900	[thread overview]
Message-ID: <xmqq36zawqr3.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180426092524.25264-2-sxlijin@gmail.com> (Samuel Lijin's message of "Thu, 26 Apr 2018 05:25:23 -0400")

Samuel Lijin <sxlijin@gmail.com> writes:

> Mark the commitable flag in the wt_status object in the call to
> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> and simplify the logic in the latter function to take advantage of the
> logic shifted to the former. This means that callers do not need to use
> `wt_longstatus_print_updated()` to collect the `commitable` flag;
> calling `wt_status_collect()` is sufficient.
>
> As a result, invoking `git commit` with `--short` or `--porcelain`
> (which imply `--dry-run`, but previously returned an inconsistent error
> code inconsistent with dry run behavior) correctly returns status code
> zero when there is something to commit. This fixes two bugs documented
> in the test suite.


Hmm, I couldn't quite get what the above two paragraphs were trying
to say, but I think I figured out by looking at wt_status.c before
applying this patch, so let me see if I correctly understand what
this patch is about by thinking a bit aloud.

There are only two assignments to s->commitable in wt-status.c; one
happens in wt_longstatus_print_updated(), when the function notices
there is even one record to be shown (i.e. there is an "updated"
path) and the other in show_merge_in_progress() which is called by
wt_longstatus_prpint_state().  The latter codepath happens when we
are in a merge and there is no remaining conflicted paths (the code
allows the contents to be committed to be identical to HEAD).  Both
are called from wt_longstatus_print(), which in turn is called by
wt_status_print().

The implication of the above observation is that we do not set
commitable bit (by the way, shouldn't we spell it with two 'T's?)
if we are not doing the long format status.  The title hints at it
but "fix" is too vague.  It would be easier to understand if it
began like this (i.e. state problem clearly first, before outlining
the solution):

	[PATCH 1/2] commit: fix exit status under --short/--porcelain options

	In wt-status.c, s->commitable bit is set only in the
	codepaths reachable from wt_status_print() when output
	format is STATUS_FORMAT_LONG as a side effect of printing
	bits of status.  Consequently, when running with --short and
	--porcelain options, the bit is not set to reflect if there
	is anything to be committed, and "git commit --short" or
	"--porcelain" (both of which imply "--dry-run") failed to
	signal if there is anything to commit with its exit status.

	Instead, update s->commitable bit in wt_status_collect(),
	regardless of the output format. ...

Is that what is going on here?  Yours made it sound as if moving the
code to _collect() was done for the sake of moving code around and
simplifying the logic, and bugfix fell out of the move merely as a
side effect, which probably was the source of my confusion.

> +static void wt_status_mark_commitable(struct wt_status *s) {
> +	int i;
> +
> +	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->commitable = 1;
> +			return;
> +		}
> +	}
> +}

I am not sure if this is sufficient.  From a cursory look of the
existing code (and vague recollection in my ageing brain ;-), I
think we say it is committable if

 (1) when not merging, there is something to show in the "to be
     committed" section (i.e. there must be something changed since
     HEAD in the index).

 (2) when merging, no conflicting paths remain (i.e. change.nr being
     zero is fine).

So it is unclear to me how you are dealing with (2) under "--short"
option, which does not call show_merge_in_progress() to catch that
case.

  reply	other threads:[~2018-05-02  5:50 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
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 [this message]
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=xmqq36zawqr3.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).