Re: [PATCH v3 0/6] rebase -i: support more options

From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Rohit Ashiwal <rohit.ashiwal265@gmail.com>,
	Johannes.Schindelin@gmx.de, git@vger.kernel.org,
	martin.agren@gmail.com, newren@gmail.com, t.gummerer@gmail.com
Subject: Re: [PATCH v3 0/6] rebase -i: support more options
Date: Tue, 20 Aug 2019 19:37:16 +0100	[thread overview]
Message-ID: <c8bede99-8926-2bdc-a3ff-b8ee49bb6e34@gmail.com> (raw)
In-Reply-To: <xmqqsgpvvi3s.fsf@gitster-ct.c.googlers.com>

On 20/08/2019 18:53, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Hi Rohit
>>
>> On 20/08/2019 04:45, Rohit Ashiwal wrote:
>>> I've tries to incorporated all the suggestions.
>>
>> It is helpful if you can list the changes to remind us all what we
>> said. (as a patch author I find composing that is helpful to remind me
>> if there's anything I've forgotten to address)
>>
>> Also there are a couple of things that were discussed such as
>> splitting up the author and passing it round as a <name, email, date,
>> tz> tuple and testing a non-default timezone which aren't included -
>> that's fine but it helps if you take a moment to explain why in the
>> cover letter.
>>
>>>
>>> Some points:
>>>     - According to v2.0.0's git-am.sh, ignore-date should override
>>>       committer-date-is-author-date. Ergo, we are not barfing out
>>>       when both flags are provided.
>>>     - Should the 'const' qualifier be removed[2]? Since it is leaving
>>>       a false impression that author should not be free()'d.
>>
>> The author returned by read_author_ident() is owned by the strbuf that
>> you pass to read_author_ident() which is confusing.
>>
>> Best Wishes
>>
>> Phillip
> 
> I've looked at this round, but will push out v2 for today's
> integration cycle, mostly due to lack of time, but there do not seem
> to be great difference between the iterations.
> 
> The "ignore-date" step conflicts semantically with b0a31861
> ("sequencer: simplify root commit creation", 2019-08-19) but in a
> good way.  Without the clean-up b0a31861 makes, we need to munge the
> timestamp in two places, but with it, there is only one place that
> needs to modify the timestamp for the feature (in try_to_commit()).

That was my hope

> You may want to see if these "more options" topic can take advantage
> of the "simplify root commit creation" by building on top of some
> form of it (I do not know offhand if b0a31861 ("sequencer: simplify
> root commit creation", 2019-08-19) must build on other two patches
> to fix "rebase -i" or it can be split out as an independent
> clean-up), and if it is feasible, work with your student to make it
> happen, perhaps?

If it is separated out from the first two there will be a minor textual 
conflict as the first patch in that series changes the root commit 
creation to stop it writing CHERRY_PICK_HEAD and then b0a31861 deletes 
the modified version but it shouldn't be a problem to resolve manually. 
Rohit do you want to try cherry-picking b0a31861 onto master and then 
rebasing your patches on top of it? - it would be nice to simplify things.

Best Wishes

Phillip

> Thanks.
> 