Phillip Wood <phillip.wood123@xxxxxxxxx> 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()).
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.
