On Sun, Sep 20, 2015 at 5:34 PM, Lars Schneider <larsxschneider@xxxxxxxxx> wrote: > On 20 Sep 2015, at 23:16, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >> On Sun, Sep 20, 2015 at 12:22 PM, <larsxschneider@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html