dusty-phillips / gitifyhg

Tools for using git as a client to mercurial repositories
GNU General Public License v3.0
62 stars 17 forks source link

Store hg sha1 using git notes (partial implementation of #4) #25

Closed jedbrown closed 11 years ago

jedbrown commented 11 years ago

This writes the hg sha1 into git notes, which can be queried using commands like

git log --notes=hg

The implementation currently favors safety by writing to a private notes ref refs/notes/hg-REMOTE where REMOTE is the configured remote name (e.g., origin). (I expect this to eventually change to a hash of the remote URL as discussed in issue #14.) This implementation currently requires the user to somehow run

git notes --ref hg merge hg-REMOTE*

as there does not appear to be a safe way for a remote helper to update the shared ref itself. I asked about this on the git mailing list (http://thread.gmane.org/gmane.comp.version-control.git/214802) and will hold off on hacks to commit the final merge.

Currently, notes are not applied in push operations, but they are applied in subsequent fetches.

jedbrown commented 11 years ago

For reference, this is an alternative to Max's pull request (#24) that avoids the race condition of updating a shared ref in favor of a deferred git notes --ref=hg merge hg-<remote>.

dusty-phillips commented 11 years ago

@fingolfin, @jedbrown: Do you two agree that this request is in a mergeable state? I've reviewed it and I love your code and your attention to organizing commits. However, I have not actually tested it and from the discussion, I think the two of you are in a better situation to say this code is complete (or shall we say, "complete enough for a 0.7 release").

If not, I would at least like to cherrypick the final commit (f9a0928), since it's useful in general and not directly related to notes support.

Both of you, thanks so much for your discussion and code. It's lovely to go away for a weekend and discover that your project has written itself. I <3 Open Source. ;-)

jedbrown commented 11 years ago

I think it's mergeable and that this is the right long-term approach for committing the notes. I've been using it with my projects and have had no problems. Issue #4 should stay open because we're still waiting to hear back from the git list on the right way to automate the final notes merge. I'm currently using an alias to get the notes merge done when I pull, but we eventually need something automatic.

fingolfin commented 11 years ago

In principle, I think this should be mergeable. The only remote reason I can think of why one might not want to merge it would be if we later discover that we need to do something about this differently, and then break existing clones in the process... but that seems remote... I think we'll have to ask people to throw away old clones and start fresh at some point (e.g. 1.0) anyway.

But I think jed's code is clean and extensible, and so the risk of problems seems low. So, fine by me to merge it :-).

fingolfin commented 11 years ago

Ah, wait, I do have an objection: the refs name currently seems to be refs/notes/hg-REMOTENAME, which will cause problems if the remote is renamed, see #14 -- I think the SHA-1 of the remote URL should be used instead.

jedbrown commented 11 years ago

I think issue #16 is the most likely "please reclone" moment, but even then, it won't be much code to remap the hg marks automatically. We should probably add a version attribute to marks-hg to make such updates more robust.

Regarding the ref name, I figured that refs/notes/hg-* should be renamed at the same time as #14 is implemented.

fingolfin commented 11 years ago

Looking at the code, it seems to me almost as if it was trying to, but fails... specifically:

        if alias[8:] == url:  # strips off 'gitifyhg::'
            alias = sha1(alias).hexdigest()
...
        output("commit refs/notes/hg-%s" % (self.hgremote.alias))

I don't understand that code looking at alias. First off, the prefix "gitifyhg::" is 10 bytes long, so I don't know where the 8 comes from. Indeed, it would be safer to check if alias == "gitifyhg::" + url:, wouldn't it?

Secondly, "alias" is always "origin" here, so this "if" is never triggered to start with...

I would recommend that instead of using this "alias" for the notes ref, we use a SHA-1 of the remote repo URL. This would later be used to to compute remotedir (as discussed on #14).

jedbrown commented 11 years ago

I believe that code was an inherited bug (from 0ae50da66d90c6ce913). It should be matching url, not alias. I thought about preparing a patch to fix that, but though I'd keep it out of this patch series pending discussion of backward-compatibility.

fingolfin commented 11 years ago

OK, so we agree that alias code is bogus, good. And yeah, it should be handled in a separate pull request. You know, I might just setup one for the remote name tracking.

dusty-phillips commented 11 years ago

FTR, I'm not personally too concerned about asking people to reclone just yet. gitifyhg works well for push/pull pipeline to branches, but it's still doing inappropriate things on clones (most notably dropping anonymous heads) that, when fixed, will need a reclone to work properly.

fingolfin commented 11 years ago

Right. And the notes code is still incomplete, as Jed documented: They only are applied in pulls, not in pushes. In fact, it seems to me that there is a bug there, too. I'll file a report.