Re: notes.rewriteRef doesn't apply to rebases that skip the commit
On Apr 7, 2014, at 2:33 PM, Junio C Hamano wrote: > Kevin Ballard 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
notes.rewriteRef doesn't apply to rebases that skip the commit
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. 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. 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 said, I'd still understand if it didn't do that and lost my note. It would be unfortunate, but it would match today's behavior. I'm ok with copying over my notes when necessary, I just want Git to handle it when it's obviously correct (e.g. when the patch-id matches). --- 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. For reference, I've been using git v1.9.0. The v1.9.1 release notes don't mention anything notes-related so I assume these issues still exist. -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
Re: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP
On Sep 19, 2012, at 12:42 PM, Jeff King wrote: >> So I am in general OK with it, but if we are to go that route, we >> should make sure that the documentation makes it clear that blame >> follows whole-file renames without any special instruction before >> doing so. Otherwise, it again will send the same wrong message to >> people who try to use the "--follow" from their experience with >> "log", no? > > I guess it depends on your perspective. I can see the argument that > blame is already doing what --follow would ask for, and thus it is a > no-op. I think of it more as --follow is nonsensical for blame. But I > do not think either is wrong per se, and there is no reason not to help > people who come to git thinking the former. So yes, I think > documentation in either case is probably a good thing. > > I am a little lukewarm on my patch if only because of the precedent it > sets. There are a trillion options that revision.c parses that are not > necessarily meaningful or implemented for sub-commands that piggy-back > on its option parser. I'm not sure we want to get into manually > detecting and disallowing each one in every caller. I tend to agree with your final sentiment there. But the point that users may not realize that blame already follows is also valid. Perhaps we should catch --follow, as in your patch, but instead of saying that it's an unknown argument, just print out a helpful message saying blame already follows renames (and then continue with the blame anyway, so as to not set a precedent to abort on unknown-but-currently-accepted flags). -Kevin -- 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: [PATCH] Documentation/git-blame.txt: --follow is a NO-OP
On Sep 19, 2012, at 1:37 PM, Jeff King wrote: > On Wed, Sep 19, 2012 at 01:31:50PM -0700, Kevin Ballard wrote: > >>> I am a little lukewarm on my patch if only because of the precedent it >>> sets. There are a trillion options that revision.c parses that are not >>> necessarily meaningful or implemented for sub-commands that piggy-back >>> on its option parser. I'm not sure we want to get into manually >>> detecting and disallowing each one in every caller. >> >> I tend to agree with your final sentiment there. But the point that >> users may not realize that blame already follows is also valid. Perhaps >> we should catch --follow, as in your patch, but instead of saying that >> it's an unknown argument, just print out a helpful message saying blame >> already follows renames (and then continue with the blame anyway, so >> as to not set a precedent to abort on unknown-but-currently-accepted >> flags). > > Sure, that would probably make sense. Care to roll a patch with > suggested wording? Sadly, no. I'm not in a position to contribute to GPL code anymore, based on my current job (I'd have to jump through some hoops to get the ok to expose myself to that potential legal liability). -Kevin -- 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
