From: Junio C Hamano <gitster@pobox.com>
To: "Stephen P. Smith" <ischis2@cox.net>
Cc: Git Mailing List <git@vger.kernel.org>,
Ovidiu Gheorghioiu <ovidiug@gmail.com>
Subject: Re: [PATCH] wt-status.c: set commitable bit if there is a meaningful merge.
Date: Tue, 16 Feb 2016 19:33:54 -0800 [thread overview]
Message-ID: <xmqqr3gcj9i5.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1455590305-30923-1-git-send-email-ischis2@cox.net> (Stephen P. Smith's message of "Mon, 15 Feb 2016 19:38:25 -0700")
"Stephen P. Smith" <ischis2@cox.net> writes:
> The 'commit --dry-run' and commit return values differed if a
> conflicted merge had been resolved and the commit would be the same as
> the parent.
>
> Update show_merge_in_progress to set the commitable bit if conflicts
> have been resolved and a merge is in progress.
>
> Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> ---
I think I mislead you into a slightly wrong direction. While the
single liner does improve the situation, I think this is merely a
band-aid upon closer inspection. For example, if you changed your
"commit --dry-run" in your test to "commit --dry-run --short", you
would notice that the test would fail.
In fact, "commit --dry-run" is already broken without this "a merge
ends up in a no-op" corner case. The management of s->commitable
flag and dry_run_commit() that uses it are unfortunately more broken
than I originally thought.
If we check for places where s->committable is set, we notice that
there is only one location: wt_status_print_updated(). This function
runs an equivalent of "diff-index --cached" and flips s->committable
on when it sees any difference.
This function is only called from wt_status_print(), which in turn
is only called from run_status() in commit.c when the status format
is unspecified or set to STATUS_FORMAT_LONG.
So if you do this:
$ git reset --hard HEAD
$ >a-new-file && git add a-new-file
$ git commit --dry-run --short; echo $?
you'd get "No, there is nothing interesting to commit", which is
clearly bogus.
I said s->committable is flipped on only when there is any change in
"diff-index --cached". There is nothing that flips it off, by
noticing that there are unmerged paths, for example. This is
another brokenness around "git commit --dry-run". Imagine that you
are in a middle of a conflicted cherry-pick. You did "git add" on a
resolved path and you still have another path whose conflict has not
been resolved. If you run a "git commit --dry-run", you will hear
"Yes, you can make a meaningful commit", which again is clearly
bogus.
These things need to be eventually fixed, and I think the fix will
involve revamping how we compute s->committable flag. Most likely,
we won't be doing any of that in any wt_status function whose name
has "print" or "show" in it. As the original designer of the wt_*
suite (before these multiple output formats are added), I would say
everything should happen inside the "collect" phase, if we wanted to
make s->committable bit usable.
So in the sense, eventually the code updated by this patch will have
to be discarded when we fix the "commit --dry-run" in the right way,
but in the meantime, the patch does not make things worse, so let's
think about queuing it as-is for now as a stop-gap measure.
Thanks.
next prev parent reply other threads:[~2016-02-17 3:34 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1649296.sC1eN3ni6k@thunderbird>
2016-02-09 1:55 ` Bug report: 'git commit --dry-run' corner case: returns error ("nothing to commit") when all conflicts resolved to HEAD Stephen & Linda Smith
2016-02-16 2:38 ` [PATCH] wt-status.c: set commitable bit if there is a meaningful merge Stephen P. Smith
2016-02-16 8:20 ` Philip Oakley
2016-02-16 21:54 ` Junio C Hamano
2016-02-16 23:26 ` Stephen & Linda Smith
2016-02-16 23:40 ` Stephen & Linda Smith
2016-02-16 23:30 ` Stephen & Linda Smith
2016-02-17 3:33 ` Junio C Hamano [this message]
2016-02-17 5:06 ` [PATCH v2] " Stephen P. Smith
2016-02-17 4:58 ` [PATCH] " Stephen & Linda Smith
2016-05-10 4:51 ` Stephen & Linda Smith
2018-08-22 5:33 ` Stephen Smith
2018-08-22 15:39 ` Junio C Hamano
2018-08-22 15:54 ` Junio C Hamano
2018-08-23 1:28 ` Stephen & Linda Smith
2016-02-12 1:04 ` Bug report: 'git commit --dry-run' corner case: returns error ("nothing to commit") when all conflicts resolved to HEAD Stephen & Linda Smith
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqr3gcj9i5.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=ischis2@cox.net \
--cc=ovidiug@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.
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 NNTP newsgroup(s).