Hi John, Le 2022-01-04 à 16:45, John Cai a écrit :
A bug in pull.c causes merge and rebase functions to ignore rebase.autostash if it is only set in the config.
The reported bug only affects fast-forwards as far as I understand, so I don't think "merge and rebase" is the best wording here. Also, 'functions' is not super clear. The actual functions in the code are 'run_rebase' and 'run_merge', if that is what you are referring to. If you mean the different underlying "modes" of 'git pull', I'd phrase it more like "the underlying 'merge' or 'rebase' invocations" or something like that - but again, only the underlying 'git merge' is affected, and only for fast-forwards.
There are a couple of different scenarios that we need to be mindful of: 1. --autostash passed in through command line $ git pull --autostash merge/rebase should get --autostashed passed through 2. --rebase passed in, rebase.autostash set in config $ git config rebase.autostash true $ git pull --rebase merge/rebase should get --autostash from config 3. --no-autostash passed in $ git pull --no-autostash --no-autostash should be passed into merge/rebase 4. rebase.autostash set but --rebase not used $ git config rebase.autostash true $ git pull --autostash should not be passed into merge but not rebase
Usually we start the commit message by a description of the current behaviour, so in the case of a bugfix like here, a description of the exact behaviour that triggers the bug. As Tilman reported, this only affects fast-forwards, so that should be mentioned in your commit message. While what you wrote is not wrong per se (although I'm not sure what you meant with point 4), almost all of the behaviour is correct, apart from the (rebase + ff) case, so I would focus on that.
This change adjusts variable names to make it more clear which autostash setting it is modifying, and ensures --autostash is passed into the merge/rebase where appropriate.
As Junio already pointed out, I'm not sure the changes you propose are really clearer... I agree that adding yet another variable is unneeded.
Reported-by: "Tilman Vogel" <tilman.vogel@xxxxxx> Co-authored-by: "Philippe Blain" <levraiphilippeblain@xxxxxxxxx>
As I remarked in the other thread, I'd prefer if you remove that trailer.
Signed-Off-by: "John Cai" <johncai86@xxxxxxxxx> --- builtin/pull.c | 15 ++++++------ t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++
The existing tests for 'git pull --autostash' are in t5520, so I think it might make more sense to add any new tests there instead of t5521.
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 66cfcb09c5..28f551db8e 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' ' test_commit -C src two && test_must_fail git -C dst pull --no-ff --no-verify --verify ' +test_expect_success 'git pull --rebase --autostash succeeds on ff' ' + test_when_finished "rm -fr src dst actual" && + git init src && + test_commit -C src "initial" file "content" && + git clone src dst && + test_commit -C src --printf "more_content" file "more content\ncontent\n" && + echo "dirty" >>dst/file && + git -C dst pull --rebase --autostash >actual 2>&1 && + grep -q "Fast-forward" actual && + grep -q "Applied autostash." actual +'
This seems to test the same thing as t5520's "--rebase --autostash fast forward", so I don't think it's necessary to add this one.
+ +test_expect_success 'git pull --rebase with rebase.autostash succeeds on ff' ' + test_when_finished "rm -fr src dst actual" && + git init src && + test_commit -C src "initial" file "content" && + git clone src dst && + test_commit -C src --printf "more_content" file "more content\ncontent\n" && + echo "dirty" >>dst/file && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Fast-forward" actual && + grep -q "Applied autostash." actual +'
OK, this is the actual test that was failing.
+ +test_expect_success 'git pull --rebase --autostash succeeds on non-ff' ' + test_when_finished "rm -fr src dst actual" && + git init src && + test_commit -C src "initial" file "content" && + git clone src dst && + test_commit -C src --printf "src_content" file "src content\ncontent\n" && + test_commit -C dst --append "dst_content" file "dst content" && + echo "dirty" >>dst/file && + git -C dst pull --rebase --autostash >actual 2>&1 && + grep -q "Successfully rebased and updated refs/heads/main." actual && + grep -q "Applied autostash." actual +'
This seems to test the same thing as t5520's "pull --rebase --autostash & rebase.autostash unset"
+ +test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-ff' ' + test_when_finished "rm -fr src dst actual" && + git init src && + test_commit -C src "initial" file "content" && + git clone src dst && + test_commit -C src --printf "src_content" file "src content\ncontent\n" && + test_commit -C dst --append "dst_content" file "dst content" && + echo "dirty" >>dst/file && + test_config -C dst rebase.autostash true && + git -C dst pull --rebase >actual 2>&1 && + grep -q "Successfully rebased and updated refs/heads/main." actual && + grep -q "Applied autostash." actual +'
This seems to test the same thing as t5520's "pull --rebase succeeds with dirty working directory and rebase.autostash set". Thanks for working on this ! :) Philippe.
