Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: > There's no need to display the annoying warning on every pull... only > the ones that are not fast-forward. Yes! And thanks to the previous two steps, the change to the code is quite obvious. I don't have to give any further comment on the part that changes code---it is well done, period. > This requires the tests to pick another base, so the merge is not > fast-forward. And in the cases where --ff-only is specified add > test_must_fail (since now they are non-fast-forward). I am not sure what this means. Existing tests pull histories that may or may not be descendants of the HEAD. If we do not change the pair of commits involved in the tests, i.e. if we do not apply many s/c0/c2/ replacement I see in the patch, some of these tests would change their behaviour with respect to their advice output (but not the outcome). The ones that pulled fast-forward would stop showing the warning, and that is a good effect of this change. We want to see that in the update to the tests, no? The ones that pulled non-fast-forward history would still show the warning as they used to, and that is also what we want to see after this change. Which means we want the tests to keep doing what they do, while adjusting what we expect out of the tested commands to the world that is made better with the code change in this patch. I do not think we need to add new tests to demonstrate the new behaviour, as they (i.e. when the test pulls fast-forwardable history, with various combinations of options and configuration) seem to be pretty well covered already. In other words, the changes the paragraph says that the commit made to the tests sound quite backwards. The actual changes to some of the tests do look sensible, testing both sides of the coin. I've looked at the changes to the tests, but cannot convince me that we are not making irrelevant changes to the tests, or losing coverage needlessly because of s/c0/c2/ (i.e. turning tests that used to check fast-forward situations into tests that check non-ff situations). > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > index 6774e9d86f..6b4adab8b1 100755 > --- a/t/t7601-merge-pull-config.sh > +++ b/t/t7601-merge-pull-config.sh > @@ -28,7 +28,7 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'pull.rebase not set' ' > - git reset --hard c0 && > + git reset --hard c2 && > git -c color.advice=always pull . c1 2>err && > test_decode_color <err >decoded && > test_i18ngrep "<YELLOW>hint: " decoded && This is not "keeping what the original test does and adjusting the expectation, to demonstrate how behaviour changed"; instead, we make sure that the message we originally gave and we intend to keep giving is shown in non-ff situation by choosing the current commit that won't allow a ff merge. This is OK if we did not lose test coverage---as long as we test that we no longer give the message in ff situation somewhere else, And that happens later, I think. > @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' ' > > ' > > -test_expect_success 'pull.rebase not set and pull.ff=true' ' > +test_expect_success 'pull.rebase not set (fast-forward)' ' > git reset --hard c0 && > + git pull . c1 2>err && > + test_i18ngrep ! "Pulling without specifying how to reconcile" err > +' This is the new test to check the other side of the coin. It sees how the original test to merge c1 into c0 would behave with the new code. We make sure we do not give the advice because it is irrelevant in this situation. So the above two are good, even though the way this patch updates tests is probably a bit more error prone than necessary. Since we have checked how the new code behave for fast-forward with this new test, the remainder of the entire test script can be modified to test only non-ff situation without losing test coverage? I am not sure if that is the case. > +test_expect_success 'pull.rebase not set and pull.ff=true' ' > + git reset --hard c2 && > test_config pull.ff true && > git pull . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' We are merely allowing fast-forward merges without unnecessary merge commits, but we are faced to merge c1 into c2, which is not ff. The command goes ahead and merges anyway, but why shouldn't we be seeing the message? I am puzzled. > test_expect_success 'pull.rebase not set and pull.ff=false' ' > - git reset --hard c0 && > + git reset --hard c2 && > test_config pull.ff false && > git pull . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and pull.ff=only' ' > - git reset --hard c0 && > + git reset --hard c2 && > test_config pull.ff only && > - git pull . c1 2>err && > + test_must_fail git pull . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' This used to test that fast-forwarding the HEAD from c0 to c2 would be done successfully without issuing the message. Shouldn't that still be true in the improved "do not complain on fast-forward" code? We seem to be losing test coverage by checking how pull.ff=only prevents the command from working in a non-ff merge. I'd stop my review on the tests here, but I generally think s/c0/c2/ done in this patch is a wrong thing to do. We are changing the condition under which the messages is given (we are narrowing it to avoid giving it when it is irrelevant), without changing the final outcome (even though we changed the condition to give the message, we didn't change what the final outcome of the pull command would be), so I'd strongly prefer testing the same set of scenarios and update the expectation to an improved reality. > test_expect_success 'pull.rebase not set and --rebase given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --rebase . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --no-rebase given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --no-rebase . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --ff given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --ff . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --no-ff given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --no-ff . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --ff-only given' ' > - git reset --hard c0 && > - git pull --ff-only . c1 2>err && > + git reset --hard c2 && > + test_must_fail git pull --ff-only . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > '
