| Message ID | 20210129182050.26143-8-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 1d410cd8c25978c1591a7d35c9077745146c6129 |
| Headers | show |
| Series | rebase -i: add options to fixup command | expand |
On Fri, Jan 29, 2021 at 1:25 PM Charvi Mendiratta <charvi077@gmail.com> wrote: > Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > --- > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh > @@ -51,6 +53,8 @@ set_fake_editor () { > exec_*|x_*|break|b) > echo "$line" | sed 's/_/ /g' >> "$1";; > + merge_*|fixup_*) > + action=$(echo "$line" | sed 's/_/ /g');; What is "merge_" doing here? It doesn't seem to be used by this patch. The function comment above this code may also need to be updated to reflect this change. > diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh > @@ -0,0 +1,213 @@ > +#!/bin/sh > +# > +# Copyright (c) 2018 Phillip Wood Did Phillip write this script? Is this patch based upon an old patch from him? > +test_commit_message () { > + rev="$1" && # commit or tag we want to test > + file="$2" && # test against the content of a file > + git show --no-patch --pretty=format:%B "$rev" >actual-message && > + if test "$2" = -m > + then > + str="$3" && # test against a string > + printf "%s\n" "$str" >tmp-expected-message && > + file="tmp-expected-message" > + fi > + test_cmp "$file" actual-message > +} By embedding comments in the function itself explaining $1, $2, and $3, anyone who adds tests to this script in the future is forced to read the function implementation to understand how to call it. Adding function documentation can remove that burden. For instance: # test_commit_message <rev> -m <msg> # test_commit_message <rev> <path> # Verify that the commit message of <rev> matches # <msg> or the content of <path>. test_commit_message () { ... } The implementation of test_commit_message() is a bit hard to follow. It might be simpler to write it more concisely and directly like this: git show --no-patch --pretty=format:%B "$1" >actual && case "$2" in -m) echo "$3" >expect && test_cmp expect actual ;; *) test_cmp "$2" actual ;; esac > +test_expect_success 'setup' ' > + cat >message <<-EOF && > + amend! B > + ${EMPTY} > + new subject > + ${EMPTY} > + new > + body > + EOF Style nit: In Git test scripts, the here-doc body and EOF are indented the same amount as the command which opened the here-doc: cat >message <<-EOF && amend! B ... body EOF Also, `$EMPTY` is perfectly fine; no need to write it as `${EMPTY}`. > + ORIG_AUTHOR_NAME="$GIT_AUTHOR_NAME" && > + ORIG_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" && > + GIT_AUTHOR_NAME="Amend Author" && > + GIT_AUTHOR_EMAIL="amend@example.com" && > + test_commit "$(cat message)" A A1 A1 && > + test_commit A2 A && > + test_commit A3 A && > + GIT_AUTHOR_NAME="$ORIG_AUTHOR_NAME" && > + GIT_AUTHOR_EMAIL="$ORIG_AUTHOR_EMAIL" && Are the timestamps of these commits meaningful in this context? If not, another way to do this would be to assign the new author name/email values in a subshell so that the values do not need to be restored manually. For instance: ( GIT_AUTHOR_NAME="Amend Author" && GIT_AUTHOR_EMAIL="amend@example.com" && test_commit "$(cat message)" A A1 A1 && test_commit A2 A && test_commit A3 A ) && It's a matter of taste whether or not that is preferable, though. > + echo B1 >B && > + test_tick && > + git commit --fixup=HEAD -a && > + test_tick && Same question about whether the commit timestamps have any significance in these tests. If not, then these test_tick() calls mislead the reader into thinking that the timestamps are significant, thus it would make sense to drop them. > +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 && I see that you mirrored the implementation of FAKE_LINES handling of "exec" here for "fixup", 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 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/-/ -/');; In fact, the recognized set of options following "fixup" is so small, that you could even get by with simpler tokens "fixupC" and "fixupc": fixupC) action="fixup -C";; fixupc) actions="fixup -c";; Though it's subjective whether or not "fixupC" and "fixupc" are nicer than "fixup-C" and "fixup-c", respectively. > +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 && > + test_cmp_rev HEAD^ A && > + test_cmp_rev HEAD^{tree} A1^{tree} && > + test_commit_message HEAD expected-message && > + get_author HEAD >actual-author && > + test_cmp expected-author actual-author > +' This test seems out of place. I would expect to see it added in the patch which adds "amend!" functionality. Alternatively, if the intention really is to support "amend!" this early in the series in [6/9], then the commit message of [6/9] should talk about it. > +test_expect_success 'fixup -C with conflicts gives correct message' ' > + test_when_finished "test_might_fail git rebase --abort" && Is there a reason this isn't written as: test_when_finished "reset_rebase" && which is more common? Is there something non-obvious which makes reset_rebase() inappropriate in these tests? > + git checkout --detach A1 && > + 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 && > + test_cmp_rev HEAD^ conflicts && > + test_cmp_rev HEAD^{tree} A1^{tree} && > + test_write_lines "" edited >>expected-message && It feels clunky and fragile for this test to be changing "expected-message" which was created in the "setup" test and used unaltered up to this point. If the content of "expected-message" is really going to change from test to test (as I see it changes again in a later test), then it would be easier to reason about the behavior if each test gives "expected-message" the precise content it should have in that local context. As it is currently implemented, it's too difficult to follow along and remember the value of "expected-message" from test to test. It also makes it difficult to extend tests or add new tests in between existing tests without negatively impacting other tests. If each test sets up "expected-message" to the precise content needed by the test, then both those problems go away. > +test_expect_success 'multiple fixup -c opens editor once' ' > + test_when_finished "test_might_fail git rebase --abort" && > + 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" \ > + EXPECT_HEADER_COUNT=4 \ > + git rebase -i $base && > + test_cmp_rev $base HEAD^ && > + test 1 = $(git show | grep Modified-A3 | wc -l) > +' These days, we would phrase the last part of the test as: git show > raw && grep Modified-A3 raw >out && test_line_count = 1 out
On Tue, Feb 2, 2021 at 3:01 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Jan 29, 2021 at 1:25 PM Charvi Mendiratta <charvi077@gmail.com> wrote: > > Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > > --- > > diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh > > @@ -51,6 +53,8 @@ set_fake_editor () { > > exec_*|x_*|break|b) > > echo "$line" | sed 's/_/ /g' >> "$1";; > > + merge_*|fixup_*) > > + action=$(echo "$line" | sed 's/_/ /g');; > > What is "merge_" doing here? It doesn't seem to be used by this patch. Yeah, it's not used, but it might be a good thing to add this for consistency while at it. > The function comment above this code may also need to be updated to > reflect this change. Yeah, good suggestion. > > diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh > > @@ -0,0 +1,213 @@ > > +#!/bin/sh > > +# > > +# Copyright (c) 2018 Phillip Wood > > Did Phillip write this script? Is this patch based upon an old patch from him? Yeah, it might be a good idea to add a "Based-on-patch-by: Phillip ..." > > +test_commit_message () { > > + rev="$1" && # commit or tag we want to test > > + file="$2" && # test against the content of a file > > + git show --no-patch --pretty=format:%B "$rev" >actual-message && > > + if test "$2" = -m > > + then > > + str="$3" && # test against a string > > + printf "%s\n" "$str" >tmp-expected-message && > > + file="tmp-expected-message" > > + fi > > + test_cmp "$file" actual-message > > +} > > By embedding comments in the function itself explaining $1, $2, and > $3, anyone who adds tests to this script in the future is forced to > read the function implementation to understand how to call it. Adding > function documentation can remove that burden. For instance: > > # test_commit_message <rev> -m <msg> > # test_commit_message <rev> <path> > # Verify that the commit message of <rev> matches > # <msg> or the content of <path>. > test_commit_message () { > ... > } Good suggestion. > The implementation of test_commit_message() is a bit hard to follow. > It might be simpler to write it more concisely and directly like this: > > git show --no-patch --pretty=format:%B "$1" >actual && > case "$2" in > -m) echo "$3" >expect && test_cmp expect actual ;; I think we try to avoid many commands on the same line. > *) test_cmp "$2" actual ;; > esac In general I am not sure that using $1, $2, $3 directly makes things easier to understand, but yeah, with the function documentation that you suggest, it might be better to write the function using them directly. > > +test_expect_success 'setup' ' > > + cat >message <<-EOF && > > + amend! B > > + ${EMPTY} > > + new subject > > + ${EMPTY} > > + new > > + body > > + EOF > > Style nit: In Git test scripts, the here-doc body and EOF are indented > the same amount as the command which opened the here-doc: I don't think we are very consistent with this and I didn't find anything about this in CodingGuidelines. In t0008 and t0021 for example, the indentation is more like: cat >message <<-EOF && amend! B ... body EOF and I like this style, as it seems clearer than the other styles. [...] > > +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 && > > I see that you mirrored the implementation of FAKE_LINES handling of > "exec" here for "fixup", 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 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. > In fact, the recognized set of options following "fixup" is so small, > that you could even get by with simpler tokens "fixupC" and "fixupc": > > fixupC) > action="fixup -C";; > fixupc) > actions="fixup -c";; > > Though it's subjective whether or not "fixupC" and "fixupc" are nicer > than "fixup-C" and "fixup-c", respectively. I don't think this would scale nicely when the number of options grows for both "fixup" and "merge". > > +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 && > > + test_cmp_rev HEAD^ A && > > + test_cmp_rev HEAD^{tree} A1^{tree} && > > + test_commit_message HEAD expected-message && > > + get_author HEAD >actual-author && > > + test_cmp expected-author actual-author > > +' > > This test seems out of place. I would expect to see it added in the > patch which adds "amend!" functionality. > > Alternatively, if the intention really is to support "amend!" this > early in the series in [6/9], then the commit message of [6/9] should > talk about it. Yeah, I think it might be better to just remove everything related to "amend!" in this series and put it into the next one. [...] > > + git checkout --detach A1 && > > + 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 && > > + test_cmp_rev HEAD^ conflicts && > > + test_cmp_rev HEAD^{tree} A1^{tree} && > > + test_write_lines "" edited >>expected-message && > > It feels clunky and fragile for this test to be changing > "expected-message" which was created in the "setup" test and used > unaltered up to this point. If the content of "expected-message" is > really going to change from test to test (as I see it changes again in > a later test), then it would be easier to reason about the behavior if > each test gives "expected-message" the precise content it should have > in that local context. As it is currently implemented, it's too > difficult to follow along and remember the value of "expected-message" > from test to test. It also makes it difficult to extend tests or add > new tests in between existing tests without negatively impacting other > tests. If each test sets up "expected-message" to the precise content > needed by the test, then both those problems go away. Yeah, perhaps the global "expected-message" could be renamed for example "global-expected-message", and tests which need a specific one could prepare and use a custom "expected-message" (maybe named "custom-expected-message") without ever changing "global-expected-message". Thanks for your review!
Hi, On Tue, 2 Feb 2021 at 15:32, Christian Couder <christian.couder@gmail.com> wrote: [...] > > The function comment above this code may also need to be updated to > > reflect this change. > > Yeah, good suggestion. > Okay, I will add that . > > > diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh > > > @@ -0,0 +1,213 @@ > > > +#!/bin/sh > > > +# > > > +# Copyright (c) 2018 Phillip Wood > > > > Did Phillip write this script? Is this patch based upon an old patch from him? > > Yeah, it might be a good idea to add a "Based-on-patch-by: Phillip ..." > Oops, I forgot to add the trailer in this patch and will add it. > > > +test_commit_message () { > > > + rev="$1" && # commit or tag we want to test > > > + file="$2" && # test against the content of a file > > > + git show --no-patch --pretty=format:%B "$rev" >actual-message && > > > + if test "$2" = -m > > > + then > > > + str="$3" && # test against a string > > > + printf "%s\n" "$str" >tmp-expected-message && > > > + file="tmp-expected-message" > > > + fi > > > + test_cmp "$file" actual-message > > > +} > > > > By embedding comments in the function itself explaining $1, $2, and > > $3, anyone who adds tests to this script in the future is forced to > > read the function implementation to understand how to call it. Adding > > function documentation can remove that burden. For instance: > > > > # test_commit_message <rev> -m <msg> > > # test_commit_message <rev> <path> > > # Verify that the commit message of <rev> matches > > # <msg> or the content of <path>. > > test_commit_message () { > > ... > > } > > Good suggestion. > Agree, I will add the comments. > > The implementation of test_commit_message() is a bit hard to follow. > > It might be simpler to write it more concisely and directly like this: > > > > git show --no-patch --pretty=format:%B "$1" >actual && > > case "$2" in > > -m) echo "$3" >expect && test_cmp expect actual ;; > > I think we try to avoid many commands on the same line. > > > *) test_cmp "$2" actual ;; > > esac > > In general I am not sure that using $1, $2, $3 directly makes things > easier to understand, but yeah, with the function documentation that > you suggest, it might be better to write the function using them > directly. > Okay, I will update it. [...] > > > +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 && > > > + test_cmp_rev HEAD^ A && > > > + test_cmp_rev HEAD^{tree} A1^{tree} && > > > + test_commit_message HEAD expected-message && > > > + get_author HEAD >actual-author && > > > + test_cmp expected-author actual-author > > > +' > > > > This test seems out of place. I would expect to see it added in the > > patch which adds "amend!" functionality. > > > > Alternatively, if the intention really is to support "amend!" this > > early in the series in [6/9], then the commit message of [6/9] should > > talk about it. > > Yeah, I think it might be better to just remove everything related to > "amend!" in this series and put it into the next one. > Agree. Eric pointed at other patches also for amend! commit. So I also admit that to avoid confusion, I will remove all these amend! part and will add to other patch series. > [...] > > > > + git checkout --detach A1 && > > > + 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 && > > > + test_cmp_rev HEAD^ conflicts && > > > + test_cmp_rev HEAD^{tree} A1^{tree} && > > > + test_write_lines "" edited >>expected-message && > > > > It feels clunky and fragile for this test to be changing > > "expected-message" which was created in the "setup" test and used > > unaltered up to this point. If the content of "expected-message" is > > really going to change from test to test (as I see it changes again in > > a later test), then it would be easier to reason about the behavior if > > each test gives "expected-message" the precise content it should have > > in that local context. As it is currently implemented, it's too > > difficult to follow along and remember the value of "expected-message" > > from test to test. It also makes it difficult to extend tests or add > > new tests in between existing tests without negatively impacting other > > tests. If each test sets up "expected-message" to the precise content > > needed by the test, then both those problems go away. > I agree with it ... > Yeah, perhaps the global "expected-message" could be renamed for > example "global-expected-message", and tests which need a specific one > could prepare and use a custom "expected-message" (maybe named > "custom-expected-message") without ever changing > "global-expected-message". > ... Okay, I will change it. Thanks Eric and Christian for the suggestions and reviews. I will include all the changes in the next revision. Thanks and Regards, Charvi
On Tue, Feb 2, 2021 at 5:02 AM Christian Couder <christian.couder@gmail.com> wrote: > On Tue, Feb 2, 2021 at 3:01 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Jan 29, 2021 at 1:25 PM Charvi Mendiratta <charvi077@gmail.com> wrote: > > > + merge_*|fixup_*) > > > + action=$(echo "$line" | sed 's/_/ /g');; > > > > What is "merge_" doing here? It doesn't seem to be used by this patch. > > Yeah, it's not used, but it might be a good thing to add this for > consistency while at it. It confuses readers (as it did to me), causing them to waste brain-cycles trying to figure out why it's present. Thus, it would be better to add it when it's actually needed. The waste of brain-cycles and time is especially important on a project like Git for which reviewers and reviewer time are limited resources. > > > +# Copyright (c) 2018 Phillip Wood > > > > Did Phillip write this script? Is this patch based upon an old patch from him? > > Yeah, it might be a good idea to add a "Based-on-patch-by: Phillip ..." Agreed. > > The implementation of test_commit_message() is a bit hard to follow. > > It might be simpler to write it more concisely and directly like this: > > > > git show --no-patch --pretty=format:%B "$1" >actual && > > case "$2" in > > -m) echo "$3" >expect && test_cmp expect actual ;; > > I think we try to avoid many commands on the same line. For something this minor, it's not likely to matter but, of course, it could be split over two lines: -m) echo "$3" >expect && test_cmp expect actual ;; > > *) test_cmp "$2" actual ;; > > esac > > In general I am not sure that using $1, $2, $3 directly makes things > easier to understand, but yeah, with the function documentation that > you suggest, it might be better to write the function using them > directly. The direct $1, $2, etc. was just an example. It's certainly possible to give them names even in the rewritten code I presented. One good reason, however, for just using $1, $2, etc. is that $2 is not well defined; sometimes it's a switch ("-m") and sometimes its a pathname, so it's hard to invent a suitable variable name for it. Also, this function becomes so simple (in the rewritten version) that explicit variable names don't add a lot of value (the cognitive load is quite low because the function is so short). > > Style nit: In Git test scripts, the here-doc body and EOF are indented > > the same amount as the command which opened the here-doc: > > I don't think we are very consistent with this and I didn't find > anything about this in CodingGuidelines. > > In t0008 and t0021 for example, the indentation is more like: > > cat >message <<-EOF && > amend! B > ... > body > EOF > > and I like this style, as it seems clearer than the other styles. I performed a quick survey of the heredoc styles in the tests. Here are the results[1] of my analysis on the 'seen' branch: total-heredocs=4128 same-indent=3053 (<<EOF & body & EOF share indent) cat >expect <<-\EOF body EOF body-eof-indented=24 (body & EOF indented) cat >expect <<-\EOF body EOF body-indented=735 (body indented; EOF not) cat >expect <<-\EOF body EOF left-margin=316 (<<EOF indented; body & EOF not) cat >expect <<\EOF body EOF So, the indentation recommended in my review -- with 3053 instances out of 4128 heredocs -- is by far the most prevalent in the project. [1]: Note that there is a miniscule amount of inaccuracy in the numbers because there are a few cases in which heredocs contain other heredocs, and some scripts build heredocs piecemeal when constructing other scripts, and I didn't bother making my analysis script handle those few cases. The inaccuracy is tiny, thus not meaningful to the overall picture. > > I see that you mirrored the implementation of FAKE_LINES handling of > > "exec" here for "fixup", 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 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. Anyhow, it's a minor point, not worth expending a lot of time discussing. > > It feels clunky and fragile for this test to be changing > > "expected-message" which was created in the "setup" test and used > > unaltered up to this point. If the content of "expected-message" is > > really going to change from test to test (as I see it changes again in > > a later test), then it would be easier to reason about the behavior if > > each test gives "expected-message" the precise content it should have > > in that local context. As it is currently implemented, it's too > > difficult to follow along and remember the value of "expected-message" > > from test to test. It also makes it difficult to extend tests or add > > new tests in between existing tests without negatively impacting other > > tests. If each test sets up "expected-message" to the precise content > > needed by the test, then both those problems go away. > > Yeah, perhaps the global "expected-message" could be renamed for > example "global-expected-message", and tests which need a specific one > could prepare and use a custom "expected-message" (maybe named > "custom-expected-message") without ever changing > "global-expected-message". That would be fine, though I wondered while reviewing the patch if a global "expect-message" file was even needed since it didn't seem like very many tests used it (but I didn't spend a lot of time counting the exact number of tests due to the high cognitive load tracing how that file might mutate as it passed through each test). Another really good reason for avoiding having later tests depend upon mutations from earlier tests, if possible, is that it makes it easier to run tests selectively with --run or GIT_SKIP_TESTS.
On Wed, 3 Feb 2021 at 11:14, Eric Sunshine <sunshine@sunshineco.com> wrote: > > > What is "merge_" doing here? It doesn't seem to be used by this patch. > > > > Yeah, it's not used, but it might be a good thing to add this for > > consistency while at it. > > It confuses readers (as it did to me), causing them to waste > brain-cycles trying to figure out why it's present. Thus, it would be > better to add it when it's actually needed. The waste of brain-cycles > and time is especially important on a project like Git for which > reviewers and reviewer time are limited resources. > Okay, I will remove "merge_" from this patch series and maybe later will make separate patch for it and also adding its tests and updating t3430-rebase-merges.sh > > > > +# Copyright (c) 2018 Phillip Wood > > > > > > Did Phillip write this script? Is this patch based upon an old patch from him? > > > > Yeah, it might be a good idea to add a "Based-on-patch-by: Phillip ..." > > Agreed. > > > > The implementation of test_commit_message() is a bit hard to follow. > > > It might be simpler to write it more concisely and directly like this: > > > > > > git show --no-patch --pretty=format:%B "$1" >actual && > > > case "$2" in > > > -m) echo "$3" >expect && test_cmp expect actual ;; > > > > I think we try to avoid many commands on the same line. > > For something this minor, it's not likely to matter but, of course, it > could be split over two lines: > > -m) echo "$3" >expect && > test_cmp expect actual ;; > > > > *) test_cmp "$2" actual ;; > > > esac > > > > In general I am not sure that using $1, $2, $3 directly makes things > > easier to understand, but yeah, with the function documentation that > > you suggest, it might be better to write the function using them > > directly. > > The direct $1, $2, etc. was just an example. It's certainly possible > to give them names even in the rewritten code I presented. One good > reason, however, for just using $1, $2, etc. is that $2 is not well > defined; sometimes it's a switch ("-m") and sometimes its a pathname, > so it's hard to invent a suitable variable name for it. Also, this > function becomes so simple (in the rewritten version) that explicit > variable names don't add a lot of value (the cognitive load is quite > low because the function is so short). > Agree, and will update it. > > > Style nit: In Git test scripts, the here-doc body and EOF are indented > > > the same amount as the command which opened the here-doc: > > > > I don't think we are very consistent with this and I didn't find > > anything about this in CodingGuidelines. > > > > In t0008 and t0021 for example, the indentation is more like: > > > > cat >message <<-EOF && > > amend! B > > ... > > body > > EOF > > > > and I like this style, as it seems clearer than the other styles. > > I performed a quick survey of the heredoc styles in the tests. Here > are the results[1] of my analysis on the 'seen' branch: > > total-heredocs=4128 > > same-indent=3053 (<<EOF & body & EOF share indent) > > cat >expect <<-\EOF > body > EOF > > body-eof-indented=24 (body & EOF indented) > > cat >expect <<-\EOF > body > EOF > > body-indented=735 (body indented; EOF not) > > cat >expect <<-\EOF > body > EOF > > left-margin=316 (<<EOF indented; body & EOF not) > > cat >expect <<\EOF > body > EOF > > So, the indentation recommended in my review -- with 3053 instances > out of 4128 heredocs -- is by far the most prevalent in the project. > > [1]: Note that there is a miniscule amount of inaccuracy in the > numbers because there are a few cases in which heredocs contain other > heredocs, and some scripts build heredocs piecemeal when constructing > other scripts, and I didn't bother making my analysis script handle > those few cases. The inaccuracy is tiny, thus not meaningful to the > overall picture. > Okay, will update the indentation. [...] > > > 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. > I also agree that fixup will not accept arbitrary arguments, So I think to go with the method using fixup-*) (as suggested above). [...] > > Yeah, perhaps the global "expected-message" could be renamed for > > example "global-expected-message", and tests which need a specific one > > could prepare and use a custom "expected-message" (maybe named > > "custom-expected-message") without ever changing > > "global-expected-message". > > That would be fine, though I wondered while reviewing the patch if a > global "expect-message" file was even needed since it didn't seem like > very many tests used it (but I didn't spend a lot of time counting the > exact number of tests due to the high cognitive load tracing how that > file might mutate as it passed through each test). > > Another really good reason for avoiding having later tests depend upon > mutations from earlier tests, if possible, is that it makes it easier > to run tests selectively with --run or GIT_SKIP_TESTS. Agree, also for this patch series I think to remove all tests for amend!, change the test setup and will take care this time to remove the test dependency (in case of expected-message). Thanks and Regards, Charvi
Hi Eric Thanks for taking such a close look at this series. On 02/02/2021 02:01, Eric Sunshine wrote: > On Fri, Jan 29, 2021 at 1:25 PM Charvi Mendiratta <charvi077@gmail.com> wrote: > [...] >> + ORIG_AUTHOR_NAME="$GIT_AUTHOR_NAME" && >> + ORIG_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" && >> + GIT_AUTHOR_NAME="Amend Author" && >> + GIT_AUTHOR_EMAIL="amend@example.com" && >> + test_commit "$(cat message)" A A1 A1 && >> + test_commit A2 A && >> + test_commit A3 A && >> + GIT_AUTHOR_NAME="$ORIG_AUTHOR_NAME" && >> + GIT_AUTHOR_EMAIL="$ORIG_AUTHOR_EMAIL" && > > Are the timestamps of these commits meaningful in this context? I think we want to ensure that the timestamp of the commits created with the different author are different from the previous commits. We ought to be checking that the author date of the rebased commit matches the author date of the original commit and not the author date of the fixup commit created with the different author. > If not, another way to do this would be to assign the new author > name/email values in a subshell so that the values do not need to be > restored manually. For instance: > > ( > GIT_AUTHOR_NAME="Amend Author" && > GIT_AUTHOR_EMAIL="amend@example.com" && > test_commit "$(cat message)" A A1 A1 && > test_commit A2 A && > test_commit A3 A > ) && > > It's a matter of taste whether or not that is preferable, though. > >> + echo B1 >B && >> + test_tick && >> + git commit --fixup=HEAD -a && >> + test_tick && > > Same question about whether the commit timestamps have any > significance in these tests. Same answer as above - we want different author dates so we can check the original author date is not modified. I'm not sure that we are actually checking the dates yet though it looks like we only check the author name and email at the moment. Best Wishes Phillip > If not, then these test_tick() calls > mislead the reader into thinking that the timestamps are significant, > thus it would make sense to drop them. > >> +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 && > > I see that you mirrored the implementation of FAKE_LINES handling of > "exec" here for "fixup", 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 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/-/ -/');; > > In fact, the recognized set of options following "fixup" is so small, > that you could even get by with simpler tokens "fixupC" and "fixupc": > > fixupC) > action="fixup -C";; > fixupc) > actions="fixup -c";; > > Though it's subjective whether or not "fixupC" and "fixupc" are nicer > than "fixup-C" and "fixup-c", respectively. > >> +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 && >> + test_cmp_rev HEAD^ A && >> + test_cmp_rev HEAD^{tree} A1^{tree} && >> + test_commit_message HEAD expected-message && >> + get_author HEAD >actual-author && >> + test_cmp expected-author actual-author >> +' > > This test seems out of place. I would expect to see it added in the > patch which adds "amend!" functionality. > > Alternatively, if the intention really is to support "amend!" this > early in the series in [6/9], then the commit message of [6/9] should > talk about it. > >> +test_expect_success 'fixup -C with conflicts gives correct message' ' >> + test_when_finished "test_might_fail git rebase --abort" && > > Is there a reason this isn't written as: > > test_when_finished "reset_rebase" && > > which is more common? Is there something non-obvious which makes > reset_rebase() inappropriate in these tests? > >> + git checkout --detach A1 && >> + 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 && >> + test_cmp_rev HEAD^ conflicts && >> + test_cmp_rev HEAD^{tree} A1^{tree} && >> + test_write_lines "" edited >>expected-message && > > It feels clunky and fragile for this test to be changing > "expected-message" which was created in the "setup" test and used > unaltered up to this point. If the content of "expected-message" is > really going to change from test to test (as I see it changes again in > a later test), then it would be easier to reason about the behavior if > each test gives "expected-message" the precise content it should have > in that local context. As it is currently implemented, it's too > difficult to follow along and remember the value of "expected-message" > from test to test. It also makes it difficult to extend tests or add > new tests in between existing tests without negatively impacting other > tests. If each test sets up "expected-message" to the precise content > needed by the test, then both those problems go away. > >> +test_expect_success 'multiple fixup -c opens editor once' ' >> + test_when_finished "test_might_fail git rebase --abort" && >> + 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" \ >> + EXPECT_HEADER_COUNT=4 \ >> + git rebase -i $base && >> + test_cmp_rev $base HEAD^ && >> + test 1 = $(git show | grep Modified-A3 | wc -l) >> +' > > These days, we would phrase the last part of the test as: > > git show > raw && > grep Modified-A3 raw >out && > test_line_count = 1 out >
On Thu, Feb 4, 2021 at 5:47 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 02/02/2021 02:01, Eric Sunshine wrote: > > Are the timestamps of these commits meaningful in this context? > > I think we want to ensure that the timestamp of the commits created with > the different author are different from the previous commits. We ought > to be checking that the author date of the rebased commit matches the > author date of the original commit and not the author date of the fixup > commit created with the different author. Such date-checking would indeed make sense and would remove any potential confusion a reader might have concerning the manual test_tick() calls. (It's not super important, but I still lean toward dropping the test_tick() calls until they are actually needed simply to avoid confusing readers. I asked about it in my review since their purpose was unclear and I was genuinely wondering if I was overlooking the reason for their presence. Future readers of the code may experience the same puzzlement without necessarily having ready access to the original author of the code from whom to receive an answer.)
Hi Eric, > > +test_expect_success 'fixup -C with conflicts gives correct message' ' > > + test_when_finished "test_might_fail git rebase --abort" && > > Is there a reason this isn't written as: > > test_when_finished "reset_rebase" && > > which is more common? Is there something non-obvious which makes > reset_rebase() inappropriate in these tests? > I missed this earlier, but I confirmed before sending next version that as reset_rebase() removes the untracked files so we could not use this as otherwise it removes the fake-editor.sh file and will be required to set a fake editor for every test and also removes other untracked files which may be used to debug. So, I think it's okay with: test_when_finished "test_might_fail git rebase --abort" Thanks and regards, Charvi
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh index b72c051f47..e10e38060b 100644 --- a/t/lib-rebase.sh +++ b/t/lib-rebase.sh @@ -4,6 +4,7 @@ # # - override the commit message with $FAKE_COMMIT_MESSAGE # - amend the commit message with $FAKE_COMMIT_AMEND +# - copy the original commit message to a file with $FAKE_MESSAGE_COPY # - check that non-commit messages have a certain line count with $EXPECT_COUNT # - check the commit count in the commit message header with $EXPECT_HEADER_COUNT # - rewrite a rebase -i script as directed by $FAKE_LINES. @@ -33,6 +34,7 @@ set_fake_editor () { exit test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1" test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1" + test -z "$FAKE_MESSAGE_COPY" || cat "$1" >"$FAKE_MESSAGE_COPY" exit ;; esac @@ -51,6 +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');; "#") echo '# comment' >> "$1";; ">") diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh new file mode 100755 index 0000000000..832971ffdb --- /dev/null +++ b/t/t3437-rebase-fixup-options.sh @@ -0,0 +1,213 @@ +#!/bin/sh +# +# Copyright (c) 2018 Phillip Wood +# + +test_description='git rebase interactive fixup options + +This test checks the "fixup [-C|-c]" command of rebase interactive. +In addition to amending the contents of the commit, "fixup -C" +replaces the original commit message with the message of the fixup +commit. "fixup -c" also replaces the original message, but opens the +editor to allow the user to edit the message before committing. +' + +. ./test-lib.sh + +. "$TEST_DIRECTORY"/lib-rebase.sh + +EMPTY="" + +test_commit_message () { + rev="$1" && # commit or tag we want to test + file="$2" && # test against the content of a file + git show --no-patch --pretty=format:%B "$rev" >actual-message && + if test "$2" = -m + then + str="$3" && # test against a string + printf "%s\n" "$str" >tmp-expected-message && + file="tmp-expected-message" + fi + test_cmp "$file" actual-message +} + +get_author () { + rev="$1" && + git log -1 --pretty=format:"%an %ae" "$rev" +} + +test_expect_success 'setup' ' + cat >message <<-EOF && + amend! B + ${EMPTY} + new subject + ${EMPTY} + new + body + EOF + + sed "1,2d" message >expected-message && + + test_commit A A && + test_commit B B && + get_author HEAD >expected-author && + ORIG_AUTHOR_NAME="$GIT_AUTHOR_NAME" && + ORIG_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" && + GIT_AUTHOR_NAME="Amend Author" && + GIT_AUTHOR_EMAIL="amend@example.com" && + test_commit "$(cat message)" A A1 A1 && + test_commit A2 A && + test_commit A3 A && + GIT_AUTHOR_NAME="$ORIG_AUTHOR_NAME" && + GIT_AUTHOR_EMAIL="$ORIG_AUTHOR_EMAIL" && + git checkout -b conflicts-branch A && + test_commit conflicts A && + + set_fake_editor && + git checkout -b branch B && + echo B1 >B && + test_tick && + git commit --fixup=HEAD -a && + test_tick && + git commit --allow-empty -F - <<-EOF && + amend! B + ${EMPTY} + B + ${EMPTY} + edited 1 + EOF + test_tick && + git commit --allow-empty -F - <<-EOF && + amend! amend! B + ${EMPTY} + B + ${EMPTY} + edited 1 + ${EMPTY} + edited 2 + EOF + echo B2 >B && + test_tick && + FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a && + echo B3 >B && + test_tick && + git commit -a -F - <<-EOF && + amend! amend! amend! B + ${EMPTY} + B + ${EMPTY} + edited 1 + ${EMPTY} + edited 2 + ${EMPTY} + edited 3 + EOF + + GIT_AUTHOR_NAME="Rebase Author" && + GIT_AUTHOR_EMAIL="rebase.author@example.com" && + GIT_COMMITTER_NAME="Rebase Committer" && + GIT_COMMITTER_EMAIL="rebase.committer@example.com" +' + +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 && + test_cmp_rev HEAD^ B && + test_cmp_rev HEAD^{tree} A2^{tree} && + test_commit_message HEAD -m "A2" +' + +test_expect_success 'simple fixup -c works' ' + test_when_finished "test_might_fail git rebase --abort" && + 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_COMMIT_AMEND="Modified A2" \ + git rebase -i B && + test_cmp_rev HEAD^ B && + test_cmp_rev HEAD^{tree} A2^{tree} && + test_commit_message HEAD expected-fixup-message +' + +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 && + test_cmp_rev HEAD^ A && + test_cmp_rev HEAD^{tree} A1^{tree} && + test_commit_message HEAD expected-message && + get_author HEAD >actual-author && + test_cmp expected-author actual-author +' + +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 && + git checkout --theirs -- A && + git add A && + FAKE_COMMIT_AMEND=edited git rebase --continue && + test_cmp_rev HEAD^ conflicts && + test_cmp_rev HEAD^{tree} A1^{tree} && + test_write_lines "" edited >>expected-message && + test_commit_message HEAD expected-message && + get_author HEAD >actual-author && + test_cmp expected-author actual-author +' + +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 && + git reset --hard && + FAKE_COMMIT_AMEND=edited git rebase --continue && + test_commit_message HEAD -m "B" +' + +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_COMMIT_AMEND=squashed \ + FAKE_MESSAGE_COPY=actual-squash-message \ + git -c commit.status=false rebase -ik --signoff A && + git diff-tree --exit-code --patch HEAD branch -- && + test_cmp_rev HEAD^ A && + test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \ + actual-squash-message +' + +test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' ' + 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 && + test_cmp_rev HEAD^ A && + test_commit_message HEAD expected-message +' + +test_expect_success 'multiple fixup -c opens editor once' ' + test_when_finished "test_might_fail git rebase --abort" && + 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" \ + EXPECT_HEADER_COUNT=4 \ + git rebase -i $base && + test_cmp_rev $base HEAD^ && + test 1 = $(git show | grep Modified-A3 | wc -l) +' + +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_MESSAGE_COPY=actual-combined-message \ + git -c commit.status=false rebase -i A && + test_i18ncmp "$TEST_DIRECTORY/t3437/expected-combined-message" \ + actual-combined-message && + test_cmp_rev HEAD^ A +' + +test_done diff --git a/t/t3437/expected-combined-message b/t/t3437/expected-combined-message new file mode 100644 index 0000000000..a26cfb2fa9 --- /dev/null +++ b/t/t3437/expected-combined-message @@ -0,0 +1,21 @@ +# This is a combination of 4 commits. +# This is the 1st commit message: + +B + +# This is the commit message #2: + +# amend! B + +new subject + +new +body + +# The commit message #3 will be skipped: + +# A2 + +# This is the commit message #4: + +A3 diff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message new file mode 100644 index 0000000000..ab2434f90e --- /dev/null +++ b/t/t3437/expected-squash-message @@ -0,0 +1,51 @@ +# This is a combination of 6 commits. +# The 1st commit message will be skipped: + +# B +# +# Signed-off-by: Rebase Committer <rebase.committer@example.com> + +# The commit message #2 will be skipped: + +# fixup! B + +# The commit message #3 will be skipped: + +# amend! B +# +# B +# +# edited 1 +# +# Signed-off-by: Rebase Committer <rebase.committer@example.com> + +# This is the commit message #4: + +# amend! amend! B + +B + +edited 1 + +edited 2 + +Signed-off-by: Rebase Committer <rebase.committer@example.com> + +# This is the commit message #5: + +# squash! amend! amend! B + +edited squash + +# This is the commit message #6: + +# amend! amend! amend! B + +B + +edited 1 + +edited 2 + +edited 3 +squashed
Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> --- t/lib-rebase.sh | 4 + t/t3437-rebase-fixup-options.sh | 213 ++++++++++++++++++++++++++++++ t/t3437/expected-combined-message | 21 +++ t/t3437/expected-squash-message | 51 +++++++ 4 files changed, 289 insertions(+) create mode 100755 t/t3437-rebase-fixup-options.sh create mode 100644 t/t3437/expected-combined-message create mode 100644 t/t3437/expected-squash-message