Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <mehul.jain2029@gmail.com>
Date: Mar 18, 2016, 15:17 UTC
Message-ID: <CA+DCAeRe4ak1Ni7CADpJdd-=+dvKTTepHY6dYNfQV3ADpz5f6Q@mail.gmail.com>
In-Reply-To: <CAPig+cQHbGAB=XCVWtC1MuubWUFc0tT-+ZGD+hW18whJP_U0_Q@mail.gmail.com>

On Fri, Mar 18, 2016 at 9:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

> Since this is now a patch series rather than a single patch, another
> way to help reviewers is to use a cover letter (see git-format-patch
> --cover-letter) where you'd explain the changes, and, importantly,
> include an interdiff between the previous and current versions.

Point noted for the future patches.

Show 9 quoted lines

>> +test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '
>
> Why do titles of some of the new test titles have a ":" after "rebase"
> while other don't?
>
> Also, how about normalizing the titles so that the reader knows in
> which tests rebase.autostash is 'true' and in which it is 'false'?
> Presently, it's difficult to decipher what's being tested based only
> on the titles.

If it's so then how about the tests titles to be the following:

* pull --rebase: --autostash works with rebase.autoStash set true

* pull --rebase: --autostash works with rebase.autoStash set false

* pull --rebase: --no-autostash works with rebase.autoStash set true

* pull --rebase: --no-autostash works with rebase.autoStash set false

Earlier I tried to keep it as less verbose as possible (and probably made it hard to decipher). Does the above titles seems short and informative to you? If so then I will use them instead of earlier ones.

> Finally, shouldn't you also be testing --autostash and --no-autostash
> when rebase.autostash is not set?

If rebase.autoStash is not set then config.autostash will remain zero through out the process. What I want to point out is that rebase.autoStash , if not set, is equivalent to being set false. So adding tests regarding "--[no-]autostash with rebase.autoStash unset" seems equivalent to tests " pull --rebase: --autostash works with rebase.autoStash set false" and "pull --rebase: --no-autostash works with rebase.autoStash set false".

Thanks, Mehul