Re: [PATCH v1 4/8] commit: move print_commit_summary() to libgit

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Nov 7, 2017, 14:32 UTC
Message-ID: <7849f0e9-24eb-89db-3524-dffdb00d82ad@talktalk.net>
In-Reply-To: <xmqqlgjig3lv.fsf@gitster.mtv.corp.google.com>

On 07/11/17 03:38, Junio C Hamano wrote:

Show 13 quoted lines

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Move print_commit_summary() from builtin/commit.c to sequencer.c so it
>> can be shared with other commands. The function is modified by
>> changing the last argument to a flag so callers can specify whether
>> they want to show the author date in addition to specifying if this is
>> an initial commit.
> 
> A movement of a long function like this one really is easier if you
> did not make any other unnecessary change in the same patch and then
> made the change as a follow-up.

I'm not sure what you mean by unnecessary, the original code called a file-local function author_date_is_interesting(). That had to be changed in order to move the code, I guess it would have been clearer to make that change first and then move the modified code to sequencer.c in a separate commit.

Show 8 quoted lines

> The end result seemed sane.  
> 
> Do not use signed int as a collection of bits "flags", as it makes
> readers wonder if you are going to do some clever thing by treating
> the topmost bit somewhat special (e.g. "if (flags < 0)").  Unless
> you are indeed doing something clever like that, use "unsigned int"
> instead.
>

Thanks, I'll change it to an unsigned int.

Best Wishes

Phillip