From: Stephen & Linda Smith <ischis2@cox.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Git Mailing List" <git@vger.kernel.org>
Subject: Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.
Date: Fri, 31 Aug 2018 11:13:24 -0700 [thread overview]
Message-ID: <1863304.0psXOz24iy@thunderbird> (raw)
In-Reply-To: 87a7p3c83g.fsf@evledraar.gmail.com
On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >> Leave the setting of the commitable flag in show_merge_in_progress. If
> >> a check for merged commits is moved to the collect phase then other
> >> --dry-run tests fail.
>
> "Some tests fail" is not a good explanation why you keep the setting
> of commitable bit in the "show" codepath. The test coverage may not
> be thorough, and the tests that fail might be expecting wrong things.
>
I didn't figure it was, but I haven't yet figured out how to explain what what
I saw last evening. I wanted to send something out to get feedback from
someone who may know the code far better than I.
> The change in this patch makes the internal "diff-index" invocation
> responsible for setting the commitable bit. This is better for non
> merge commits than the current code because the current code only
> sets the commitable bit when longstatus is asked for (and the code
> to show the longstatus detects changes to be committed), so the
> short-form will not have chance to set the bit, but the internal
> "diff-index" is what determines if the resulting commit would have
> difference relative to the HEAD, so it is a better place to make
> that decision.
>
> Merge commits need to be allowed even when the resulting commit
> records a tree that is identical to that of the current HEAD
> (i.e. we merged a side branch, but we already had all the necessary
> changes done on it). So it is insufficient to let "diff-index"
> invocation to set the committable bit. Is that why "other --dry-run
> tests fail"? What I am getting at is to have a reasonable "because
> ..." to explain why "other --dry-run tests fail" after it, to make
> it clear to the readers that the failure is not because tests are
> checking wrong things but because a specific condition
thatwt_status_collect(), isYes
> expeted from the code gets violated if we change the code in
> show_merge_in_progress() function.
Agreed. I'm just green at this code base, and so don't quite know what I
should see as opposed to what I do see.
>
> That brings us to another point. Is there a case where we want to
> see s->commitable bit set correctly but we do not make any call to
> show_merge_in_progress() function? It is conceivable to have a new
> "git commit --dry-run --quiet [[--] <pathspec>]" mode that is
> totally silent but reports if what we have is committable with the
> exit status, and for that we would not want to call any show_*
> functions. That leads me to suspect that ideally we would want to
> see wt_status_collect_changes_index() to be the one that is setting
> the commitable bit. Or even its caller wt_status_collect(), which
> would give us a better chance of being correct even during the
> initial commit. For the "during merge" case, we would need to say
> something like
>
> if (state->merge_in_progress && !has_unmerged(s))
> s->commitable = 1;
>
I placed the following in wt_status_collect() last evening, and received
errors from three early tests in 7501-commit.sh. Thanks for a hint.
if (!has_unmerged(s))
s->commitable = 1;
Maybe the missing first condition was what I needed.
Which leads me to asking: Do you want a preparatory patch moving
has_unmerged() further up in the file before adding a reference to
has_unmerged() in wt_status_collect().
> but the "state" thing is passed around only among the "print/show"
> level of functions in the current code. We might need to merge that
> into the wt_status structure to pass it down to the "collect" phase
> at the lower level before/while doing so. I dunno.
Would you explain what you are thinking for passing moving the "stat" think
into wt_status. I haven't figured out how the "collect" sequence, relates
to the "print/show" squence.
>
> Thanks for working on this.
I decided sometime back to work on something I didn't know using a process I
don't normally use to broaden my experience. I'm enjoying this and hope you
don't mind lots of questions from someone new.
sps
prev parent reply other threads:[~2018-08-31 18:13 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-31 5:39 [PATCH 0/3] wt-status.c: commitable flag Stephen P. Smith
2018-08-31 5:39 ` [PATCH 1/3] Change tests from expecting to fail to expecting success Stephen P. Smith
2018-08-31 7:17 ` Ævar Arnfjörð Bjarmason
2018-08-31 5:39 ` [PATCH 2/3] Add test for commit --dry-run --short Stephen P. Smith
2018-08-31 7:18 ` Ævar Arnfjörð Bjarmason
2018-08-31 16:26 ` Junio C Hamano
2018-08-31 17:58 ` Stephen & Linda Smith
2018-09-01 11:55 ` SZEDER Gábor
2018-08-31 5:39 ` [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase Stephen P. Smith
2018-08-31 7:23 ` Ævar Arnfjörð Bjarmason
2018-08-31 16:54 ` Junio C Hamano
2018-08-31 18:13 ` Stephen & Linda Smith [this message]
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=1863304.0psXOz24iy@thunderbird \
--to=ischis2@cox.net \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.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).