{"thread":{"id":"36365","subject":"notes.rewriteRef doesn't apply to rebases that skip the commit","startedAt":"2014-04-07T20:26:47Z","lastAt":"2014-04-07T21:54:29Z","messageCount":3,"participants":["Kevin Ballard","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"238511","messageId":"99F95780-059D-4F62-A851-C43729BB9893@sb.org","threadId":"36365","inReplyTo":null,"subject":"notes.rewriteRef doesn't apply to rebases that skip the commit","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2014-04-07T20:26:47Z","receivedAt":"2014-04-07T20:26:47Z","isPatch":false,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"\nI’ve started using notes recently, and I have notes.rewriteRef set so that\nwhen I rebase, my notes will be kept. Unfortunately, it turns out that if a\nrebase deletes my local commit because it already exists in upstream, it\ndoesn’t copy the note to the upstream commit. It seems perfectly reasonable to\nme to expect the note to be copied to the upstream commit, as it represents\nthe same change.\n\nOne complication I can see is when my local commit is deleted not because it\nexists upstream, but because it ends up being an empty commit due to the\nchanges existing across multiple upstream commits. In this case I see no\nalternative but to have the note disappear. But I think that's acceptable.\n\nAnother potential issues is if the commit exists upstream, but the surrounding\ncontext has changed enough that it contains a different patch-id. In this\ncase, I would want Git to take the extra effort to correlate the upstream\ncommit with my local one (it has the same message, modulo any Signed-Off-By\nlines, the same authorship info, and all the - and + lines in the diff are\nidentical). That said, I'd still understand if it didn't do that and lost my\nnote. It would be unfortunate, but it would match today's behavior. I'm ok\nwith copying over my notes when necessary, I just want Git to handle it when\nit's obviously correct (e.g. when the patch-id matches).\n\n---\n\nOn a semi-related note, I don't see why Git should be warning about\nnotes.displayRef evaluating to a reference that doesn't exist. It doesn't\nexist because I haven't created any notes for that ref in this repository yet.\nBut that doesn't mean I won't be creating them eventually, and when I do I\nwant them to be displayed.\n\nFor reference, I've been using git v1.9.0. The v1.9.1 release notes don't\nmention anything notes-related so I assume these issues still exist.\n\n-Kevin Ballard\n"},{"id":"238512","messageId":"xmqqzjjwlt9p.fsf@gitster.dls.corp.google.com","threadId":"36365","inReplyTo":"99F95780-059D-4F62-A851-C43729BB9893@sb.org","subject":"Re: notes.rewriteRef doesn't apply to rebases that skip the commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-07T21:33:06Z","receivedAt":"2014-04-07T21:33:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Ballard <kevin@sb.org> writes:\n\n> I’ve started using notes recently, and I have notes.rewriteRef set so that\n> when I rebase, my notes will be kept. Unfortunately, it turns out that if a\n> rebase deletes my local commit because it already exists in upstream, it\n> doesn’t copy the note to the upstream commit. It seems perfectly reasonable to\n> me to expect the note to be copied to the upstream commit, as it represents\n> the same change.\n\nThat would cut both ways, depending on the use case.  I suspect that\nthose who use notes as remainder of what are still to be sent out\nwould appreciate the current behaviour.\n\n> One complication I can see is when my local commit is deleted not because it\n> exists upstream, but because it ends up being an empty commit due to the\n> changes existing across multiple upstream commits. In this case I see no\n> alternative but to have the note disappear. But I think that's acceptable.\n\nOh, no question about that.\n\n> Another potential issues is if the commit exists upstream, but the surrounding\n> context has changed enough that it contains a different patch-id. In this\n> case, I would want Git to take the extra effort to correlate the upstream\n> commit with my local one (it has the same message, modulo any Signed-Off-By\n> lines, the same authorship info, and all the - and + lines in the diff are\n> identical).\n\nThat would be an orthogonal improvement, I would think.  Such a\nsmarter \"patch-id may mistake it, but it is a moral equivalent\"\ndetection would not only be useful for copying notes, but also for\nskipping the commit from getting replayed in the first place, no?\n\n> On a semi-related note, I don't see why Git should be warning about\n> notes.displayRef evaluating to a reference that doesn't exist. It doesn't\n> exist because I haven't created any notes for that ref in this repository yet.\n> But that doesn't mean I won't be creating them eventually, and when I do I\n> want them to be displayed.\n\nThat also cuts both ways. I think a warning is primarily to let\nthose who mistyped the refname take notice.\n"},{"id":"238515","messageId":"047C96E4-80B6-431C-906C-D9DFFDBFE9FF@sb.org","threadId":"36365","inReplyTo":"xmqqzjjwlt9p.fsf@gitster.dls.corp.google.com","subject":"Re: notes.rewriteRef doesn't apply to rebases that skip the commit","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2014-04-07T21:54:29Z","receivedAt":"2014-04-07T21:54:29Z","isPatch":false,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Apr 7, 2014, at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Kevin Ballard <kevin@sb.org> writes:\n> \n>> I’ve started using notes recently, and I have notes.rewriteRef set so that\n>> when I rebase, my notes will be kept. Unfortunately, it turns out that if a\n>> rebase deletes my local commit because it already exists in upstream, it\n>> doesn’t copy the note to the upstream commit. It seems perfectly reasonable to\n>> me to expect the note to be copied to the upstream commit, as it represents\n>> the same change.\n> \n> That would cut both ways, depending on the use case.  I suspect that\n> those who use notes as remainder of what are still to be sent out\n> would appreciate the current behavior.\n\nIt depends on how things are sent out. I know Git operates by sending all\npatches to the ML, which are then reapplied, so they end up with a different\ncommit hash. But in most of the projects I've worked in, the main workflow\nends up with commits getting merged into master without getting rewritten. The\nreason why I'm requesting this behavior is that committing without rewriting\nisn't necessarily a strict rule.\n\nFor example, in the project that I'm using notes for, every commit needs to go\nthrough Gerrit for code review. Normally it gets reviewed and merged into\norigin/master without a rewrite, and my note is preserved. But sometimes\nsomeone else will sneak a commit in first and I'll need to rebase. If I rebase\nlocally, that works, but Gerrit also offers to rebase my commit for me. And if\nI let Gerrit do it, I still want my note to be preserved. In the end, there\nshould be no practical difference between me rebasing and Gerrit rebasing.\n\nIn general, Git doesn't know what the user is using notes for. If the user has\nrequested that notes persist through rewrite operations, it seems reasonable\nthat Git should recognize rewrites that happened remotely too, not just\nlocally.\n\nAs for your particular example of tracking what still needs to be sent out,\nI'm not sure I understand that example. If I `git push` or use format-patch\nand send an email, isn't that sending it out? Therefore I need to\nupdate/delete my note explicitly. And if I want to track what hasn't made it\ninto origin/master yet, well, the origin/master ref already does that for me.\n\n>> Another potential issues is if the commit exists upstream, but the surrounding\n>> context has changed enough that it contains a different patch-id. In this\n>> case, I would want Git to take the extra effort to correlate the upstream\n>> commit with my local one (it has the same message, modulo any Signed-Off-By\n>> lines, the same authorship info, and all the - and + lines in the diff are\n>> identical).\n> \n> That would be an orthogonal improvement, I would think.  Such a\n> smarter \"patch-id may mistake it, but it is a moral equivalent\"\n> detection would not only be useful for copying notes, but also for\n> skipping the commit from getting replayed in the first place, no?\n\nPerhaps, but replaying an empty commit already does nothing. Although I\nsuppose `git rebase` does have the `--keep-empty` flag, so it might be useful\nthere.\n\n>> On a semi-related note, I don't see why Git should be warning about\n>> notes.displayRef evaluating to a reference that doesn't exist. It doesn't\n>> exist because I haven't created any notes for that ref in this repository yet.\n>> But that doesn't mean I won't be creating them eventually, and when I do I\n>> want them to be displayed.\n> \n> That also cuts both ways. I think a warning is primarily to let\n> those who mistyped the refname take notice.\n\nI get that, but I don't think that's particularly important. There's no\npractical difference between typoing the ref in notes.displayRef and forgtting\nto set up notes.displayRef in the first place. Git certainly can't warn about\nthe latter. And the warning about the former is quite annoying if I did not\nin fact typo, but rather just haven’t created any notes in that ref yet.\n\n-Kevin Ballard"}]}