Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Alex Henrie <alexhenrie24@gmail.com>]

2017-03-23 9:54 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
>> 2017-03-22 10:54 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
>>> Alex Henrie <alexhenrie24@gmail.com> 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