GSOC remote-svn

Hi!

Refering to Jonathan's concerns in Saturday night's IRC log:

helper project

like that refspec issue which should be straightforward to an experienced eye

Let me explain the refspec issue:

In the the exisiting code in contrib/svn-fe commits are always imported to refs/heads/master, that was hardcoded. So I thought that couldn't be it. I made the name of the branch to import variable, depending on the name of the remote. But my remote-helper didn't advertise the refspec capability, so transport- helper assumed it imports to refs/heads/master, which is the default import refspec. The subsequent update of references in store_updated_refs lead to wrong values after the fetch, which I considered a bug and tried to fix.

In fact I didn't realize that the actual updating of references is not done by the remote-helper. I thought the remote-helper would have to evaluate the fetch refspec and tell fast-import the correct target branch. Furthermore I confused 'private namespace' with refs/remotes/<remote's name>/, which I considered somehow private too.

After several mailing iterations, showing me that I was wrong, I found what the right point is, namely that the remote helper writes references to a really private dir in refs/<remote name>/, it doesn't touch anything else, and by advertising the 'refspec' capability, git-fetch knows where the private refs are and updates non-private references according to the fetch refspec in some post-processing in store_updated_refs. (Ok, you will say "of course!", but I didn't know that I was wrong and it's hidden in some 1000 lines of code).

For me that was not very easy to figure out, and it took a lot of time, but I think now remote-svn does it right.

can be tested more easily with tests in t/

haven't pinned down details about the svn:: conversion yet. That's why Dmitry's was called git-remote-svn-alpha)

Ok. Why is that important? I think if it's not called remote-svn git doesn't find it as a helper for the 'svn' protocol. Actually in my local git tree, I have a symlink in the toplevel (to simpify PATH).

time for it, which has been a problem:

they seem to come out of the blue.  That's fine, but

with a commit message describing the context and what the patch is trying to accomplish. That makes review way, way easier when it is happening.

Probably I should stop sending proposals or incomplete stuff to the list/you. The current state may probably be viewed easier in my github repo.

I think for creating patches that are acceptable I will need to squash and split a lot of my development commits after the code is somehow finished and no longer experimental.

about remote-svn.c that need fixing, and then patches for other things are appearing on top of that.

About the current state:

Tester: I wrote a small simulation script in python that mimics svnrdumps behaviour by replaying an existing svn dump file from a start rev up to an end rev to test incremental imports. I use it together with a little testrepo shell script. Will need to bring that into t/ later, after figuring out how the test framework works. As it's not finished it's not published.

Incremental import: By reading the latest svn revision number from a note attached to the private master ref, it starts future imports from the next svn revision. That basically works well. It doesn't reuse mark files. What's the point of reusing them? Dmitry's svn- alpha did that. All I need to know is the revision to start from and the branch i want to add commits to, right? It now simply reads that from the note.

This got stuck on another problem: Incremental update of the note tree doesn't work. fast-import refuses to update the notes tree: '<newsha1> doesn't contain <oldsha1>'. I don't yet know what's the reason for this. I'm digging into the internals of notes to find out why.. (no problem with the file tree).

This state hasn't hit the list of course, as it's in no way useful nor complete.

I often get caught in the traingle of those three processes (git transport- helper, fast-import, remote-svn) needing to understand a lot about the existing two to understand why things don't work and why they need to work like they do.

-- Florian

Hi,

Florian Achleitner wrote:

Right, that's fine. And you did a fine job of navigating the existing documentation (which could be improved, hint hint).

What I am more concerned about is that you had code you sent a while ago for review, that this would have been obvious to David, Ram, Dmitry, or me if we had seen it, and yet none of us gave you that help. We are failing at _our_ job of giving you prompt advice and instead you have had to work on your own.

Isn't it likely that there are multiple other bugs like that which still haven't been fixed?

That's why I think we need to get into a habit of giving and getting feedback quickly and incrementally improving work. Soon, before the summer ends.

[...]

It finds it as a helper for the 'svn-alpha' protocol instead.

The point is that when I perform the following steps:

	git clone svn://path/to/remote/repo

	... wait a day, update git

	cd repo
	git pull

nobody would expect the result to be a non-fast-forward update caused by the details of svn-to-git conversion changing. Using a name like testsvn or svn-alpha would help in managing expectations --- the remote helper is meant for experimentation for now and not meant to be something people can rely on for collaboration.

[...]

No, incomplete stuff is nice. Just please do explain the _purpose_ of the code you are sending out. The best possible outcome is if someone realizes that something that would have taken hours doesn't need to be done at all.

[...]

Sounds neat --- how can one try it out?

The marks are used to handle copyfrom operations referring to older revisions. Are you sure you want to abandon them? Can you explain a little more about your plan?

[...]

A good habit to get into is to make sure your partial progress toward a goal is in a usable state periodically, even if it is not complete. That makes it a lot easier to test and to get other people to look it over.

A rule of thumb is that unless you are adding a new and complicated feature, it should be possible to fit each change in a patch of around 250 lines including context (not including documentation and tests).

Probably you are finding documentation bugs right and left (yes, information not being easy to find is a bug) that don't get fixed because no one has reported them. Questions are welcome and very useful.

Hope that helps, Jonathan

On Sunday 22 July 2012 16:43:33 Jonathan Nieder wrote:

Ok, that makes sense. Renaming is easily done!

I'll send a patch ...

Ok, that makes sense. I didn't need the marks for incremental import. But to evaluate the copyfrom props we need some revision->sha1 mapping. I just added the options to save and import marks to fast-import's command line. If the file is missing, it will need to be generated from the notes, or the whole history will be reimported.

But when I fetch from a git repo that imported from svn, the notes are not fetched automatically. In this case I currently loose marks and notes. What can I do?

Florian

Florian Achleitner wrote:

In the long term, git will need to be tweaked to automatically fetch notes along with branches by default. There are other reasons not related to remote helpers to want this, too.

In the short term, we can document which ref the notes are expected to be fetched to. Maybe someone interested would provide commands like "git remote-testsvn --clone <repo>" and "git remote-testsvn --add-remote <nickname> <repo>" as a stopgap to set up the appropriate fetch refspec automatically so the user doesn't have to worry about it.

Thanks, Jonathan

To ease testing without depending on a reachable svn server, this
compact python script mimics parts of svnrdumps behaviour.
It requires the remote url to start with sim://.
Start and end revisions are evaluated.
If the requested revision doesn't exist, as it is the case with
incremental imports, if no new commit was added, it returns 1
(like svnrdump).
To allow using the same dump file for simulating multiple
incremental imports the highest revision can be limited by setting
the environment variable SVNRMAX to that value. This simulates the
situation where higher revs don't exist yet.
---
 contrib/svn-fe/svnrdump_sim.py |   53 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 contrib/svn-fe/svnrdump_sim.py

Florian Achleitner wrote:

Thanks.  Mind if I forge your sign-off?

On Monday 23 July 2012 07:59:21 Jonathan Nieder wrote:

Ups. No problem, anyways I've added it locally, so here's the new version ..

To ease testing without depending on a reachable svn server, this compact python script mimics parts of svnrdumps behaviour. It requires the remote url to start with sim://. Start and end revisions are evaluated. If the requested revision doesn't exist, as it is the case with incremental imports, if no new commit was added, it returns 1 (like svnrdump). To allow using the same dump file for simulating multiple incremental imports the highest revision can be limited by setting the environment variable SVNRMAX to that value. This simulates the situation where higher revs don't exist yet.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---

I had to fix the missing sign-off anyways..

 contrib/svn-fe/svnrdump_sim.py |   53 
++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 contrib/svn-fe/svnrdump_sim.py

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

You also have whitespace damages (i.e. line wrapping introduced by your mailer). Using git-send-email avoids this kind of problem (there are also some advices for some mailers in Documentation/SubmittingPatches).

On Monday 23 July 2012 18:24:40 Matthieu Moy wrote:

Damn. That's usually no problem with kmail either, if the config is right. I've already used git-send-email several times. But for replying to threads and adding several Cc: addresses it's a little cumbersome. How do you do that in a nice way?

-- Florian

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

For the threading itself, I usually find the message-id, and use "git send-email --in-reply-to='<cut-and-pasted-id>'". The painful part is when you want to reproduce a Cc: list, but I have no magic trick for that ;-).

On Mon, Jul 23, 2012 at 09:46:49PM +0200, Matthieu Moy wrote:

I save a copy of the message I am replying to (usually my cover letter, which I generated by just replying in my MUA) into a well-known location (I use a mutt hot-key to do this), and then run it through this monstrosity:

  get_reply_headers() {
    perl -ne '
      if (defined $opt && /^\s+(.*)/) {
        $val .= " $1";
        next;
      }
      if (defined $opt) {
        print "--$opt=", quotemeta($val), " ";
        $opt = $val = undef;
      }
      if (/^(cc|to):\s*(.*)/i) {
        $opt = lc($1);
        $val = $2;
      }
      elsif (/^message-id:\s*(.*)/i) {
        $opt = "in-reply-to";
        $val = $1;
      }
    '
  }

which can be used on the command line of format-patch or send-email, like:

  eval "git format-patch $(get_reply_headers <your-patch-file)"

I put the result in an mbox, then review and send it out in mutt (using the resend-message command), but you could invoke send-email directly, or format-patch into a file for review and send it with send-email.

-Peff

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

It is a bit unclear where "start" and "end" comes from, and if "evaluated" is the most important aspect of the handling of these two values. Do you mean the tool takes start and end revisions as arguments? If so, describe "how". E.g. as two arguments (-rSTART -rEND)? As an argument that shows a range (-rSTART-END? -rSTART,END)?

Do not answer with "It is in the code" (I cheated and peeked to find out it is -rSTART:END, but the reader should not have to peek).

This sentence does not parse for me. What is it trying to say? Requested revision does not exist _where_? It is unclear how "incremental import" and "revision doesn't exist" are related. "no new commit was added" to _what_ by _whom_? I presume that nobody is adding new commit _to_ an existing dump file, and the only thing this script does is to read and selectively write part of a dump file, so that would not add any new commit either.

Puzzled.

To ease testing without depending on a reachable svn server, this compact python script mimics parts of svnrdumps behaviour. It requires the remote url to start with sim://. Eventual slashes at the end of the url are stripped. The url specifies the path of the svn dump file (as created by svnrdump). Selectable parts of it, or the whole file, are written to stdout. The part is selectable by giving start and end revision on the command line.

Start and end revisions can be specified on the command line (-rSTART:END, like for svnrdump). Only revisions between START and excluding END are replayed from the dumpfile specified by the url. END can also be HEAD.

If the start revision specified on the command line doesn't exist in the dump file, it returns 1. This emulates the behaviour of svnrdump when START>HEAD, i.e. the requested start revision doesn't exist on the server.

To allow using the same dump file for simulating multiple incremental imports the highest visible revision can be limited by setting the environment variable SVNRMAX to that value. This effectively limits HEAD to simulate the situation where higher revs don't exist yet.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/svnrdump_sim.py |   53 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 contrib/svn-fe/svnrdump_sim.py

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

s/ventual/xcess/ perhaps?

Much more understandable than before.

It is unclear how this is different from giving the ceiling by specifying it as the "END" in -rSTART:END command line. Is this feature really needed?

Hi,

Junio C Hamano wrote:

[...]

I think the idea is that you put this script (or a symlink to it) on your $PATH with higher precedence than svnrdump and run a command that expected to be able to use svnrdump. Then instead of going to the network, the command you run magically uses your test data instead.

If the command you are testing wanted to run "svnrdump" without the upper endpoint set, we need to handle that request, either by emitting all the revs we have, or by stopping somewhere. The revlimit feature provides the "stopping somewhere" behavior which is not strictly needed but is presumably very useful when testing incremental fetch.

Florian, do you mind if I make the revlimit feature a separate patch when applying this?

Anyway, it looks good and reasonable to me, so will apply.

Thanks. Jonathan

On Tuesday 24 July 2012 14:50:49 Jonathan Nieder wrote:

Exactly, the purpose is to transparently replace svnrdump. Callers of svnrdump usually will specify -rSTART:HEAD, because they want to fetch everything they don't yet have. This feature allows to limit HEAD and to simulate incremental fetches using the same dump file. For me it proved very useful.

No problem.

-- Florian

On Mon, Jul 23, 2012 at 2:44 PM, Florian Achleitner <florian.achleitner.2.6.31@gmail.com> wrote:

Nit: add a \n after "sys.exit(ret)", perhaps?

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

The man page for git-remote-helpers says:

refspec <refspec>
    [...] It is recommended that all importers providing the import
    capability use this.

I'm not sure I fully understand the rationale, but one difference between refs/<remote name>/* and refs/remotes/<remote name> is that refs/remotes/ is automatically updated by Git on push, while the private namespace isn't (it only receives updates when importing).

I played a bit with that it git-remote-mediawiki. There's a configuration variable mediawiki.dumbPush that controls what "push" does. It can either export the local history without touching local metadata (and then, it is expected that the user does a "git pull --rebase" right after to re-import the history), or update the local metadata (private ref, and notes that keep the <local commit> <-> <remote revision number> map), so that the next "git pull" says "Already up to date.".

Matthieu Moy wrote:

It's mostly to allow "git fetch" to avoid non fast-forward updates unless -f was used or the refspec starts with +.

I always liked the idea of tweaking the fast-import stream format to allow the import to happen on no branch at all since it would avoid all these questions. Maybe another day.

Thanks, Jonathan

On Sunday 22 July 2012 23:03:27 Florian Achleitner wrote:

btw, this one was rather simple, just a syntax error in the fast-import stream..