"Mark J. Nelson" <Mark.J.Nelson at Sun.COM> writes: >>>>>> #356: >>>>>> >>>>>> http://mail.opensolaris.org/pipermail/scm-migration-dev/2008-April/001864.html >>> >>>> After the recommit, the .hgtags file should not show up in the recommited >>>> cset...rather than stripping .hgtags maybe you ought to just toss it out >>>> and >>>> not include it in the recommit? >>> >>> I'm glad Dean sent this, it helped clarify the way I think this should >>> happen. >>> >>> Like Dean said, I think that our recommit code should disallow >>> modification of .hgtags, possibly allowing an override (for use by gk, >>> since they're also allowed to push csets that include that file). >>> >>> This does NOT account for localtags. I'm OK with modifying localtags, >>> mostly as per Rich's webrev above, but I think it would be nice if >>> >>> - we defer the cleanup 'til after the recommit >> >> It's currently done before the reci, since hgtags would be in the reci > > Right, but I think that ignoring .hgtags is more consistent with the > rest of our work flow and tools, wherein we don't allow the user to > push it anyway.
My response to this is mostly covered by your paragraph about project gates, below. >>> - we cleanup all hanging localtags, not just the ones we strip > >> If by hanging you mean, pointing off into nowhere, we currently >> cleanup any that *we* left hanging. I suppose I'm willing to try and >> remove any that point off to nowhere, but I'd rather not cover for >> problems that aren't our own. > > I'm ambivalent about that aspect, I just see the "cleanup localtags" > as something that can be cleanly separable from the recommit, since > localtags isn't under revision control or part of any csets. > >>> - we provide this as a command that can be run outside or reci >> >> Assuming "outside of reci", I'm wondering why. > > Yes, "of." Per above. Are you meaning just for localtags here? Or are you meaning a 'wx reset' equivalent that could be used on hgtags (or any other file, for that matter). The latter would be, and is, a separate RFE. I'm not seeing why doing this in a separate command for just localtags is useful. >>> - we report on which local tags were deleted >> >> Ok >> >>> Something that still bothers me: the removal of a tag, without >>> overriding it to refer to cset 0, can expose an older definition of >>> that same tag. But in localtags, I don't think we should override with >>> cset 0, because that would forever elide .hgtags. So I think that, in >>> addition to reporting on which local tags we deleted, we should also >>> report on which of those, if any, are implicitly redefined by said >>> deletion. >> >> That may not be much fun, but I guess we can try. > > I don't think it's all that difficult. You can pretty easily keep a > list of the tags that you delete from localtags, and it's not hard to > do listtags after the recommit, and cross reference the tag names. ? The initial code I had that reported this to the user would list each instance of a tag that was removed, so we were going through the files in question ourselves. Doing it as you describe above is, yes, easier. >>> Am I making this too difficult? >> >> You're covering cases I never intended the bug to, I tried to explain >> this to you yesterday on IRC. > > I understood, it just seems like we're bolting unnecessary complexity > onto our recommit. The .hgtags cleanup (and maybe the localtags one, > too) belongs in repair.strip(), and since we're not going to let folks > push hgtags anyway, going out of our way to clean it up seems wrong. Regarding .hgtags, again, project gates, where I could see uses for real tags. Per our conversation on IRC, I too would prefer this was dealt with by Hg, but I don't think waiting on a change there, and a release, is what we need to do. >> The case I was aiming to cover was that when you reci, we remove >> certain csets, those csets may have tags pointing to them. I intended >> to cover the case of *only* those tags (that point to node reci would >> remove), not other tags the user added, or any other form of user >> modification, error or otherwise. > > OK, I can see limiting the cleanup to nodes that we're actually > deleting, but (again) I think that doing so pushes the logic into > reci, when it belongs in strip(). Strip would no doubt do the same thing, remove tags that reference nodes it will remove. So yes, the logic is with us rather than them, but it would (I would assume) be the same logic, to the same end. >> I could understand why you'd like that done, but I still think fooling >> around with something the user chose to do, and that would remain >> consistent (if not something we'd like) is wrong. > > So I agree, but it's kind of a lesser of two evils. > > I think it's evil to mess with stuff that's "not our problem," but > also to include this logic in our code instead of hg. Right, Ideally I would like Hg to do this, but I'd prefer not to block on an Hg release after that fix. > A compromise solution, in my mind, is to file a bug against hg, AND to > provide standalone extensions to fill the gap 'til that bug is > fixed. Standalone as in outside of cdm, outside of reci? I'm wondering how you'd see that implemented. Making people do something else, by hand, to clean up what is clearly an error condition bothers me. > At a minimum, those extensions should handle localtags, and I'm OK > either way with whether they fix up .hgtags or ignore it. Slight > preferenc (I think I just waffled here) for fixing it up, because (I > just realized) we DO want to allow project-gate style management > wherein developers collapse deltas (relative to project gate), but > wherein developers are also allowed to set .hgtags. Right. >> You're advocating dropping .hgtags on the floor (which I actually used >> to do, and I decided that was wrong), and stripping everything >> unreachable out of localtags. That is more far-reaching in effect >> than what I'd been intending to cover, and you don't really justify >> that in this mail (I can imagine justification for removing any tag >> that's dangling in general, but not necessarily the rest). > > Dean, you actually hit this case in practice, right? Was that > contrived, or the result of a normal workflow? I have hit it in practice, I often use localtags pointing to the changeset I last sent out a webrev for. I haven't hit it with real tags, because I don't use real tags, because I know I shouldn't push them (which is to say, that may not be a great example of common use). What I've been doing to deal with that situation, is to switch to the cdm in this workspace to reci those workspaces. > This conversation is probably already too long and involved, and I'm > not deeply offended by Rich's solution, only I hate putting this code > here when it really, truly belongs in repair.strip(). As above, I'd prefer Hg do it too. I have no problem with however long it takes us to reach a point where we know what the right behaviour for cdm is (aside from whether Hg should cover these bases or not). I'm far more concerned with these bits being correct than them being available quickly (though I suspect everyone else disagrees with that, on some level). -- Rich
