All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Pat Notz <patnotz@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 2/2] commit: add support for --fixup <commit> -m"<extra message>"
Date: Fri, 22 Dec 2017 13:28:01 -0800	[thread overview]
Message-ID: <xmqqzi6api5a.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20171222204152.4822-1-avarab@gmail.com> ("Ævar Arnfjörð Bjarmason"'s message of "Fri, 22 Dec 2017 20:41:52 +0000")

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Those options could also support combining with -m, but given what
> they do I can't think of a good use-case for doing that, so I have not
> made the more invasive change of splitting up the logic in commit.c to
> first act on those, and then on -m options.
>
> 1. d71b8ba7c9 ("commit: --fixup option for use with rebase
>    --autosquash", 2010-11-02)

To be fair, when "rebase --autosquash -i" is run (which is why you
would use --fixup in the first place), the log message of the fixup
one is used only for locating which one is to be corrected, and the
contents of the log message is discarded.  So "given what it does",
I can't think of a good use-case for using --fixup and -m together,
either.  So "nobody immediately thought of it when it was added" is
certainly not a reason to later make the combination possible.

But I personally am moderately negative on one of these two imagined
use cases.

> Add support for supplying the -m option with --fixup. Doing so has
> errored out ever since --fixup was introduced. Before this, the only
> way to amend the fixup message while committing was to use --edit and
> amend it in the editor.
>
> The use-case for this feature is one of:
>
>  * Leaving a quick note to self when creating a --fixup commit when
>    it's not self-evident why the commit should be squashed without a
>    note into another one.

This is probably OK.

>  * (Ab)using the --fixup feature to "fix up" commits that have already
>    been pushed to a branch that doesn't allow non-fast-forwards,
>    i.e. just noting "this should have been part of that other commit",
>    and if the history ever got rewritten in the future the two should
>    be combined.

This has a smell of the tail wagging the dog.

Perhaps your editor does not have a good integration with external
commands to allow you to insert a single-liner output from

    git show --date=short -s --pretty='format:%h ("%s", %ad)' "$1"

and that is what you are abusing --fixup for?  

It is simply bad practice to leave a log entry that begins with
!fixup marker that would confuse automated tools like "rebase -i"
machinery on a commit that you have no intention of squashing into
another, as it invites mistakes.  

I do agree with the scenario where you would wish you could take
back an earlier mistake but you cannot.  But the log for such a
follow-up fix should be written just like any other follow-up fix
commit, i.e. describe what was wrong and how the wrongness is
corrected with the follow-up change.  What was wrong in "which
commit" is of course important part, but it is a relatively small
part.

  reply	other threads:[~2017-12-22 21:28 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-20 19:38 [PATCH 1/2] commit doc: document that -c, -C, -F and --fixup with -m error Ævar Arnfjörð Bjarmason
2017-12-20 19:38 ` [PATCH 2/2] commit: add support for --fixup <commit> -m"<extra message>" Ævar Arnfjörð Bjarmason
2017-12-20 20:03   ` Eric Sunshine
2017-12-20 21:40     ` Ævar Arnfjörð Bjarmason
2017-12-20 21:50       ` Eric Sunshine
2017-12-22 16:00         ` [PATCH v2 0/2] support -m"<msg>" combined with commit --fixup Ævar Arnfjörð Bjarmason
2017-12-22 19:53           ` Eric Sunshine
2017-12-22 20:43             ` Ævar Arnfjörð Bjarmason
2017-12-22 16:00         ` [PATCH v2 1/2] commit doc: document that -c, -C, -F and --fixup with -m error Ævar Arnfjörð Bjarmason
2017-12-22 20:41         ` [PATCH v2 2/2] commit: add support for --fixup <commit> -m"<extra message>" Ævar Arnfjörð Bjarmason
2017-12-22 21:28           ` Junio C Hamano [this message]
2017-12-22 21:29             ` Junio C Hamano
2017-12-22 21:58             ` Ævar Arnfjörð Bjarmason
2017-12-22 22:06               ` Junio C Hamano
2017-12-23 12:49                 ` Ævar Arnfjörð Bjarmason
2017-12-20 19:45 ` [PATCH 1/2] commit doc: document that -c, -C, -F and --fixup with -m error Eric Sunshine

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=xmqqzi6api5a.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=patnotz@gmail.com \
    --cc=sunshine@sunshineco.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.