patch, 7 partst5520: test pulling multiple branches into an empty repository

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(+)

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 <pyokagan@gmail.com>
---
 t/t5520-pull.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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 <pyokagan@gmail.com>
---

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(+)

Thanks for the patch. On overall, it looks good to me. Some minor comments below.

Paul Tan <pyokagan@gmail.com> writes:

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)

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.

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"

Hi,

On Mon, May 4, 2015 at 4:04 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

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.

On Wed, May 6, 2015 at 2:04 PM, Paul Tan <pyokagan@gmail.com> wrote:

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
'

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 <pyokagan@gmail.com>
---
 t/t5520-pull.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Paul Tan <pyokagan@gmail.com> writes:

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.

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(+)

On Sat, May 2, 2015 at 11:37 AM, Paul Tan <pyokagan@gmail.com> wrote:

Missing sign-off.

On Sun, May 3, 2015 at 10:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Right, whoops.

Signed-off-by: Paul Tan <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 <pyokagan@gmail.com>
---
 t/t5520-pull.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

On Sat, May 2, 2015 at 8:37 AM, Paul Tan <pyokagan@gmail.com> wrote:

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).

Stefan Beller <sbeller@google.com> writes:

Excellent.  Thanks for carefully reading patches.

Hi,

On Tue, May 5, 2015 at 1:09 AM, Stefan Beller <sbeller@google.com> wrote:

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.

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 <pyokagan@gmail.com>
---

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(+)

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 <pyokagan@gmail.com>
---
 t/t5521-pull-options.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Paul Tan <pyokagan@gmail.com> writes:

I did a semi-thourough review of the whole series, it looks good to me.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

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).

Hi Junio,

On Tue, May 5, 2015 at 1:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

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

Hi Junio,

On Tue, May 5, 2015 at 1:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

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