Re: [PATCH v5 09/11] rebase: support automatic notes copying

Thomas Rast <trast@student.ethz.ch> writes:

I did my first "rebase -i / commit --amend / rebase --continue" cycle with this series, and it correctly carried notes I keep track of Message-Id of original patches with. Good job.

After looking at the output from "git log refs/notes/amlog", I have a few observations, though.

 - "git notes add" and friends honor GIT_COMMITER_* and GIT_AUTHOR_* as
   usual.  Since I use post-applypatch hook to record the message-id from
   "git am", I ended up getting my notes written by original submitters.

 - The log messages are mostly useless.  The scripted implementation said
   "Annotate 6d8094a8aa32c30ff39c6b8609acb8d057ccb5e5" which was not the
   most informative, but the new one says "Notes added by 'git notes add'".

I haven't thought things through thoroughly, and I am not complaining, but it may be worth thinking about what the "commit"-ness of generations of notes tree _should_ mean.

As we are recording the history of how the set of my notes have grown over time, I think it is natural to expect that the user can record who is recording this notes tree at what time for what reason, just like a regular commit does.

 - "added by 'git notes add'" is just as immaterial as "commit log edited
   with vi".  'copied via notes.rebase.rewrite hook' would make much more
   sense.

 - "git notes add" already uses -m option to take the text used as note
   (i.e. payload of the commit); the user may need a way to affect the
   commit log message to record why the note is added (or modified).

 - As to the determination of committer/author identity, I think what the
   code currently does (i.e. honor the environment and user.* config) is
   perfectly sane, but at the same time I think it would be surprising for
   unsuspecting users.  We may want to advise users about this in the
   documentation.

But the above is merely my personal feeling on the "notes history".

The notes implementation (from the scripted one to the one in 'pu') seems to take quite a different view and is designed as if aspects other than the topology and the tree each "commit" object records are not useful at all, and commits are used to implement the notes history only because the ancestry might help when we later implement merges of notes histories. I think that could also be a _valid_ position to take.