> 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. Earlier from the discussions I thought it would be ok to make separate rules for command taking arbitrary arguments(exec) and the command taking single option(fixup). But I also agree we can make the same rules and will remove it. > It might be a good idea to > improve the documentation before the function though. Okay, Maybe we can improve like below: update the current comment: # "exec_cmd_with_args" -- add an "exec cmd with args" line. with: # "_" -- add a space, like "fixup_-C" implies "fixup -C" and # "exec_cmd_with_args" add an "exec cmd with args" line. Thanks and Regards, Charvi.
