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

Add some simple notes support #24

Closed fingolfin closed 11 years ago

fingolfin commented 11 years ago

This is a very early proof-of-concept work of adding notes support, see the discussion on issue #4. This should NOT be merged, as there are multiple issues with it. Partly because things are not implemented yet, partly because I do not yet fully understand how git notes work. Here are some things that need to be done.

  1. It only adds notes when importing new commits; if you add commits in git, then git push no note is added to the git commit, not even when you git pull again.
  2. Actually, it won't work after the initial import either; if there are new commits made on the hg side then this happens:
$ git pull
warning: Not updating refs/notes/hg (new tip SOME_SHA does not contain OTHER_SHA)
fatal: Error while running fast-import
$

This is because notes are managed in commits, and the commits my hack generates do not bother to specify the correct parent commit. They really should. (I am somewhat surprised that cloning a repo with more than one commit works with this hack... clearly there is more I am not understanding).

  1. What should be used as the committer and commit message for these not commits? I guess the committer could be something like "gitifyhg ", the message could be empty or contain information about the import (e.g. when it happened, which revisions where imported, the weather forecast...)
  2. If I understand correctly how notes work, then we can have multiple notemodify messages in a single notes commit there. So instead of adding each note in its own "commit", we should just add all notes at once in a single commit to refs/notes/hg.

I am sure there are more problems. But at least now we (well, at least I, not sure how helpful my rambling are :-) have a slightly better idea of what needs to be done.

fingolfin commented 11 years ago

To see the notes, use git log --notes=hg, which gives me in a small test repo I made:

commit 2184b4d9fdac3986d3eb789baf65376d0e9c37ad
Author: Max Horn <max@quendi.de>
Date:   Fri Jan 25 23:16:59 2013 +0100

    A

Notes (hg):
    5ceaf606bc135fb4d3f847ec1a1bcf7ac98d6b1f

commit 35f43572f6db553cc0aad6be68c0cee64a942709
Author: Max Horn <max@quendi.de>
Date:   Fri Jan 25 23:16:59 2013 +0100

    b

Notes (hg):
    38615e5414f47c76e62cdabba9ca2fff6617faf3

commit 0df24dde49adbd993916a4db369cc334cbc352e9
Author: Max Horn <max@quendi.de>
Date:   Fri Jan 25 23:16:59 2013 +0100

    a

Notes (hg):
    d6534f89fe79f4145c0ceac7daca3f39b4b44ab1
fingolfin commented 11 years ago

I addressed points 2 and 4 from above. Here is the shell script I used to test this:

#!/bin/sh -ex
mkdir repo_orig
cd repo_orig
hg init
echo a > a
hg add a
hg commit -m a
echo b > b
hg add b
hg commit -m b
echo A > a
hg commit -m A a
cd ..

git clone gitifyhg::repo_orig repo_clone

cd repo_clone
git log --notes=hg
cd ..

cd repo_orig
echo c > c
hg add c
hg commit -m c
cd ..

cd repo_clone
git pull
git log --notes=hg
fingolfin commented 11 years ago

Yes, you are right, it is racy (besides other issues it has) -- it is indeed a gross hack, just meant as a very rough POC to explore the problem space (and help me wrap my brain around it).

And you are pointing out a serious issue there. I am very glad you are interested in this, too :-).

So, it seems that there is just no way a fast-import based remote helper can safely modify a global namespace via the fast-import stream. So yeah, it would have to use a unique namespace based on the remote's identity, as you suggest. So refs/notes/hg-<gitifyhg-remote-id> -- the remote-id could be a SHA-1 of the remote URL (as I suggested for the name of the .git/hg/ dir). Or something else, that is for now not the main issue, I guess.

So this leaves the issue of how to expose the notes nicely to the end user. Having to type git log --refs=hg seems acceptable, but git log --refs=hg-<gitifyhg-remote-id> much less so. Right now I see two ways (not necessarily excluding each other) to do that:

  1. Have a git hg log command that knows how to pull in all these refs/notes/hg-* namespaces.
  2. Merge the various refs/notes/hg-* namespaces into refs/notes/hg -- just as you suggest.

Option 2 sure sounds nicer overall, and it would be beneficial to all git commands supporting the --notes=REF option. So I guess my lengthy diatribe means "yes I agree" ;-).

But pulling it off right seems delicate. Your idea of a hook sounds interesting, but I wonder who would install the hook? Ideally, I would wish that as much as possible works out of the box, and asking a user to install a hook is quite far from that. On the other hand, automatically installing a hook seems quite problematic to me... and leaves the question as to when it should happen, too :-/. But it might be the only chance, since it seem impossible to do this right from within the remote-helper... or at least it seems so to me: We can't know exactly when our fast-import streams are being processed, so even if we were willing to take a global lock during the duration of the import, we couldn't be completely sure when it would be safe to release that lock -- or could we?

Perhaps it would be worthwhile to raise this point on the git mailing list, as other people might be interested in this problem, too, and on the other hand, experienced git developers might have some additional insights, too?

jedbrown commented 11 years ago

As it turns out, there is no local post-fetch hook. I asked on the git list (http://thread.gmane.org/gmane.comp.version-control.git/214802) so hopefully someone will be able to suggest a good way to achieve this. I don't feel that refs/notes/hg-<remote> has any semantic meaning, it's just an artifact of avoiding races when applying the notes. There is a 1-1 correspondence between hg commits and git commits so there should only be one notes ref that the user needs to interact with (refs/notes/hg).

As for committing the merge from within the remote helper, it may be possible using checkpoint and progress to get feedback so that we can safely take a lock and apply the merge. This feels quite dirty, however, so I'd rather not have to do this.

fingolfin commented 11 years ago

Your pull request looks nicer, but I'll keep this one open for now, due to the discussion content that's been recorded here already.

Looking at the git-remote-helper docs, though, I think that checkpoint followed by progress would be a viable solution, iff we also switch to implementing the bidi-import remote helper capability. I sent an according reply to your mail on the git mailing list; let's continue discussing there?

fingolfin commented 11 years ago

Closing this. The discussion will still be documented here (and we can even discuss on a closed PR ;).