Re: [PATCH 1/3] cherry-pick: add support to copy notes

[Thomas Rast <trast@inf.ethz.ch>]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast Cc'ed as he has been the primary force behind this line
> of "notes" usability.

Thanks for pointing this out to me.

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  builtin/revert.c  |   2 +
>>  sequencer.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  sequencer.h       |   2 +
>>  t/t3500-cherry.sh |  32 +++++++++++++
>>  4 files changed, 169 insertions(+), 3 deletions(-)
>
> "git cherry-pick" should help maintaining notes just like amend and
> rebase, but how should this interact with notes.rewrite.<command>,
> where the command is capable of doing this without an explicit
> option once you tell which notes need to be maintained?

Since we already have the notes.rewrite.<command> convention, it would
seem the obvious choice to line it up with the others.  The main
bikeshedding opportunity is whether this should be an exception and
default to false (all other commands default it to true).

Also: how does this interact with notes.rewriteRef and the corresponding
env vars?  Why?

How does it interact with 'cherry-pick -n' if this is done in sequence,
effectively squashing several commits (this use-case is actually
suggested by the manpage), if multiple source commits had notes?  Should
it respect notes.rewriteMode (and by default concatenate)?  (I don't
know if the sequencer state is expressive enough already to carry this
in a meaningful way across cherry-pick commands.)

A commit message and some docs would be a nice idea, too.

> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> index f038f34..79c1219 100755
> --- a/t/t3500-cherry.sh
> +++ b/t/t3500-cherry.sh

This file starts out with

  test_description='git cherry should detect patches integrated upstream

  This test cherry-picks one local change of two into master branch, and
  checks that git cherry only returns the second patch in the local branch
  '

So either your tests should go to a different file or the description
becomes stale and needs to be updated.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch