2017-03-23 9:54 GMT-06:00 Junio C Hamano <gitster@xxxxxxxxx>: > Alex Henrie <alexhenrie24@xxxxxxxxx> writes: > >> 2017-03-22 10:54 GMT-06:00 Junio C Hamano <gitster@xxxxxxxxx>: >>> Alex Henrie <alexhenrie24@xxxxxxxxx> writes: >>>> No problem. Do I need to submit a second version of the patch with a >>>> test for `git -p log`? >>> >>> You do want to protect this "without an option, we default to >>> 'auto'" feature from future breakage, no? >> >> Yes, but I need to know whether you want a v2 of this patch with all >> of the changes including the new test, or a second patch that depends >> on the first patch and only adds the new test. > > Sorry, I misunderstood the question. > > In general, we prefer to have tests that protects the updated > behaviour in the same patch that makes code changes that brings in > the new behaviour, i.e. a single v2 patch with new test would be > more appropriate in this case. > > When people work on a large bugfix, especially one that needs > multiple steps, we sometimes see a patch that adds new tests that > describe the desired behaviour as failing tests first, and then > subsequent patches to the code to update the behaviour flip > "test_expect_failure" to "test_expect_success" as they fix the > behaviour. But for a small change like this one, that approach is > inappropriate. > > When a patch that was reviewed, deemed good enough and has been > already merged to the 'next' branch later turns out that it needs > further work (like "we do need some tests"), we do such necessary > updates as separate follow-up patches, simply because we promise > that 'next' won't be rewound and are not allowed to replace patches. > But this one is not yet in 'next', so we can freely take a > replacement patch. > > Hope this message makes it clear enough? Yes, that makes sense. I assume that when you talk about 'next', you mean 'master'? Unfortunately, I think I found a bug. Even when using `git -p`, the function pager_in_use() always returns false if the output is not a TTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty || (pager_in_use() && pager_use_color)` are redundant. If we want to use `git -p log` in a test, we'll have to change the behavior of pager_in_use(). Alternatively, we could use `GIT_PAGER_IN_USE=1 git log` instead. -Alex