Re: notes.rewriteRef doesn't apply to rebases that skip the commit

2014-04-07 Thread Junio C Hamano
Kevin Ballard ke...@sb.org writes:

 I’ve started using notes recently, and I have notes.rewriteRef set so that
 when I rebase, my notes will be kept. Unfortunately, it turns out that if a
 rebase deletes my local commit because it already exists in upstream, it
 doesn’t copy the note to the upstream commit. It seems perfectly reasonable to
 me to expect the note to be copied to the upstream commit, as it represents
 the same change.

That would cut both ways, depending on the use case.  I suspect that
those who use notes as remainder of what are still to be sent out
would appreciate the current behaviour.

 One complication I can see is when my local commit is deleted not because it
 exists upstream, but because it ends up being an empty commit due to the
 changes existing across multiple upstream commits. In this case I see no
 alternative but to have the note disappear. But I think that's acceptable.

Oh, no question about that.

 Another potential issues is if the commit exists upstream, but the surrounding
 context has changed enough that it contains a different patch-id. In this
 case, I would want Git to take the extra effort to correlate the upstream
 commit with my local one (it has the same message, modulo any Signed-Off-By
 lines, the same authorship info, and all the - and + lines in the diff are
 identical).

That would be an orthogonal improvement, I would think.  Such a
smarter patch-id may mistake it, but it is a moral equivalent
detection would not only be useful for copying notes, but also for
skipping the commit from getting replayed in the first place, no?

 On a semi-related note, I don't see why Git should be warning about
 notes.displayRef evaluating to a reference that doesn't exist. It doesn't
 exist because I haven't created any notes for that ref in this repository yet.
 But that doesn't mean I won't be creating them eventually, and when I do I
 want them to be displayed.

That also cuts both ways. I think a warning is primarily to let
those who mistyped the refname take notice.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: notes.rewriteRef doesn't apply to rebases that skip the commit

2014-04-07 Thread Kevin Ballard
On Apr 7, 2014, at 2:33 PM, Junio C Hamano gits...@pobox.com wrote:

 Kevin Ballard ke...@sb.org writes:
 
 I’ve started using notes recently, and I have notes.rewriteRef set so that
 when I rebase, my notes will be kept. Unfortunately, it turns out that if a
 rebase deletes my local commit because it already exists in upstream, it
 doesn’t copy the note to the upstream commit. It seems perfectly reasonable 
 to
 me to expect the note to be copied to the upstream commit, as it represents
 the same change.
 
 That would cut both ways, depending on the use case.  I suspect that
 those who use notes as remainder of what are still to be sent out
 would appreciate the current behavior.

It depends on how things are sent out. I know Git operates by sending all
patches to the ML, which are then reapplied, so they end up with a different
commit hash. But in most of the projects I've worked in, the main workflow
ends up with commits getting merged into master without getting rewritten. The
reason why I'm requesting this behavior is that committing without rewriting
isn't necessarily a strict rule.

For example, in the project that I'm using notes for, every commit needs to go
through Gerrit for code review. Normally it gets reviewed and merged into
origin/master without a rewrite, and my note is preserved. But sometimes
someone else will sneak a commit in first and I'll need to rebase. If I rebase
locally, that works, but Gerrit also offers to rebase my commit for me. And if
I let Gerrit do it, I still want my note to be preserved. In the end, there
should be no practical difference between me rebasing and Gerrit rebasing.

In general, Git doesn't know what the user is using notes for. If the user has
requested that notes persist through rewrite operations, it seems reasonable
that Git should recognize rewrites that happened remotely too, not just
locally.

As for your particular example of tracking what still needs to be sent out,
I'm not sure I understand that example. If I `git push` or use format-patch
and send an email, isn't that sending it out? Therefore I need to
update/delete my note explicitly. And if I want to track what hasn't made it
into origin/master yet, well, the origin/master ref already does that for me.

 Another potential issues is if the commit exists upstream, but the 
 surrounding
 context has changed enough that it contains a different patch-id. In this
 case, I would want Git to take the extra effort to correlate the upstream
 commit with my local one (it has the same message, modulo any Signed-Off-By
 lines, the same authorship info, and all the - and + lines in the diff are
 identical).
 
 That would be an orthogonal improvement, I would think.  Such a
 smarter patch-id may mistake it, but it is a moral equivalent
 detection would not only be useful for copying notes, but also for
 skipping the commit from getting replayed in the first place, no?

Perhaps, but replaying an empty commit already does nothing. Although I
suppose `git rebase` does have the `--keep-empty` flag, so it might be useful
there.

 On a semi-related note, I don't see why Git should be warning about
 notes.displayRef evaluating to a reference that doesn't exist. It doesn't
 exist because I haven't created any notes for that ref in this repository 
 yet.
 But that doesn't mean I won't be creating them eventually, and when I do I
 want them to be displayed.
 
 That also cuts both ways. I think a warning is primarily to let
 those who mistyped the refname take notice.

I get that, but I don't think that's particularly important. There's no
practical difference between typoing the ref in notes.displayRef and forgtting
to set up notes.displayRef in the first place. Git certainly can't warn about
the latter. And the warning about the former is quite annoying if I did not
in fact typo, but rather just haven’t created any notes in that ref yet.

-Kevin Ballard--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html