On 20/08/2019 18:53, Junio C Hamano wrote:
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()).
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.
