Re: [PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options

From: Junio C Hamano <gitster@pobox.com>
To: Charvi Mendiratta <charvi077@gmail.com>
Cc: git@vger.kernel.org, sunshine@sunshineco.com,
	christian.couder@gmail.com, phillip.wood123@gmail.com,
	Christian Couder <chriscool@tuxfamily.org>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options
Date: Mon, 08 Feb 2021 13:36:43 -0800	[thread overview]
Message-ID: <xmqq1rdq5jis.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20210208192528.21399-5-charvi077@gmail.com> (Charvi Mendiratta's message of "Tue, 9 Feb 2021 00:55:22 +0530")

Charvi Mendiratta <charvi077@gmail.com> writes:

> However, "fixup" is very different from "exec". Its arguments are not
> arbitrary at all, so there isn't a good reason to mirror the choice of
> "_" to represent a space, which leads to rather unsightly tokens such
> as "fixup_-C". Let's replace it with simpler tokens such as "fixup-C"
> and "fixup-c".

Sadly, I have to say that this change may be making the developer
experience worse.

To use the original, test writers only need to remember a single
rule: "when a single command needs to embed a SP, replace it with
underscore" regardless of which insn they are listing in FAKE_LINES.

Now they need to remember that rule only applies to exec, and merge
and fixup uses a different rule, namely, a SP immediately before a
dash must be removed.

So, if I didn't know you folks have invested enough hours in this
patch, I would have said not to do this, but it is such a small
change, its effect isolated to only those who would be writing tests
for "rebase -i", it may be OK to let them endure a bit additional
burden to remember an extra rule with this patch.  I dunno.