Re: [PATCH v3 2/9] t5520: ensure origin refs are updated

[Junio C Hamano <gitster@pobox.com>]

Paul Tan <pyokagan@gmail.com> writes:

> Should all of the tests before "setup for avoiding reapplying old
> patches" fail or be skipped, the repo "dst" will not have fetched the
> updated refs from origin. To be resilient against such failures, run
> "git fetch origin".
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
> * Hmm, no reviews the last round?

It is not unusual when the change is trivially correct.

I do not think this hurts, but I do not think that this is vastly
better, either.  If you suspect that the previous one may fail but
you at the same time are so trusting that the one before that one
would succeed, yes, this will help in that case.  But if you suspect
the previous one may fail, the one before that may have also failed,
in which case this may not be sufficient (the result of fetching may
not match what this test expects to see).

It all depends on where our paranoia ends. The current code is very
much trusting all previous ones equally, and accepts "upon the first
error, all bets are off for the later ones". With this patch, it
becomes slightly less trusting.

If everything else were equal, I would say this change is a "Meh" to
me, but I think the change improves this test in a different way.

It begins with "Run 'git rebase --abort', just in case"; which is a
signal that it does consider that the previous one may have failed
and attempts to prepare for that possibility, while trusting the one
before that would have succeeded.  And under that assumption, what
it currently does is _not_ consistent; the previous "pull --rebase"
may have failed in the "rebase" phase, in which case "abort just in
case" is a good measure to go back to the clean state, but it may
have failed in the "fetch" phase, in which case "abort" does not
help.  And this patch is needed to fix that inconsistency.

If justified in that way in the log message, then I wouldn't have
said "I do not think that this is vastly better", I think.

>  t/t5520-pull.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 20ad373..14a9280 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -339,6 +339,7 @@ test_expect_success 'git pull --rebase detects upstreamed changes' '
>  test_expect_success 'setup for avoiding reapplying old patches' '
>  	(cd dst &&
>  	 test_might_fail git rebase --abort &&
> +	 git fetch origin &&
>  	 git reset --hard origin/master
>  	) &&
>  	git clone --bare src src-replace.git &&