Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'

From: Junio C Hamano <gitster@pobox.com>
To: Sergey Organov <sorganov@gmail.com>
Cc: "Jeff King" <peff@peff.net>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Elijah Newren" <newren@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"huang guanlong" <gl041188@gmail.com>,
	"Glen Choo" <chooglen@google.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'
Date: Sun, 18 Dec 2022 12:10:33 +0900	[thread overview]
Message-ID: <xmqqbko1xv86.fsf@gitster.g> (raw)
In-Reply-To: <20221217132955.108542-1-sorganov@gmail.com> (Sergey Organov's message of "Sat, 17 Dec 2022 16:29:50 +0300")

Sergey Organov <sorganov@gmail.com> writes:

> The last attempt to fix '-m' option for "git log" to imply '-p', to
> make it consistent with similar options (-c/--cc), was called by the
> request on the mailing list, here:

In retrospect, this old attempt may probably shouldn't have been
done, as there wasn't really a compelling need to change the
behaviour of "-m".  The "combined diff" options were "if specified,
give output" from day one, unlike "-m" which was "modify the
behaviour of '-p' if given" for a long time.  Changing any
established behaviour risks breaking the exising users and the
upside must outweigh the risk.  There wasn't overwhelming upside
back then to risk, and of course it backfired, ...

> However, the suggested (and accepted at first) patch series:
>
> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t
>
> appeared to have two problems:
>
> * --diff-merges options are incomplete and have no way to provide
>   exact existing '-m' semantics.
>
> * implying '-p' by '-m' by default broke some legacy uses of
>   "git log --firt-parent -m".

... like so.  Without learning from the experience, we may repeat
doing the same thing over and over and expecting different outcome,
but it would not give us a very good end-user experience if it
breaks them every time we try doing that.

> Due to this, the last patch of those patch series has been later
> reverted:
>
> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/
>
> effectively leaving the issue unresolved.

Fairly accurate description.

These patches do look like a good approach to solve the first point
among the "two problems" in the previous round. Thanks for working
on it.

IIRC, the previous round (why is this round marked as v1, by the
way?) was reviewed by some folks, so lets wait to hear from them
how this round does better.

Unfortunately, I do not think of any "solution" that would avoid
breaking folks, if its end goal is to flip the default, either by
hardcoding or with a configuration variable.  IOW, the other one
among the "two problems" in the previous round sounds unsolvable.
We should question if it was really an "issue" worth "resolving",
though.