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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2021, 21:36 UTC
Message-ID: <xmqq1rdq5jis.fsf@gitster.c.googlers.com>
In-Reply-To: <20210208192528.21399-5-charvi077@gmail.com>

Charvi Mendiratta <charvi077@gmail.com> writes:

Show 5 quoted lines

> 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.