| Message ID | 20210208192528.21399-5-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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.
On Mon, Feb 8, 2021 at 10:36 PM Junio C Hamano <gitster@pobox.com> wrote: > > 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. 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: ----------------------- > > > 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: > > ----------------------- > > > > > 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.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh index e10e38060b..e6bd295c05 100644 --- a/t/lib-rebase.sh +++ b/t/lib-rebase.sh @@ -15,8 +15,8 @@ # specified line. # # "<cmd> <lineno>" -- add a line with the specified command -# ("pick", "squash", "fixup", "edit", "reword" or "drop") and the -# SHA1 taken from the specified line. +# ("pick", "squash", "fixup"|"fixup-C"|"fixup-c", "edit", "reword" or "drop") +# and the SHA1 taken from the specified line. # # "exec_cmd_with_args" -- add an "exec cmd with args" line. # @@ -53,8 +53,8 @@ set_fake_editor () { action="$line";; exec_*|x_*|break|b) echo "$line" | sed 's/_/ /g' >> "$1";; - merge_*|fixup_*) - action=$(echo "$line" | sed 's/_/ /g');; + merge-*|fixup-*) + action=$(echo "$line" | sed 's/-/ -/');; "#") echo '# comment' >> "$1";; ">") diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh index 945df2555b..36dee15c4b 100755 --- a/t/t3437-rebase-fixup-options.sh +++ b/t/t3437-rebase-fixup-options.sh @@ -112,7 +112,7 @@ test_expect_success 'setup' ' test_expect_success 'simple fixup -C works' ' test_when_finished "test_might_fail git rebase --abort" && git checkout --detach A2 && - FAKE_LINES="1 fixup_-C 2" git rebase -i B && + FAKE_LINES="1 fixup-C 2" git rebase -i B && test_cmp_rev HEAD^ B && test_cmp_rev HEAD^{tree} A2^{tree} && test_commit_message HEAD -m "A2" @@ -123,7 +123,7 @@ test_expect_success 'simple fixup -c works' ' git checkout --detach A2 && git log -1 --pretty=format:%B >expected-fixup-message && test_write_lines "" "Modified A2" >>expected-fixup-message && - FAKE_LINES="1 fixup_-c 2" \ + FAKE_LINES="1 fixup-c 2" \ FAKE_COMMIT_AMEND="Modified A2" \ git rebase -i B && test_cmp_rev HEAD^ B && @@ -134,7 +134,7 @@ test_expect_success 'simple fixup -c works' ' test_expect_success 'fixup -C removes amend! from message' ' test_when_finished "test_might_fail git rebase --abort" && git checkout --detach A1 && - FAKE_LINES="1 fixup_-C 2" git rebase -i A && + FAKE_LINES="1 fixup-C 2" git rebase -i A && test_cmp_rev HEAD^ A && test_cmp_rev HEAD^{tree} A1^{tree} && test_commit_message HEAD expected-message && @@ -145,7 +145,7 @@ test_expect_success 'fixup -C removes amend! from message' ' test_expect_success 'fixup -C with conflicts gives correct message' ' test_when_finished "test_might_fail git rebase --abort" && git checkout --detach A1 && - test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts && + test_must_fail env FAKE_LINES="1 fixup-C 2" git rebase -i conflicts && git checkout --theirs -- A && git add A && FAKE_COMMIT_AMEND=edited git rebase --continue && @@ -160,7 +160,7 @@ test_expect_success 'fixup -C with conflicts gives correct message' ' test_expect_success 'skipping fixup -C after fixup gives correct message' ' test_when_finished "test_might_fail git rebase --abort" && git checkout --detach A3 && - test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A && + test_must_fail env FAKE_LINES="1 fixup 2 fixup-C 4" git rebase -i A && git reset --hard && FAKE_COMMIT_AMEND=edited git rebase --continue && test_commit_message HEAD -m "B" @@ -168,7 +168,7 @@ test_expect_success 'skipping fixup -C after fixup gives correct message' ' test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' ' git checkout --detach branch && - FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \ + FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4 squash 5 fixup-C 6" \ FAKE_COMMIT_AMEND=squashed \ FAKE_MESSAGE_COPY=actual-squash-message \ git -c commit.status=false rebase -ik --signoff A && @@ -182,7 +182,7 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix test_when_finished "test_might_fail git rebase --abort" && git checkout branch && git checkout --detach branch~2 && git log -1 --pretty=format:%b >expected-message && - FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4" git rebase -i A && + FAKE_LINES="1 fixup 2 fixup-C 3 fixup-C 4" git rebase -i A && test_cmp_rev HEAD^ A && test_commit_message HEAD expected-message ' @@ -192,7 +192,7 @@ test_expect_success 'multiple fixup -c opens editor once' ' git checkout --detach A3 && base=$(git rev-parse HEAD~4) && FAKE_COMMIT_MESSAGE="Modified-A3" \ - FAKE_LINES="1 fixup_-C 2 fixup_-c 3 fixup_-c 4" \ + FAKE_LINES="1 fixup-C 2 fixup-c 3 fixup-c 4" \ EXPECT_HEADER_COUNT=4 \ git rebase -i $base && test_cmp_rev $base HEAD^ && @@ -202,7 +202,7 @@ test_expect_success 'multiple fixup -c opens editor once' ' test_expect_success 'sequence squash, fixup & fixup -c gives combined message' ' test_when_finished "test_might_fail git rebase --abort" && git checkout --detach A3 && - FAKE_LINES="1 squash 2 fixup 3 fixup_-c 4" \ + FAKE_LINES="1 squash 2 fixup 3 fixup-c 4" \ FAKE_MESSAGE_COPY=actual-combined-message \ git -c commit.status=false rebase -i A && test_i18ncmp "$TEST_DIRECTORY/t3437/expected-combined-message" \
"fixup" and "merge" mirrors the implementation of FAKE_LINES handling of "exec", but the cases are quite different. The argument to "exec" is arbitrary and can have any number of spaces embedded in it, which conflicts with the meaning of spaces in FAKE_LINES, which separate the individual commands in FAKE_LINES. Consequently, "_" was chosen as a placeholder in "exec" to mean "space". 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". Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> --- t/lib-rebase.sh | 8 ++++---- t/t3437-rebase-fixup-options.sh | 18 +++++++++--------- 2 files changed, 13 insertions(+), 13 deletions(-)