Re: [PATCH v3 1/2] git-p4: add test case for "Translation of file content failed" error

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Sep 20, 2015 at 5:34 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> On 20 Sep 2015, at 23:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Sep 20, 2015 at 12:22 PM,  <larsxschneider@gmail.com> wrote:
>>> A P4 repository can get into a state where it contains a file with
>>> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
>>> attempts to retrieve the file then the process crashes with a
>>> "Translation of file content failed" error.
>>
>> Hmm, are these tests going to succeed only after patch 2/2 is applied?
>> If so, the order of these patches is backward since you want each
>> patch to be able to stand on its own and not introduce any sort of
>> breakage.
>
> Yes, these tests succeed only after 2/2. I think I saw this approach
> somewhere in the Git history. I thought it would ease the reviewing
> process: show the problem in the first commit, fix it in a
> subsequent commit.  However, I understand your point as 1/2 would
> break the build.

Yes, people sometimes do that, however, the patch which demonstrates
the problem uses test_expect_failure, and the follow-up patch which
fixes the problem flips it to test_expect_success.

> What is the preferred way by the Git community? Combine patch and
> test in one commit or a patch commit followed by a test commit? I
> would prefer to have everything in one commit.

If the tests are in a separate patch, Junio seems to prefer adding
them after the problem is fixes; the idea being that tests are added
to ensure that some future change doesn't break the feature, as
opposed to showing that your patch fixes a bug.

Whether or not to combine the fix with the new tests often depends
upon the length of the patches and how easy or hard it is to review
them. In this case, the fix itself is fairly short, but the tests are
slightly lengthy, so there may not be a clear cut answer. As a
reviewer, I tend to prefer smaller patches, however, this situation
doesn't demand it, so use your best judgment.