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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 20, 2019, 17:53 UTC
Message-ID: <xmqqsgpvvi3s.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <71c313d7-e08d-f62f-c52e-aabca0d97002@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 29 quoted lines

> 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()).

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?

Thanks.