On Mon, Feb 8, 2021 at 10:36 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Charvi Mendiratta <charvi077@xxxxxxxxx> 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. I agree with that, and discussed it with Eric. See: https://lore.kernel.org/git/CAPig+cSBVG0AdyqXH2mZp6Ohrcb8_ec1Mm_vGbQM4zWT_7yYxQ@xxxxxxxxxxxxxx/ The discussion was: ----------------------- > > > However, "fixup" is a very different beast. 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". It would work just as well to use simpler tokens such > > > as "fixup-C" and "fixup-c", in which case t/lib-rebase.sh might parse > > > them like this (note that I also dropped `g` from the `sed` action): > > > > > > fixup-*) > > > action=$(echo "$line" | sed 's/-/ -/');; > > > > I agree that "fixup" arguments are not arbitrary at all, but I think > > it makes things simpler to just use one way to encode spaces instead > > of many different ways. > > Is that the intention here, though? Is the idea that some day `fixup` > will accept arbitrary arguments thus needs to encode spaces? If not, > then mirroring the treatment given to `exec` confuses readers into > thinking that it will/should accept arbitrary arguments. I brought > this up in my review specifically because it was confusing to a person > (me) new to this topic and reading the patches for the first time. The > more specific and exact the code can be, the less likely it will > confuse readers in the future. ----------------------- > 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. I would be ok with dropping this patch. It might be a good idea to improve the documentation before the function though.
