Charvi Mendiratta [off-list ref] 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.
On Mon, Feb 8, 2021 at 10:36 PM Junio C Hamano [off-list ref] wrote:
Charvi Mendiratta [off-list ref] writes:
quoted
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@mail.gmail.com/
The discussion was:
-----------------------
quoted
quoted
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.
I agree with that, and discussed it with Eric. See:
https://lore.kernel.org/git/CAPig+cSBVG0AdyqXH2mZp6Ohrcb8_ec1Mm_vGbQM4zWT_7yYxQ@mail.gmail.com/
The discussion was:
-----------------------
quoted
quoted
quoted
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.
-----------------------
quoted
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.