Re: [PATCH v2 2/2] t3200: verify "branch --list" sanity when rebasing from detached HEAD

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 3, 2018, 08:00 UTC
Message-ID: <CAPig+cSrAN2LgL1dAEUoR4PJk-rUzHdqTusXm8MYUn7p6G4puQ@mail.gmail.com>
In-Reply-To: <20180403043101.4072-2-kaartic.sivaraam@gmail.com>

On Tue, Apr 3, 2018 at 12:31 AM, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 17 quoted lines

> From: Eric Sunshine <sunshine@sunshineco.com>
>
> "git branch --list" shows an in-progress rebase as:
>
>   * (no branch, rebasing <branch>)
>     master
>     ...
>
> However, if the rebase is started from a detached HEAD, then there is no
> <branch>, and it would attempt to print a NULL pointer. The previous
> commit fixed this problem, so add a test to verify that the output is
> sane in this situation.
>
> Note that the "detached HEAD" test case might actually fail in some cases
> as the actual output of "git branch --list" might contain remote branch
> names which is not considered by the test case as it is rare to happen
> in the test environment.

This paragraph was not in the original patch[1]. I _think_ what you are saying (which took a while to decipher) is that if a command such as "git checkout origin/next" ever gets inserted into the script before the test, the test will be fooled since "git branch --list" will show "detached HEAD origin/next" rather than "detached HEAD d3adb33f", the latter of which is what the test is expecting.

Unfortunately, this paragraph makes it sound as if the test can fail randomly (which, I believe, is not the case), and nobody would want a test added which is unreliable, thus this paragraph is not helping to sell this patch (in fact, it's actively hurting it). Ideally, the test should be entirely deterministic so that it can't be fooled like this. Rather than including this (harmful) paragraph in the commit message, let's ensure that the test is deterministic (see below).

[1]: https://public-inbox.org/git/20180325054824.GA56795@flurp.local/

Show 8 quoted lines

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -1246,6 +1247,29 @@ test_expect_success '--merged is incompatible with --no-merged' '
> +test_expect_success '--list during rebase from detached HEAD' '
> +       test_when_finished "reset_rebase && git checkout master" &&
> +       git checkout HEAD^0 &&

This is the line which I think is causing concern for you. If someone inserted a new test before this one which invoked "git checkout origin/next" (or something), then even after "git checkout HEAD^0", "git branch --list" would still report the unexpected "detached HEAD origin/next". Let's fix this, and make the test deterministic, by doing this instead:

    git checkout master^0 &&

Thanks.

Show 8 quoted lines

> +       oid=$(git rev-parse --short HEAD) &&
> +       FAKE_LINES="1 edit 2" &&
> +       export FAKE_LINES &&
> +       set_fake_editor &&
> +       git rebase -i HEAD~2 &&
> +       git branch --list >actual &&
> +       test_i18ngrep "rebasing detached HEAD $oid" actual
> +'