# [PATCH 1/7] t5520: test pulling multiple branches into an empty repository 21 messages from 2015-05-02 to 2015-05-06. Participants: Paul Tan, Eric Sunshine, Matthieu Moy, Stefan Beller, Junio C Hamano. Thread: https://gitlist.dev/t/39223 ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 0/7] Improve git-pull test coverage Message-ID: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-1-git-send-email-pyokagan%40gmail.com ``` This patch series improves test coverage of git-pull.sh from 50.9%[1] to 73.4%[2]. Take the test coverage reports with a grain of salt though, as there are known deficiencies[3]. This is part of my GSoC project to rewrite git-pull into a builtin. Improving test coverage helps to prevent regressions that could occur due to the rewrite. [1] http://pyokagan.github.io/git/20150430132408-a75942b//kcov-merged/git-pull.360f32c2.html [2] http://pyokagan.github.io/git/20150502145725-c94dfc2//kcov-merged/git-pull.360f32c2.html [3] https://github.com/pyokagan/git/commit/a7d6ab4677b97afd789b0ce860280c80f70f6e32 Paul Tan (7): t5520: test pulling multiple branches into an empty repository t5520: implement tests for no merge candidates cases t5520: test for failure if index has unresolved entries t5520: test work tree fast-forward when fetch updates head t5520: test --rebase with multiple branches t5520: test --rebase failure on unborn branch with index t5521: test --dry-run does not make any changes t/t5520-pull.sh | 119 ++++++++++++++++++++++++++++++++++++++++++++++++ t/t5521-pull-options.sh | 13 ++++++ 2 files changed, 132 insertions(+) -- 2.1.4 ``` ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 1/7] t5520: test pulling multiple branches into an empty repository Message-ID: <1430581035-29464-2-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-2-git-send-email-pyokagan%40gmail.com In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Commit d09e79cb ("allow pulling into an empty repository") introduced the ability to pull into an empty head. As pulling in multiple branches does not make sense in this context, git-pull explicitly fails when there is an empty head and multiple branches are specified. Add a test to ensure that this safeguard works. Signed-off-by: Paul Tan --- t/t5520-pull.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 227d293..01ae1bf 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -86,6 +86,18 @@ test_expect_success 'pulling into void does not remove new staged files' ' ) ' +test_expect_success 'refuse to pull multiple branches into void' ' + git branch test master && + test_when_finished "git branch -D test" && + git init cloned-multiple-branches && + test_when_finished "rm -rf cloned-multiple-branches" && + ( + cd cloned-multiple-branches && + test_must_fail git pull .. master test 2>out && + test_i18ngrep "Cannot merge multiple branches into empty head" out + ) +' + test_expect_success 'test . as a remote' ' git branch copy master && -- 2.1.4 ``` ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 2/7] t5520: implement tests for no merge candidates cases Message-ID: <1430581035-29464-3-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-3-git-send-email-pyokagan%40gmail.com In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Commit a8c9bef4 fully established the current advices given by git-pull for the different cases where git-fetch will not have anything marked for merge: 1. We're not on a branch, so there is no branch configuration. 2. We're on a branch, but there is no configuration for this branch. 3. We fetched from the configured remote, but the configured branch to merge didn't get fetched (either it doesn't exist, or wasn't part of the fetch refspec). 4. We fetched from the non-default remote, but didn't specify a branch to merge. We can't use the configured one because it applies to the default remote. 5. We fetched from a specified remote, and a refspec was given, but it ended up not fetching anything. Implement tests for the above 5 cases to ensure that the correct code paths are triggered for each of these cases. Signed-off-by: Paul Tan --- I'm having trouble hitting the 1st case without resorting to the hack below. A detached HEAD will always have no remote configured, and the code flow would make it such that case (4) is hit in the detached HEAD case instead of case (1). t/t5520-pull.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 01ae1bf..635ec02 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -122,6 +122,58 @@ test_expect_success 'the default remote . should not break explicit pull' ' test `cat file` = modified ' +test_expect_success 'fail if not on a branch' ' + cp .git/config .git/config.bak && + test_when_finished "cp .git/config.bak .git/config" && + git remote add test_remote . && + git checkout HEAD^{} && + test_when_finished "git checkout -f copy" && + cat >>.git/config <<-\EOF && + [branch ""] + remote = test_remote + EOF + test_must_fail git pull test_remote 2>out && + test_i18ngrep "You are not currently on a branch" out +' + +test_expect_success 'fail if no configuration for current branch' ' + git remote add test_remote . && + test_when_finished "git remote remove test_remote" && + git checkout -b test copy && + test_when_finished "git checkout -f copy && git branch -D test" && + test_config branch.test.remote test_remote && + test_must_fail git pull 2>out && + test_i18ngrep "There is no tracking information" out +' + +test_expect_success 'fail if upstream branch does not exist' " + git checkout -b test copy && + test_when_finished 'git checkout -f copy && git branch -D test' && + test_config branch.test.remote . && + test_config branch.test.merge refs/heads/nonexisting && + test_must_fail git pull 2>out && + test_i18ngrep \"Your configuration specifies to merge with the ref 'nonexisting'\" out +" + +test_expect_success 'fail if no branches specified with non-default remote' ' + git clone --bare . test_repo && + test_when_finished "rm -rf test_repo" && + git remote add test_remote test_repo && + test_when_finished "git remote remove test_remote" && + git checkout -b test master && + test_when_finished "git checkout -f master && git branch -D test" && + test_config branch.test.remote . && + test_must_fail git pull test_remote 2>out && + test_i18ngrep "you must specify a branch on the command line" out +' + +test_expect_success 'fail if wildcard spec does not match any refs' " + git checkout -b test copy && + test_when_finished 'git checkout -f copy && git branch -D test' && + test_must_fail git pull . 'refs/nonexisting1/*:refs/nonexisting2/*' 2>out && + test_i18ngrep 'There are no candidates for merging' out +" + test_expect_success '--rebase' ' git branch to-rebase && echo modified again > file && -- 2.1.4 ``` ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 3/7] t5520: test for failure if index has unresolved entries Message-ID: <1430581035-29464-4-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-4-git-send-email-pyokagan%40gmail.com In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Commit d38a30df (Be more user-friendly when refusing to do something because of conflict) introduced code paths to git-pull which will error out with user-friendly advices if the user is in the middle of a merge or has unmerged files. Implement tests to ensure that git-pull will not run, and will print these advices, if the user is in the middle of a merge or has unmerged files in the index. Signed-off-by: Paul Tan --- t/t5520-pull.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 635ec02..9f57e0d 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -174,6 +174,21 @@ test_expect_success 'fail if wildcard spec does not match any refs' " test_i18ngrep 'There are no candidates for merging' out " +test_expect_success 'fail if the index has unresolved entries' ' + git checkout -b third master^ && + test_when_finished "git checkout -f copy && git branch -D third" && + echo file >expected && + test_cmp expected file && + echo modified2 >file && + git commit -a -m modified2 && + test_must_fail git pull . second && + test_must_fail git pull . second 2>out && + test_i18ngrep "Pull is not possible because you have unmerged files" out && + git add file && + test_must_fail git pull . second 2>out && + test_i18ngrep "You have not concluded your merge" out +' + test_expect_success '--rebase' ' git branch to-rebase && echo modified again > file && -- 2.1.4 ``` ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 4/7] t5520: test work tree fast-forward when fetch updates head Message-ID: <1430581035-29464-5-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-5-git-send-email-pyokagan%40gmail.com In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Since commit b10ac50f (Fix pulling into the same branch), git-pull, upon detecting that git-fetch updated the current head, will fast-forward the working tree to the updated head commit. Implement tests to ensure that the fast-forward occurs in such a case, as well as to ensure that the user-friendly advice is printed upon failure. --- t/t5520-pull.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 9f57e0d..25d519d 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -189,6 +189,27 @@ test_expect_success 'fail if the index has unresolved entries' ' test_i18ngrep "You have not concluded your merge" out ' +test_expect_success 'fast-forwards working tree if branch head is updated' ' + git checkout -b third master^ && + test_when_finished "git checkout -f copy && git branch -D third" && + echo file >expected && + test_cmp expected file && + git pull . second:third 2>out && + test_i18ngrep "fetch updated the current branch head" out && + echo modified >expected && + test_cmp expected file +' + +test_expect_success 'fast-forward fails with conflicting work tree' ' + git checkout -b third master^ && + test_when_finished "git checkout -f copy && git branch -D third" && + echo file >expected && + test_cmp expected file && + echo conflict >file && + test_must_fail git pull . second:third 2>out && + test_i18ngrep "Cannot fast-forward your working tree" out +' + test_expect_success '--rebase' ' git branch to-rebase && echo modified again > file && -- 2.1.4 ``` ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 5/7] t5520: test --rebase with multiple branches Message-ID: <1430581035-29464-6-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-6-git-send-email-pyokagan%40gmail.com In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Since rebasing on top of multiple upstream branches does not make sense, since commit 51b2ead0 ("disallow providing multiple upstream branches to rebase, pull --rebase"), git-pull explicitly disallowed specifying multiple branches in the rebase case. Implement tests to ensure that git-pull fails and prints out the user-friendly error message in such a case. Signed-off-by: Paul Tan --- t/t5520-pull.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 25d519d..17c63ff 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -223,6 +223,14 @@ test_expect_success '--rebase' ' test $(git rev-parse HEAD^) = $(git rev-parse copy) && test new = $(git show HEAD:file2) ' + +test_expect_success '--rebase fails with multiple branches' ' + git reset --hard before-rebase && + test_must_fail git pull --rebase . copy master 2>out && + test_when_finished "rm -f out" && + test_i18ngrep "Cannot rebase onto multiple branches" out +' + test_expect_success 'pull.rebase' ' git reset --hard before-rebase && test_config pull.rebase true && -- 2.1.4 ``` ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 6/7] t5520: test --rebase failure on unborn branch with index Message-ID: <1430581035-29464-7-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-7-git-send-email-pyokagan%40gmail.com In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Commit 19a7fcbf (allow pull --rebase on branch yet to be born) special cases git-pull on an unborn branch in a different code path such that git-pull --rebase is still valid even though there is no HEAD yet. This code path still ensures that there is no index in order not to lose any staged changes. Implement a test to ensure that this check is triggered. Signed-off-by: Paul Tan --- I wonder if the .git/index check is still required, as git-read-tree -m will take into account entries in the index as well. t/t5520-pull.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 17c63ff..34f3780 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -417,6 +417,17 @@ test_expect_success 'pull --rebase works on branch yet to be born' ' test_cmp expect actual ' +test_expect_success 'pull --rebase fails on unborn branch with staged changes' ' + git init empty_repo2 && + test_when_finished "rm -rf empty_repo2" && + ( + cd empty_repo2 && + echo staged-file >staged-file && + git add staged-file && + test_must_fail git pull --rebase .. master + ) +' + test_expect_success 'setup for detecting upstreamed changes' ' mkdir src && (cd src && -- 2.1.4 ``` ## Paul Tan, 2015-05-02 15:37 Subject: [PATCH 7/7] t5521: test --dry-run does not make any changes Message-ID: <1430581035-29464-8-git-send-email-pyokagan@gmail.com> URL: https://gitlist.dev/e/1430581035-29464-8-git-send-email-pyokagan%40gmail.com In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Test that when --dry-run is provided to git-pull, it does not make any changes, namely: * --dry-run gets passed to git-fetch, so no FETCH_HEAD will be created and no refs will be fetched. * The index and work tree will not be modified. Signed-off-by: Paul Tan --- t/t5521-pull-options.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 453aba5..7c4d624 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -117,4 +117,17 @@ test_expect_success 'git pull --all' ' ) ' +test_expect_success 'git pull --dry-run' ' + git init clonedry && + test_when_finished "rm -rf clonedry" && + ( + cd clonedry && + git pull --dry-run "../parent" && + test_path_is_missing .git/FETCH_HEAD && + test_path_is_missing .git/refs/heads/master && + test_path_is_missing .git/index && + test_path_is_missing "file" + ) +' + test_done -- 2.1.4 ``` ## Eric Sunshine, 2015-05-03 02:42 Subject: Re: [PATCH 4/7] t5520: test work tree fast-forward when fetch updates head Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTBm9iN8bFEZx3rHzYhwapSk4wYcAQXiqtu_BzxNd9VSA%40mail.gmail.com In-Reply-To: <1430581035-29464-5-git-send-email-pyokagan@gmail.com> ``` On Sat, May 2, 2015 at 11:37 AM, Paul Tan wrote: > Since commit b10ac50f (Fix pulling into the same branch), git-pull, > upon detecting that git-fetch updated the current head, will > fast-forward the working tree to the updated head commit. > > Implement tests to ensure that the fast-forward occurs in such a case, > as well as to ensure that the user-friendly advice is printed upon > failure. Missing sign-off. > --- > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index 9f57e0d..25d519d 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -189,6 +189,27 @@ test_expect_success 'fail if the index has unresolved entries' ' > test_i18ngrep "You have not concluded your merge" out > ' > > +test_expect_success 'fast-forwards working tree if branch head is updated' ' > + git checkout -b third master^ && > + test_when_finished "git checkout -f copy && git branch -D third" && > + echo file >expected && > + test_cmp expected file && > + git pull . second:third 2>out && > + test_i18ngrep "fetch updated the current branch head" out && > + echo modified >expected && > + test_cmp expected file > +' > + > +test_expect_success 'fast-forward fails with conflicting work tree' ' > + git checkout -b third master^ && > + test_when_finished "git checkout -f copy && git branch -D third" && > + echo file >expected && > + test_cmp expected file && > + echo conflict >file && > + test_must_fail git pull . second:third 2>out && > + test_i18ngrep "Cannot fast-forward your working tree" out > +' > + > test_expect_success '--rebase' ' > git branch to-rebase && > echo modified again > file && > -- > 2.1.4 ``` ## Paul Tan, 2015-05-03 02:47 Subject: Re: [PATCH 4/7] t5520: test work tree fast-forward when fetch updates head Message-ID: URL: https://gitlist.dev/e/CACRoPnQx9g-VOP88%3DH19S5TqF6EkKcCFAQfTJ_6vvk2Qwsn5nA%40mail.gmail.com In-Reply-To: ``` On Sun, May 3, 2015 at 10:42 AM, Eric Sunshine wrote: > Missing sign-off. Right, whoops. Signed-off-by: Paul Tan ``` ## Matthieu Moy, 2015-05-04 08:04 Subject: Re: [PATCH 2/7] t5520: implement tests for no merge candidates cases Message-ID: URL: https://gitlist.dev/e/vpqvbg8u5sq.fsf%40anie.imag.fr In-Reply-To: <1430581035-29464-3-git-send-email-pyokagan@gmail.com> ``` Thanks for the patch. On overall, it looks good to me. Some minor comments below. Paul Tan writes: > Commit a8c9bef4 fully established the current advices given by git-pull > for the different cases where git-fetch will not have anything marked > for merge: > > 1. We're not on a branch, so there is no branch > configuration. Nit: you seem to be wrapping your lines with a rather short length. I would prefer reading this as a single line: 1. We're not on a branch, so there is no branch configuration. (62 columns) > --- > > I'm having trouble hitting the 1st case without resorting to the hack below. A > detached HEAD will always have no remote configured, and the code flow would > make it such that case (4) is hit in the detached HEAD case instead of case > (1). This should appear in comments in the test 'fail if not on a branch'. People reading your [branch ""] in the future won't look for below-triple-dash comments in the mailing-list archives ... And actually, it would be more user-friendly to trigger this error message in the normal senario, i.e. check for 1. before 4. in the code. This was most likely the intension of the programmer who wrote this error message. You may want to fix this now, or add a test_expect_failure which will become a test_expect_success when you replace git-pull.sh with builtin/pull.c. > t/t5520-pull.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index 01ae1bf..635ec02 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -122,6 +122,58 @@ test_expect_success 'the default remote . should not break explicit pull' ' > test `cat file` = modified > ' > > +test_expect_success 'fail if not on a branch' ' > + cp .git/config .git/config.bak && > + test_when_finished "cp .git/config.bak .git/config" && > + git remote add test_remote . && > + git checkout HEAD^{} && > + test_when_finished "git checkout -f copy" && > + cat >>.git/config <<-\EOF && > + [branch ""] > + remote = test_remote > + EOF > + test_must_fail git pull test_remote 2>out && > + test_i18ngrep "You are not currently on a branch" out It may make sense to grep only a subset of the string, to be less sensitive to rewords of error message. For example, just: test_i18ngrep "not currently on a branch" -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Matthieu Moy, 2015-05-04 08:09 Subject: Re: [PATCH 3/7] t5520: test for failure if index has unresolved entries Message-ID: URL: https://gitlist.dev/e/vpqmw1ku5l3.fsf%40anie.imag.fr In-Reply-To: <1430581035-29464-4-git-send-email-pyokagan@gmail.com> ``` Paul Tan writes: > Commit d38a30df (Be more user-friendly when refusing to do something > because of conflict) introduced code paths to git-pull which will error > out with user-friendly advices if the user is in the middle of a merge > or has unmerged files. When referencing a commit like this, you may want to add the author of the commit in Cc: to make sure he reads. I did read, so it's OK. And sorry for not testing my code enough ;-). The patch looks good to me. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Matthieu Moy, 2015-05-04 08:16 Subject: Re: [PATCH 0/7] Improve git-pull test coverage Message-ID: URL: https://gitlist.dev/e/vpqoam0sqp5.fsf%40anie.imag.fr In-Reply-To: <1430581035-29464-1-git-send-email-pyokagan@gmail.com> ``` Paul Tan writes: > Paul Tan (7): > t5520: test pulling multiple branches into an empty repository > t5520: implement tests for no merge candidates cases > t5520: test for failure if index has unresolved entries > t5520: test work tree fast-forward when fetch updates head > t5520: test --rebase with multiple branches > t5520: test --rebase failure on unborn branch with index > t5521: test --dry-run does not make any changes I did a semi-thourough review of the whole series, it looks good to me. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Stefan Beller, 2015-05-04 17:09 Subject: Re: [PATCH 5/7] t5520: test --rebase with multiple branches Message-ID: URL: https://gitlist.dev/e/CAGZ79kbSMbhW2FLK05CaQjJjSTqEurB8iyeEBgyA1wbWJ4g2vw%40mail.gmail.com In-Reply-To: <1430581035-29464-6-git-send-email-pyokagan@gmail.com> ``` On Sat, May 2, 2015 at 8:37 AM, Paul Tan wrote: > Since rebasing on top of multiple upstream branches does not make sense, > since commit 51b2ead0 ("disallow providing multiple upstream branches > to rebase, pull --rebase"), git-pull explicitly disallowed specifying > multiple branches in the rebase case. > > Implement tests to ensure that git-pull fails and prints out the > user-friendly error message in such a case. > > Signed-off-by: Paul Tan > --- > t/t5520-pull.sh | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index 25d519d..17c63ff 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -223,6 +223,14 @@ test_expect_success '--rebase' ' > test $(git rev-parse HEAD^) = $(git rev-parse copy) && > test new = $(git show HEAD:file2) > ' > + > +test_expect_success '--rebase fails with multiple branches' ' > + git reset --hard before-rebase && > + test_must_fail git pull --rebase . copy master 2>out && > + test_when_finished "rm -f out" && I think it would make sense to switch the previous 2 lines, because the test_when_finished should also be run if the test actually fails. The actual tested part may segfault, which would interrupt the && chain from reaching the test_when_finished line. And here, the line to test is meant to be the "git pull --rebase" line, so we'd assume it may be stopping there due to failures in the tested program. If you grep through the code base, you'll find lots of test_when_finished commands as the very first command (or the earliest spot where it makes sense). > + test_i18ngrep "Cannot rebase onto multiple branches" out > +' > + > test_expect_success 'pull.rebase' ' > git reset --hard before-rebase && > test_config pull.rebase true && > -- > 2.1.4 > ``` ## Junio C Hamano, 2015-05-04 17:35 Subject: Re: [PATCH 0/7] Improve git-pull test coverage Message-ID: URL: https://gitlist.dev/e/xmqqlhh4tfd0.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Matthieu Moy writes: > Paul Tan writes: > >> Paul Tan (7): >> t5520: test pulling multiple branches into an empty repository >> t5520: implement tests for no merge candidates cases >> t5520: test for failure if index has unresolved entries >> t5520: test work tree fast-forward when fetch updates head >> t5520: test --rebase with multiple branches >> t5520: test --rebase failure on unborn branch with index >> t5521: test --dry-run does not make any changes > > I did a semi-thourough review of the whole series, it looks good to me. Thanks. I did the same, and it looked OK. One test was duplicated with jc/merge series and caused a textual conflict, but that was nothing major. I didn't like the grepping of error messages alone as a way to diagnose the failure, though. When we expect the pull to fail without touching anything in the working tree, I'd prefer to see that tested explicitly (e.g. "if pull mistakenly tried to go ahead and touch this file that would be involved in the merge, its contents would change---let's make sure that does not happen by making sure the contents before and after are the same" kind of thing). ``` ## Junio C Hamano, 2015-05-04 19:24 Subject: Re: [PATCH 5/7] t5520: test --rebase with multiple branches Message-ID: URL: https://gitlist.dev/e/xmqqmw1krvqu.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Stefan Beller writes: >> +test_expect_success '--rebase fails with multiple branches' ' >> + git reset --hard before-rebase && >> + test_must_fail git pull --rebase . copy master 2>out && >> + test_when_finished "rm -f out" && > > I think it would make sense to switch the previous 2 lines, because the > test_when_finished should also be run if the test actually fails. Excellent. Thanks for carefully reading patches. ``` ## Paul Tan, 2015-05-05 10:39 Subject: Re: [PATCH 0/7] Improve git-pull test coverage Message-ID: URL: https://gitlist.dev/e/CACRoPnSJdMmJNQ4xrKr5Og6rZOjCm4Ad8rv5dVfTZod6d5pKWg%40mail.gmail.com In-Reply-To: ``` Hi Junio, On Tue, May 5, 2015 at 1:35 AM, Junio C Hamano wrote: > I didn't like the grepping of error messages alone as a way to > diagnose the failure, though. When we expect the pull to fail > without touching anything in the working tree, I'd prefer to see > that tested explicitly (e.g. "if pull mistakenly tried to go ahead > and touch this file that would be involved in the merge, its > contents would change---let's make sure that does not happen by > making sure the contents before and after are the same" kind of > thing). Yes, you're right. Some tests do need these additional checks. Will add them in the re-roll. Just to be sure, aside from checking that git-pull does not touch files it should not touch in the event of an error, I do believe we also need to check its error message, as printing out the correct user-friendly error messages is one of the features of git-pull. Regards, Paul ``` ## Paul Tan, 2015-05-05 16:00 Subject: Re: [PATCH 5/7] t5520: test --rebase with multiple branches Message-ID: URL: https://gitlist.dev/e/CACRoPnSeJT8MK5esRManC7a7v4D2jA4d8YOSvW90%2BrnoDxuhTQ%40mail.gmail.com In-Reply-To: ``` Hi, On Tue, May 5, 2015 at 1:09 AM, Stefan Beller wrote: > On Sat, May 2, 2015 at 8:37 AM, Paul Tan wrote: >> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh >> index 25d519d..17c63ff 100755 >> --- a/t/t5520-pull.sh >> +++ b/t/t5520-pull.sh >> @@ -223,6 +223,14 @@ test_expect_success '--rebase' ' >> test $(git rev-parse HEAD^) = $(git rev-parse copy) && >> test new = $(git show HEAD:file2) >> ' >> + >> +test_expect_success '--rebase fails with multiple branches' ' >> + git reset --hard before-rebase && >> + test_must_fail git pull --rebase . copy master 2>out && >> + test_when_finished "rm -f out" && > > I think it would make sense to switch the previous 2 lines, because the > test_when_finished should also be run if the test actually fails. > The actual tested part may segfault, which would interrupt the && chain > from reaching the test_when_finished line. And here, the line to test > is meant to be the "git pull --rebase" line, so we'd assume it may be > stopping there due to failures in the tested program. > > If you grep through the code base, you'll find lots of test_when_finished > commands as the very first command (or the earliest spot where it makes > sense). Hmm, thinking about it again, I actually don't really think there's that much of a need to "rm -f out" in this case ;-) But yes, I should go review the placements of the test_when_finished calls :-). Thanks. ``` ## Paul Tan, 2015-05-06 06:04 Subject: Re: [PATCH 2/7] t5520: implement tests for no merge candidates cases Message-ID: URL: https://gitlist.dev/e/CACRoPnQ5EaCmO-tmDxmNW_snFLbFNOq06wdF_p4FPHHv%2BHiJBA%40mail.gmail.com In-Reply-To: ``` Hi, On Mon, May 4, 2015 at 4:04 PM, Matthieu Moy wrote: > Paul Tan writes: >> I'm having trouble hitting the 1st case without resorting to the hack below. A >> detached HEAD will always have no remote configured, and the code flow would >> make it such that case (4) is hit in the detached HEAD case instead of case >> (1). > > This should appear in comments in the test 'fail if not on a branch'. > People reading your [branch ""] in the future won't look for > below-triple-dash comments in the mailing-list archives ... > > And actually, it would be more user-friendly to trigger this error > message in the normal senario, i.e. check for 1. before 4. in the code. > This was most likely the intension of the programmer who wrote this > error message. You may want to fix this now, or add a > test_expect_failure which will become a test_expect_success when you > replace git-pull.sh with builtin/pull.c. Ah, I just figured out how to trigger this error. Just for reference, before I forget: 1. HEAD must be detached (such that there is no configured remote) 2. The default remote "origin" must exist, so that git-fetch will succeed even though there is no configured remote for the detached HEAD. 3. git pull must be called with no arguments, to avoid (4). I guess everything is working as intended. I copied the different cases from the commit message of a8c9bef4, but I think that they should be re-ordered to match the logic in the code. Furthermore, cases (1) and (2) could probably be explained as: (1) git-fetch succeeded in fetching from the branch's or repo's default remote, but: (1a) We are not on a branch, so there will never be a configured upstream branch to merge with. (1b) We are on a branch, but there is no configured upstream branch to merge with. ``` ## Paul Tan, 2015-05-06 06:06 Subject: Re: [PATCH 2/7] t5520: implement tests for no merge candidates cases Message-ID: URL: https://gitlist.dev/e/CACRoPnQMAsy3%3DVoOkjF61QujPMa0L%3DWmMXqLHakcoyjxaSRx6g%40mail.gmail.com In-Reply-To: ``` On Wed, May 6, 2015 at 2:04 PM, Paul Tan wrote: > Ah, I just figured out how to trigger this error. Just for reference, > before I forget: Whoops, hit send too early. The test for case (1) would look something like: test_expect_success 'fail if not on a branch' ' git remote add origin . && test_when_finished "git remote rm origin" && git checkout HEAD^{} && test_when_finished "git checkout -f copy" && test_must_fail git pull 2>out && test_i18ngrep "You are not currently on a branch" out ' ``` ## Paul Tan, 2015-05-06 06:30 Subject: Re: [PATCH 0/7] Improve git-pull test coverage Message-ID: URL: https://gitlist.dev/e/CACRoPnRKcDcvRgMqQefXHhaoFs6s3UMb3CNvNOxo-Rt8hyYnFg%40mail.gmail.com In-Reply-To: ``` Hi Junio, On Tue, May 5, 2015 at 1:35 AM, Junio C Hamano wrote: > One test was duplicated with jc/merge series and caused a textual > conflict, but that was nothing major. I will be rebasing the git-pull tests on top of your jc/merge patch series (as it makes the git-pull rewrite easier), so I will remove that offending patch. Thanks, Paul ```