From: Phillip Wood <phillip.wood@talktalk.net>
Date: Tue, 07 Nov 2017 14:32:40 GMT
Subject: Re: [PATCH v1 4/8] commit: move print_commit_summary() to libgit
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:
> 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.

> 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