v5, 15 partsfast-export and remote-testgit improvements

Hi,

Basically resending with a few fixes...

I found more issues in fast-export. remote-testgit, and eventually I decided there's no reason to use this python script, so I wrote a much simpler version that does the same, and more. I'm not going to list all the reasons because apparently opinions are not welcome in the list any more. For the actual differences you can check the patch itself.

The old remote-testgit is now remote-testpy (as it's testing the python framework, not really remote helpers). The tests are simplified, and exercise more features of transport-helper, and unsuprisingly, find more bugs.

Some of these bugs are fixed in this patch series as well, for which I already sent 3 versions, and they come at the end. I was surprised they did fix them, but hey... good is good.

I know how to fix the rest of the issues, but I'm not going to bother sending a patch because obvious... er, simple? fixes are not accepted, so there's no chance of something less... evident? getting through.

Cheers.

Changes since v4:

 * Add check for bash in test
 * Avoid bash associative arrays for older versions
 * Apply trivial comments
 * White-space cleanups

Felipe Contreras (15):
  fast-export: avoid importing blob marks
  remote-testgit: fix direction of marks
  remote-helpers: fix failure message
  Rename git-remote-testgit to git-remote-testpy
  Add new simplified git-remote-testgit
  remote-testgit: get rid of non-local functionality
  remote-testgit: remove irrelevant test
  remote-testgit: cleanup tests
  remote-testgit: exercise more features
  remote-testgit: report success after an import
  remote-testgit: make clear the 'done' feature
  fast-export: trivial cleanup
  fast-export: fix comparison in tests
  fast-export: make sure updated refs get updated
  fast-export: don't handle uninteresting refs

 .gitignore                           |   2 +-
 Documentation/git-remote-testgit.txt |   2 +-
 Makefile                             |   2 +-
 builtin/fast-export.c                |  20 ++-
 git-remote-testgit                   |  82 +++++++++++
 git-remote-testgit.py                | 272 -----------------------------------
 git-remote-testpy.py                 | 272 +++++++++++++++++++++++++++++++++++
 git_remote_helpers/git/importer.py   |   2 +-
 t/t5800-remote-helpers.sh            | 148 -------------------
 t/t5800-remote-testpy.sh             | 148 +++++++++++++++++++
 t/t5801-remote-helpers.sh            | 158 ++++++++++++++++++++
 t/t9350-fast-export.sh               |  41 +++++-
 12 files changed, 717 insertions(+), 432 deletions(-)
 create mode 100755 git-remote-testgit
 delete mode 100644 git-remote-testgit.py
 create mode 100644 git-remote-testpy.py
 delete mode 100755 t/t5800-remote-helpers.sh
 create mode 100755 t/t5800-remote-testpy.sh
 create mode 100644 t/t5801-remote-helpers.sh

We want to be able to import, and then export, using the same marks, so that we don't push things that the other side already received.

Unfortunately, fast-export doesn't store blobs in the marks, but fast-import does. This creates a mismatch when fast export is reusing a mark that was previously stored by fast-import.

There is no point in one tool saving blobs, and the other not, but for now let's just check in fast-export that the objects are indeed commits.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c  |  4 ++++
 t/t9350-fast-export.sh | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

On 11.11.12 14:59, Felipe Contreras wrote:

Unless I messed up the patch:

Minor issue: still a typo "biridectionality" Major issue: "echo -n" is still not portable.

Could we simply use

touch marks-cur && touch marks-new

/Torsten

On Sun, Nov 11, 2012 at 05:36:53PM +0100, Torsten Bögershausen wrote:

Yes, "echo -n" is definitely not portable. Our preferred way of creating an empty file is just ">file".

-Peff

Jeff King <peff@peff.net> writes:

Yes.

And it is misleading to use "touch" in this case; unless you are updating the timestamp of an existing file while preserving the contents of it, please don't use the command.

Thanks.

On Sun, Nov 11, 2012 at 5:36 PM, Torsten Bögershausen <tboegi@web.de> wrote:

Yeah.

Unless somebody else already modified those marks. Better be safe than sorry.

Basically this is what we want:

  == pull ==

	testgit			transport-helper

	* export ->		import

	# testgit.marks		git.marks

  == push ==

	testgit			transport-helper

	* import		<- export

	# testgit.marks		git.marks

Each side should be agnostic of the other side. Because testgit.marks (our helper marks) could be anything, not necesarily a format parsable by fast-export or fast-import. In this test hey happen to be compatible, because we use those tools, but in the real world it would be something compelely different. For example, they might be mapping marks to mercurial revisions (certainly not parsable by fast-import/export).

This is what we have:

  == pull ==

	testgit			transport-helper

	* export ->		import

	# testgit.marks		git.marks

  == push ==

	testgit			transport-helper

	* import		<- export

	# git.marks		testgit.marks

The only reason this is working is that git.marks and testgit.marks are roughly the same.

This new behavior used to not be possible before due to a bug in fast-export, but with the bug fixed, it works fine.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-remote-testgit.py              | 2 +-
 git_remote_helpers/git/importer.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

On 11.11.2012, at 14:59, Felipe Contreras wrote:

Typo: necesarily => necessarily

Cheers, Max

This is remote-testgit, not remote-hg.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5800-remote-helpers.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

This script is not really exercising the remote-helper functionality, but more the python framework for remote helpers that live in git_remote_helpers.

It's also not a good example of how to write remote-helpers, unless you are planning to use python, and even then you might not want to use this framework.

So let's use a more appropriate name: git-remote-testpy.

A patch that replaces git-remote-testgit with a simpler version is on the way.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 .gitignore                |   2 +-
 Makefile                  |   2 +-
 git-remote-testgit.py     | 272 ----------------------------------------------
 git-remote-testpy.py      | 272 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t5800-remote-helpers.sh | 148 -------------------------
 t/t5800-remote-testpy.sh  | 148 +++++++++++++++++++++++++
 6 files changed, 422 insertions(+), 422 deletions(-)
 delete mode 100644 git-remote-testgit.py
 create mode 100644 git-remote-testpy.py
 delete mode 100755 t/t5800-remote-helpers.sh
 create mode 100755 t/t5800-remote-testpy.sh

It's way simpler. It exerceises the same features of remote helpers. It's easy to read and understand. It doesn't depend on python.

It does _not_ exercise the python remote helper framework; there's another tool and another test for that.

For now let's just copy the old remote-helpers test script, although some of those tests don't make sense for this testgit (they still pass).

In addition, this script would be able to test other features not currently being tested.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-remote-testgit.txt |   2 +-
 git-remote-testgit                   |  62 ++++++++++++++++
 t/t5801-remote-helpers.sh            | 139 +++++++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+), 1 deletion(-)
 create mode 100755 git-remote-testgit
 create mode 100755 t/t5801-remote-helpers.sh

On 11.11.2012, at 14:59, Felipe Contreras wrote:

Typo: exerceises => exercises

Cheers, Max

Felipe Contreras <felipe.contreras@gmail.com> writes:

You mention why you _think_ it is better, and what it is _not_, but with your excitement, end up failing to mention what it is. I'll try to reword the commit with this sentence:

	This script is to test the remote-helper interface.

somewhere. Please check what I'll push out on 'pu' after I'm done for the day (probably in 8 hours).

I hinted at this in an earlier message, but creating this file as a tracked file at this point in the history is a bit irritating for bisectability. After you build and test an earlier commit, this path is a generated and ignored, and then checking out this commit or a later one will fail without "make clean". It is only a minor irritation, but still noticeable.

Thanks.

On Wed, Nov 21, 2012 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

That's right.

This only makes sense for the python remote helpers framework.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-remote-testgit        |  3 ---
 t/t5801-remote-helpers.sh | 50 ++++++++++++++++++++---------------------------
 2 files changed, 21 insertions(+), 32 deletions(-)

Felipe Contreras <felipe.contreras@gmail.com> writes:

A better explanation is sorely needed for this. If the test were feeding python snippet to be sourced by python remote helper to be tested, the new remote-testgit.bash would not have any hope (nor need) to grok it, and "this only makes sense for python" makes perfect sense and clear enough, but that is not the case.

If the justification were like this:

    remote-testgit: remove non-local tests
    
    The simplified remote-testgit does not talk with any remote
    repository and incapable of running non-local tests.  Remove
    them.

I would understand it, and I wouldn't say it is a regression in the test not to test "non-local", as that is not essential aspect of these tests (we are only interested in testing the object/ref transfer over remote-helper interface and do not care what the "other side" really is).

But I am not quite sure what you really mean by "non-local" functionality in the first place. The original test weren't opening network port to emulate multi-host remote transfer, were it?

Thanks.

On Wed, Nov 21, 2012 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

No, that's not it at all.

By local, I mean 'file:///home/user/foo', by remote, I mean 'http://user.org/foo'. How each of these URLs is handled is entirely up to the remote helper.

bzr for example doesn't need any change at all, the same API works on both cases. hg OTOH has different APIs, so the code needs a local clone to do most operations. The python remote helper framework has APIs to make it easier to implement the local clone functionality (for the remote helpers that need it).

This has absolutely nothing to do with with remote helpers, this is 100% a python remote helper feature. So we don't need those tests here, they belong in git-remote-testpy.

Cheers.

Only makes sense for remote-testpy.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 13 -------------
 1 file changed, 13 deletions(-)

We don't need a bare 'server' and an intermediary 'public'. The repos can talk to each other directly; that's what we want to exercise.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh | 63 ++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 34 deletions(-)

Felipe Contreras <felipe.contreras@gmail.com> writes:

The previous patch to remove the test (the one that covered a case where a bug was fixed in an older git-remote-testpy and tried to catch the bug when it resurfaced) made sense even with its ultra-short justification "irrelevant".

But I am not sure if this one is so cut-and-dried. The repos can talk to each other directly, but at the same time the tests were exercising interactions between bare and non-bare repositories, weren't they? Talking to each other may be one of the things we want to exercise, but that does not necessarily be the only thing.

If it were explained like this (note that I am *guessing* what you meant to achieve by this patch, which may be wrong, in which case the log message needs further clarification):

	Going through an intermediary 'public' may have exercised
	interactions among combinations of bare and non-bare
	repositories a bit more, but that is not an issue specific
	to the remote-helper transfer that we want to be testing in
	this script.  Simplify the tests to let two repositories
	talk directly with each other.

I think the changes themselves make sense.

On Wed, Nov 21, 2012 at 7:28 PM, Junio C Hamano <gitster@pobox.com> wrote:

Right. I don't think bare vs. non-bare has anything to do with it; the intermediary repository was there to have 3 types of repos interacting with each other local testpy, remote testpy, local git. But this doesn't exercise anything from transport helper.

Cheers.

Unfortunately they do not work.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-remote-testgit        | 18 +++++++++++++----
 t/t5801-remote-helpers.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)
 mode change 100755 => 100644 t/t5801-remote-helpers.sh

Felipe Contreras <felipe.contreras@gmail.com> writes:

As far as I can tell, "more features" simply mean one, no?  Perhaps

    remote-testgit: exercise non-default refspec feature

or something.

Oops.

Again, please check the fixup! interspersed in the result I'll queue on 'pu'.

Thanks.

On Wed, Nov 21, 2012 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

It's the other way around, a good refspec works, anything else doesn't. s/non-default/default/ but there's other stuff:

1) *:* refspec
2) no refspec
3) no marks

Doesn't make a difference for the tests, but it does for the ones seeking reference.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-remote-testgit | 11 +++++++++++
 1 file changed, 11 insertions(+)

People seeking for reference would find it useful.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-remote-testgit | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On 11.11.2012, at 14:59, Felipe Contreras wrote:

Hm, I don't understand this commit message. Probably means I am just too dumb, but since I am one of those people who would likely be seeking for reference, I would really appreciate if it could clarified. Like, for example, I don't see how the patch below makes anything "clear", it just seems to change the "import" command of git-remote-testgit to make use of the 'done' feature?

Perhaps the idea of the patch is to make use of the "done" feature so that remote-testgit acts as "reference implementation"? If that is the intention, then perhaps this could be used as commit message:

  remote-testgit: make use of the 'done' feature

  This might be helpful for people who would like to see how to properly
  implement the "done" feature.

But again, I am not sure if I understood the purpose of this patch correctly. So please forgive me if this was totally off-base :-(.

Cheers, Max

On Sun, Nov 11, 2012 at 9:49 PM, Max Horn <max@quendi.de> wrote:

No, the done feature was there already, but not so visible: git fast-export --use-done-feature <-there. Which is the problem, it's too easy to miss, therefore the need to make it clear.

It's already there.

Everybody should implement the 'done' feature. Otherwise random error messages quite easily appear.

On 11.11.2012, at 22:22, Felipe Contreras wrote:

Aha, now I understand what this patch is about. So I would suggest this alternate commit message:

  remote-testgit: make it explicit clear that we use the 'done' feature

  Previously we relied on passing '--use-done-feature ' to git fast-export, which is
  easy to miss when looking at this script. Since remote-testgit is also a reference
  implementation, we now explicitly output 'feature done' / 'done' to make it
  crystal clear that we implement this feature.

Or perhaps a little bit less verbose. With a commit message like the above, I think I would have grokked the patch right away. With the original message, that was not the case (else I wouldn't have wrote my initial email). And even though I now understand (or at least believe to understand) the patch, I don't think the original message is that helpful... indeed, "make clear the 'done' feature" is ambiguous. You meant it as "make clear the 'done' feature is implemented / used", while I understood it as "make clear what the 'done' feature is about". Looking at the patch can help to resolve that, but (a) my wrong interpretation threw me off-track and (b) I thought that the point of commit messages was to give an overview of a patch without having to look at it... So at the very least, the message should explain what exactly is "made clear".

Anyway, a small change to the commit message hopefully will not be a problem. :-)

Cheers, Max

Max Horn wrote:

I'm not immediately sure I agree this is even a problem. Is the point that other fast-import frontends do not have a --use-done-feature switch, so a typical remote helper has to do that work itself, and the sample "testgit" remote helper would be a more helpful example by doing that work itself?

The idea behind --use-done-feature is that if fast-export exits early for some reason and its output is going to a pipe then at least the stream will be malformed, making it easier to catch errors. So there is something to be weighed here: is it more important to illustrate how to make your fast-export tool's output prefix-free, or is it more important to illustrate how to work around a fast-export tool that doesn't support that feature? The answer is not immediately obvious to me. A good description could provide context to make it obvious.

Hoping that clarifies, Jonathan

On Mon, Nov 12, 2012 at 4:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

You mean other fast-exports.

And what other fast-exports? Most remote helpers don't use an external fast-export tool, and the only I know that used one is the one I wrote (and is now deprecated) that used bzr fast-export, and no, that one didn't support the done feature.

Most remote helpers would probably be doing the equivalent of fast-export themselves.

Yes.

What fast-export tool?

This is a remote helper.

Ditto.

If you want to launch a campaign of adding the 'done' feature to whatever fast-export tools are out there (that I'm not aware of), go ahead, but this is about remote helpers, most (all?) of which would not use a fast-export tool to achieve the export, but do it themselves.

Cheers.

Max Horn <max@quendi.de> writes:

I'd state it like this, but I may have guessed what Felipe intended incorrectly.

    remote-testgit: advertise "done" feature and write "done" ourselves
    
    Instead of letting "fast-export" advertise the feature and ending
    its stream with "done", do it ourselves.  This way, it would make it
    more clear to people who want to write their own remote-helper to
    produce fast-export streams without using "fast-export
    --use-done-feature" that they are supposed to end their stream with
    "done".

On Wed, Nov 21, 2012 at 10:11 AM, Junio C Hamano <gitster@pobox.com> wrote:

With that commit message:

Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

Setting 'commit' to 'commit' is a no-op. It might have been there to avoid a compiler warning, but if so, it was the compiler to blame, and it's certainly not there any more.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

First the expected, then the actual, otherwise the diff would be the opposite of what we want.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9350-fast-export.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

When an object has already been exported (and thus is in the marks) it's flagged as SHOWN, so it will not be exported again, even if in a later time it's exported through a different ref.

We don't need the object to be exported again, but we want the ref updated, which doesn't happen.

Since we can't know if a ref was exported or not, let's just assume that if the commit was marked (flags & SHOWN), the user still wants the ref updated.

IOW: If it's specified in the command line, it will get updated,
regardless of wihether or not the object was marked.

So:

 % git branch test master
 % git fast-export $mark_flags master
 % git fast-export $mark_flags test

Would export 'test' properly.

Additionally, this fixes issues with remote helpers; now they can push refs wich objects have already been exported, and a few other issues as well. So update the tests accordingly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c     | 10 +++++++---
 t/t5801-remote-helpers.sh | 24 ++++++++++--------------
 t/t9350-fast-export.sh    | 15 +++++++++++++++
 3 files changed, 32 insertions(+), 17 deletions(-)

On 11.11.2012, at 14:59, Felipe Contreras wrote:

Typo: wihether => whether

Typo: wich => which

Cheers, Max

Max Horn <max@quendi.de> writes:

I'd rather use "whose" there.

They have been marked as UNINTERESTING for a reason, lets respect that.

Currently the first ref is handled properly, but not the rest, so:

 % git fast-export master ^master

Would currently throw a reset for master (2nd ref), which is not what we want.

 % git fast-export master ^foo ^bar ^roo
 % git fast-export master salsa..tacos

Even if all these refs point to the same object; foo, bar, roo, salsa, and tacos would all get a reset, and to a non-existing object (invalid mark :0).

And even more, it would only happen if the ref is pointing to exactly the same commit, but not otherwise:

 % git fast-export ^next next
 reset refs/heads/next
 from :0

 % git fast-export ^next next^{commit}
 # nothing
 % git fast-export ^next next~0
 # nothing
 % git fast-export ^next next~1
 # nothing
 % git fast-export ^next next~2
 # nothing

The reason this happens is that before traversing the commits, fast-export checks if any of the refs point to the same object, and any duplicated ref gets added to a list in order to issue 'reset' commands after the traversing. Unfortunately, it's not even checking if the commit is flagged as UNINTERESTING. The fix of course, is to do precisely that.

The current behavior is most certainly not what we want. After this patch, nothing gets exported, because nothing was selected (everything is UNINTERESTING).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c  | 4 +++-
 t/t9350-fast-export.sh | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

On Sun, Nov 11, 2012 at 2:59 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

And here's yet another reason why this is obviously correct that I just found:

% git fast-export --use-done-feature --{import,export}-marks=.git/hg/origin/marks-git ^refs/hg/origin/branches/default ^refs/hg/origin/bookmarks/test6 refs/heads/test6 ^refs/hg/origin/bookmarks/master ^refs/hg/origin/bookmarks/test feature done reset refs/hg/origin/bookmarks/test from :4

reset refs/heads/test6 from :14

done

What is that refs/hg/origin/bookmarks/test doing there?

transport-helper does use a fast-export command like that to specify precisely what refs should be *IGNORED*, and yet fast-export will throw a reset for a ref that has been marked as UNINTERESTING. So, the receiving end in the helper will see a reset for a ref that it explicitly said was marked as outside it's refspec realm:

refspec refs/heads/*:refs/hg/origin/bookmarks/*

What is remote-hg supposed to do with 'refs/hg/origin/bookmarks/test'? There's nothing that can be done, it's a bug in fast-export that such a thing was exported in the first place. And the reason it happens is that another ref happens to be pointing to the same object, (in this case refs/hg/origin/branches/default)

So yeah, the patch is good.

Of course, transport-helper shouldn't even be specifying the negative (^) refs, but that's another story.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

Hrm, I am not sure I understand what you mean by this.

How should it be telling the fast-export up to what commit the receiving end should already have the history for (hence they do not need to be sent)? Or are you advocating to re-send the entire history down to the root commit every time?

On Tue, Nov 20, 2012 at 11:43 PM, Junio C Hamano <gitster@pobox.com> wrote:

No, it would not re-send the whole history, that's what marks are for.

And right now it doesn't exactly which was the last commit. Let's suppose the remote helper has a refspec like this:

refs/heads/*:refs/hg/origin/heads/*

1) What happens the first time you push?

5203a268546295ebd895fd87522217ef53bd3313 refs/heads/master 5203a268546295ebd895fd87522217ef53bd3313 refs/remotes/tmp/master

Notice how the remote ref is updated correctly, but it's not the remote helper refspec, so the next time you push, you will from root.

It's only when you fetch that you get the refspec'ed refs:

5203a268546295ebd895fd87522217ef53bd3313 refs/heads/master 5203a268546295ebd895fd87522217ef53bd3313 refs/hg/tmp/heads/master 5203a268546295ebd895fd87522217ef53bd3313 refs/remotes/tmp/master

So, there's already a mismatch.

2) What happens when you have no marks?

You get something like: reset refs/heads/heads from :0

Which is totally useless. Somebody proposed a patch that would replace the :0 with a git sha-1, but that is equally useless for a remote helper: we need a hg ref id, or a bzr id, or whatever, and no, there's mapping between git sha-1's and hg ref ids, there's only git->mark mark->hg, without marks, there's no way to map the git id to the hg id.

3) What happens when you have a refspec like this?

*:*

Now nothing works, because we would be requesting ^refs/heads/master refs/heads/master.

And according to the documentation, this is the default when no refspec is used, which is not true.

4) What happens when there's no refspec at all.

Now it's even worst; nothing gets done at all:

if (!data->refspecs)
	continue;

I documented all this breakages in this patch:

http://article.gmane.org/gmane.comp.version-control.git/209365

not ok 10 - push new branch with old:new refspec # TODO known breakage ok 11 - cloning without refspec ok 12 - pulling without refspecs not ok 13 - pushing without refspecs # TODO known breakage not ok 14 - pulling with straight refspec # TODO known breakage not ok 15 - pushing with straight refspec # TODO known breakage not ok 16 - pulling without marks # TODO known breakage not ok 17 - pushing without marks # TODO known breakage

And if you apply this patch:

Junio C Hamano wrote:

I think Felipe has mentioned before that he considers it the remote helper's responsibility to keep track of what commits have already been imported, for example using a marks file.

Never mind that others have said that that's not the current interface (I don't yet see why it would be a good interface after a transition, but maybe it would be). Still, hopefully that clarifies the intended meaning.

Hope that helps, Jonathan

On Wed, Nov 21, 2012 at 5:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

It's not the remote helper, fast-export does that.

The current interface is broken.

not ok 16 - pulling without marks # TODO known breakage not ok 17 - pushing without marks # TODO known breakage

See? A remote helper without marks doesn't work.

Jonathan Nieder <jrnieder@gmail.com> writes:

Care to explain how the current interface is supposed to work, how fast-export and transport-helper should interact with remote helpers that adhere to the current interface, and how well/correctly the current implementation of these pieces work?

What I am trying to get at is to see where the problem lies. Felipe sees bugs in the aggregated whole. Is the root cause of the problems he sees some breakages in the current interface? Is the interface designed right but the problem is that the implementation of the transport-helper is buggy and driving fast-export incorrectly? Or is the implementation of the fast-export buggy and emitting wrong results, even though the transport-helper is driving fast-export correctly? Something else?

I see Felipe keeps repeating that there are bugs, and keeps posting patches to change fast-export, but I haven't seen a concrete "No, the reason why you see these problems is because you are not using the interface correctly; the currrent interface is fine. Here is how you can fix your program" from "others".

With such a one-sided discussion, I've been having a hard time convincing myself if Felipe's effort is making the interface better, or just breaking it even more for existing remote helpers, only to fit his world model better.

Help?

On Wed, Nov 21, 2012 at 6:08 AM, Junio C Hamano <gitster@pobox.com> wrote:

IIRC you mentioned something about this mailing list being focused on *technical* merit. I've explained as much as I could, but at the end of the talk, talk is cheap, the code speaks for itself. I added a new very very very simple testgit remote helper, so anybody can see what's going on, and figure out how the interface could be used wrong.

Anybody can modify the bash version of git-remote-testgit and say 'no, the interface is not broken, here is how you push and pull without marks'. How hard is it to hack 82 lines of bash code?

But lets assume my testgit is fatally broken, would you, Junio, accept these patches if I show the same broken behavior with the python git-remote-testgit?

I'm afraid I have to point out the hard truth; the reason why nobody is doing that is because a) the interface is truly broken b) if they try, they most likely would fail, and that would prove they were wrong in previous discussion, or c) not enough familiarity with the code. I don't want to point fingers, nor do I intend to offend anybody, but I cannot find any other explanation of why this patch series, which is obviously correct (to me), doesn't receive any feedback, even though in theory, it should be very very very easy to show what's wrong with the series.

The tests are there, and the remote helper is as simple as it gets. There's nothing else but fast-export and transport-helper to blame for the issues. It's that simple.

Cheers.

On Wed, Nov 21, 2012 at 6:08 AM, Junio C Hamano <gitster@pobox.com> wrote:

Let me give it a shot at explaining the case for remote helpers that use export/import.

== listing ==

All operations begin with the transport helper requesting a list of refs. Basically 'git show-ref'.

== fetching ==

In fetch mode the transport helper will initiate the process by requesting refs to the remote helper, like 'master', or 'devel', and so on. These refs were previously provided by the remote helper itself in the "listing" step.

It is the total responsibility of the remote helper to decide what to do: nothing, only update the ref pointers, retrieve the whole repository, retrieve only the listed refs, etc. It's also the responsibility of the remote helper to keep track of marks, last known commits the refs pointed to, update local transitory repositories, etc.

It's also the responsibility of the remote helper to throw the right 'feature' commands to fast-import for everything, including where to store the marks.

Note that there are two sets of marks; the marks of the remote helper, which could be anything: JSON, text files, binary, etc. and don't contain git SHA-1's, and the git marks, which do contain git SHA-1's and are exported/imported by fast-import, but *both* are totally under control of the remote helper.

At this point, git (transport helper), has absolutely no idea what's going on, the communication is completely between the remote helper and fast-import. After this process has finished, control goes back to the transport helper, which proceeds to check what fast-import did.

Then, the result is shown to the user as the typical fetch that updated certain refs.

== pushing ==

In this mode the roles are reversed, now git (transport helper) is in control, and everything that happens depends on what commands are passed to fast-export. Now the remote helper is a passive receiver of data, and has two options, receive it or die.

Which refs get updated and how, is the total responsibility of transport helper.

The only control the remote helper has, is before the export begins, in the configuration (capabilities command) that happens at the very beginning (before listing), and where it specifies features to support, which are then used to pass the relevant arguments to fast-export.

And these capabilities are very limited:
* import-marks
* export-marks
* refspec

After the push has finished, the remote helper then proceeds to report which refs were actually updated, and the user gets notified.

== details ==

As it should be obvious by now, there's not many ways in which a remote helper can screw things up (other than the parsing and generation of data for fast-import/export). The only tricky part is the refspec.

To function properly, a remote helper should specify a refspec such as 'refs/heads/*:refs/test/heads/*', this way, all the changes a remote helper does are isolated in a specific refspec namespace, and the update to normal git happens in a controlled way.

However, the refspec only makes sense in the *fetching* mode; the remote-helper is supposed to throw updates in the form of 'commit refs/test/heads/master', not 'commit refs/heads/master' (although in some case that might work, but I'm not sure which).

But when pushing the remote helper will receive the refs in the normal form 'refs/heads/master'. Also, the namespaced refs are only updated when fetching, not when pushing.

Marks are very straightforward; the same import and export marks should be specified for both importing and exporting.

Everything works mostly fine as long as the remote helper follows this. Things break in all sorts of ways when it doesn't.

But I want to emphasize again that there's not many ways in which a remote helper can screw things: marks, or refspec, that's it. *Specially* when pushing.

== no marks ==

Let's imagine a very simple repository with 3 commits, which gets pushed to a remote one:

4e891f6 :3 d9d17c3 :2 e1aef7b :1

I'm obviously simplifying the marks, but essentially that's what fast-export would do when pushing commits to a remote helper; it the parent of :2 is :1, and the parent of :3 is :2, but the remote side *never* sees any git SHA-1, because they are not interesting in any way, there's nothing useful that can be done with them.

The remote side would generate commits such as:

:3 103 :2 102 :1 100

Again, for simplification purposes (you can picture them as mercurial revs).

Now the push has finished. The marks are gone (no marks).

What happens when you fetch? You might think that we will get only the commits after :3, but that's not the case, the transport helper would use 'refs/test/heads/master' to find out the last commit, but that doesn't get updated when pushing, only when fetching, so we would start from the top.

4e891f6 :3 d9d17c3 :2 e1aef7b :1

:3 103 :2 102 :1 100

The same will happen if you push, because push also uses 'refs/test/heads/master'.

But *now* that we are doing a fetch, the 'refs/test/heads/master' pointer is updated to 4e891f6. But don't think that those marks are the same as the previous ones: they happen to be the same because they were generated the same way, but they are completely independent.

What happens when you push now? Now the 'refs/test/heads/master' is pointing to 4e891f6, and suppose we have two new commits:

88764ee 4607106 4e891f6 :3 <- I'm putting these for reference, but in reality they are gone d9d17c3 :2 e1aef7b :1

The transport helper would do an export of '^refs/test/heads/master refs/heads/master', or '^4e891f6 88764ee'. And here comes the interesting part:

What is the parent of 4607106? It's not :3, because that mark is gone, and in fact, even if we sent :3; things would break down because the other side has no idea what :3 means; it's gone, caput. What really happens is:

88764ee :2 4607106 :1

This is a new tree. That's exactly what you would expect if you do 'git fast-export ^v1.8.0^ master'; export all the commits as if v1.8.0 was the root.

But in the context of remote helpers, that's not what we want.

What can we do to fix this? Let's suppose that through some magic we get the parent of 4607106 to be 4e891f6; is that helpful? No. To the remote helper 4e891f6 is useless. What we need is 103, but without marks, we can't find that out.

Maybe if we stored it in the last run? We need to parse the git marks, and then match our marks with those, and we could get a mapping like '4e891f6 -> 103', but what if the parent is 102? So, we need a mapping for all the marks, and then we have to store such mapping anyway. And guess what? We are back to using marks again! Except that instead of using the standard git way, we are using a custom hacky way.

Are there other solutions? Maybe we can store the information in refs:

4e891f6 refs/test/ids/103 d9d17c3 refs/test/ids/102 e1aef7b refs/test/ids/100

But that also would require parsing the git marks, and going outside of the intended fast-export tool would kind of defeat the purpose of being fast an efficient, and still very hacky.

And lets not even go as to what would be needed for 'git fast-export' to actually generate 4e891f6 in the first place, as that would probably require changes that would break other things.

So no, you can't do it without marks.

And why are we even discussing about this? Why would anybody want to avoid marks? Not only there's no other ways to achieve the same, marks are cheap and efficient, as efficient as any other solution could be, and then some. And do we have any real remote helpers that try to do export/import without marks? No, heck, we don't even have fake ones.

It just doesn't work. Seriously.

And my patches actually make it work: if there are no marks, then _everything_ is pushed. I don't see the point of supporting the functionality of no marks, clearly nobody is using that because it just doesn't work. Nobody has shown a shred of evidence to the contrary. With my patches, at least we try to do something without failing too miserably.

Cheers.

On Tue, Nov 20, 2012 at 09:08:36PM -0800, Junio C Hamano wrote:

Felipe responded in more detail, but I will just add the consensus we came to earlier in the discussion: the series does make things better for users of fast-export that use marks, but does not make things any better for users of negative refs on the command line. However, I do not think that it makes things worse for them, either (neither by changing the behavior negatively, nor by making the code harder for a more complete fix later).

So while fixing everybody might be nice, there is no need to hold up progress for the marks case. Which, as he has noted, is probably the sanest way to implement a remote-helper[1].

-Peff

[1] There are other possible use cases for fast-export which might
    benefit from negative refs working more sanely, but since they are
    in the minority and are not being made worse, I think the partial
    fix is OK.

On Wed, Nov 21, 2012 at 8:48 PM, Jeff King <peff@peff.net> wrote:

Patch 14 changes the behavior depending on the marks, patch 15 doesn't.

This patch is mostly orthogonal to marks.

Before without marks: % git branch unintresting master % git fast-export master ^uninteresting reset refs/heads/uninteresting from :0

Before with marks: % git fast-export --import-marks=marks master ^uninteresting reset refs/heads/uninteresting from :100

See? In both cases git is doing something the user doesn't want, nor specified. After my patch nothing gets updated, because nothing was specified to be updated.

I'm not going to bother explaining why other people objected to this patch (again), which is indeed related to marks, they should do it for themselves. Let me reaffirm that no valid reason has put forward to object to this patch.

I would like to understand that that even means. What behavior is currently broken? And for who? And how is this patch related to that?

Which ones? I don't think this is a partial fix.

Nobody has put forward such a use-case.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

I do not know if this is the same as what Peff was referring to, but I found this message in the discussion thread during my absense.

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
Date: Fri, 2 Nov 2012 16:17:14 +0100 (CET)
Message-ID: <alpine.DEB.1.00.1211021612320.7256@s15462909.onlinehome-server.info>

(which is $gmane/208946) that says:

	Note that

		$ git branch foo master~1
		$ git fast-export foo master~1..master

	still does not update the "foo" ref, but a partial fix is better
	than no fix.

On Mon, Nov 26, 2012 at 6:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

First of all, do we agree that this patch does not change the situation for this command? If so, I don't see why that would be relevant while discussing this patch series.

Second, this is what I get:

% git log --decorate --oneline foo master~1..master 8c7a786 (tag: v1.8.0, master) Git 1.8.0

Notice that 'foo' is not there? It's not there because we explicitly stated that we didn't want it there.

And what do you expect that command to do with 'foo'? To throw a 'reset refs/heads/foo'? To what commit? There is no mark for that commit. 'reset :0'? That doesn't help anybody. No, that command is not broken, it works as expected.

Notice the situation would be different with 'git fast-export --import-marks=marks foo master~1..master', because if there's a mark for foo, *now* we can do something about it. This particular patch series doesn't, but the next one does.

Cheers.

Hi Junio,

On Sun, 25 Nov 2012, Junio C Hamano wrote:

If you changed your stance on the patch Sverre and I sent to fix this, we could get a non-partial fix for this. You wanted a fix for a bigger problem, though, which I am unwilling to fix because it is not my itch to scratch and I have to balance my time.

Ciao, Johannes

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

This is long time ago so I may be misremembering the details, but I thought the original patch was (ab)using object flags to mark "this was explicitly asked for, even though some other range operation may have marked it uninteresting". Because it predated the introduction of the rev_cmdline_info mechanism to record what was mentioned on the command line separately from what objects are uninteresting (i.e. object flags), it may have been one convenient way to record this information, but it still looked unnecessarily ugly hack to me, in that it allocated scarce object flag bits to represent a narrow special case (iirc, only a freestanding "A" on the command line but not "A" spelled in "B..A", or something), making it more expensive to record other kinds of command line information in a way consistent with the approach chosen (we do not want to waste object flag bits in order to record "this was right hand side tip of the symmetric difference range" and such).

If you are calling "do not waste object flags to represent one special case among endless number of possibilities, as it will make it impossible to extend it" my stance, that hasn't changed.

We added rev_cmdline_info since then so that we can tell what refs were given from the command line in what way, and I thought that we applied a patch from Sverre that uses it instead of the object flags. Am I misremembering things?

On Mon, Nov 26, 2012 at 6:56 PM, Junio C Hamano <gitster@pobox.com> wrote:

The problem with those patches is that they were doing many things at the same time.

You are correct that one of the problems being solved was the fact that we wanted to differentiate B from A in B..A independently of the object, because it might have been referenced by ^C. My latest patch series deals with that by using rev cmdline_info.

But there's another problem that series tried to fix: weather or not A was exported by fast-export, which is not strictly the same as SHOWN.

This becomes a non-issue if my patch series is applied because it properly identifies when an object has been marked or not. But it's not when marks are not used.

For example:

% git branch A v1 % git branch B v0 % git branch C v0 % git branch D v1 % git fast-export B..A ^C D

A would be updated through a 'commit refs/heads/A' command, D would be updated through 'reset refs/heads/D'.

But what if C points to v1? The code will assume A will be exported, and it will be skipped, and there will be only one reset: 'reset refs/heads/D'. Either way it doesn't matter, because the reset would be to mark :0, so even if there was a 'reset refs/heads/A' (because A was never exported), a mark :0 would be useless.

When marks are used my patch fixes the problem because it doesn't care if A was exporeted or not; by knowing it was marked, it knows it was never intended to be exported, so we get resets for both A and D, with real marks.

No, the patch from Sverre was never merged.

Hi Junio,

On Mon, 26 Nov 2012, Junio C Hamano wrote:

Good to know. I will find some time to look at rev_cmdline_info and patch my patch.

It does sound so familiar that I am intended to claim that you remember things correctly.

Ciao, Johannes

On Mon, Nov 26, 2012 at 11:26 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

FWIW, I implemented that in http://thread.gmane.org/gmane.comp.version-control.git/184874 but didn't do the work to get it merged.

Sverre Rabbelier <srabbelier@gmail.com> writes:

Ah, OK. Should I expect an updated series then? How would it interact with the recent work by Felipe?

On 21.11.2012, at 06:08, Junio C Hamano wrote:

Yes, please!

I was wondering about the same, actually... Moreover, I started to try to understand more about this, but found this a bit difficult. Apparently I am primarily supposed to learn about remote helpers by reverse engineering the (sparsely commented, if at all) existing ones. The fact that remote helpers can implement different subsets of the feature spectrum complicates this further. 

Overall, my impression is that there are two kinds of remote helpers:

1) Some are git-to-git helpers, which allow access to another git repos via some intermediate media / protocol (via http, ssh, ...). Those use either connect, or fetch+push. They do not need marks, because they can use the git sha1s. Examples (together with the capabilities they claim to implement):

- remote-curl: fetch, option, push
- remote-ext: connect
- remote-fd: connect

2) Some are interfaces to foreign systems (bzr, hg, mediawiki, ...). They cannot use sha1s and must use marks (at least that is how I understand felipe's explanation). These tools use import combined with either export, or push. Examples:

- git-remote-mediawiki: import, push, refspec
    (its capabilities command also prints "list", but that seems to be a bug?)
- git-remote-hg: import, export, refspec, import-marks, export-marks
    (both the msysgit one and felipe's
- git-remote-bzr: import, push
    (the one from https://github.com/lelutin/git-remote-bzr)
- git-remote-bzr (felipe's): import, export, refspec, *import-marks, *export-marks
    (but why the * ?)

Does that sound about right? If so, can somebody give me a hint when a type 2 helper would use "export" and when "push"?

And while I am at it: git-remote-helpers.txt does not mention the "export", "import-marks" and "export-marks" capabilities. Could somebody who knows what they do look into fixing that? Overall, that doc helped me a bit, but it is more a reference to somebody who already understands in detail how remote helpers work, and who just wants to look up some specific detail :-(. Some hints on when to implement which capabilities might be useful (similar to the "Tips and Tricks" section in git-fast-import.txt).

As it is, felipe's recent explanation on why he thinks marks are essential for remote-helpers (I assume he was only referring to type 2 helpers, though) was one of the most enlightening texts I read on the whole subject so far (then again, I am fairly new to this list, so I may have missed lots of past goodness). Anyway, it would be nice if this could be augmented by "somebody from the other camp" ;).

Cheers, Max

On Wed, Nov 21, 2012 at 11:30 PM, Max Horn <max@quendi.de> wrote:

I don't think remote mediawiki uses marks. They are strictly not needed by 'import' because the remote helper has full control over this operation. For example, in my remote helpers, I store the previous tip of the branches, so when git ask for 'import refs/heads/master', I only import the new commits. I named this file 'marks' anyway, but they are not marks for fast-import.

AFAIK both of my remote helpers have * in them, they are to denote they are required.

I don't know when would be appropriate to use push, there's another git-remote-bzr that uses the bzr-git infrastructure, and uses push.

I picked export/import because I think they are the easiest to implement to get all the features, and should be efficient.

I've had problems finding out the right information, but I hope the git-remote-testgit I wrote in bash in less than 90 lines of code should give a pretty good idea of how the whole thing works.

Wouldn't that be nice?

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

The old behaviour was an incorrect "workaround" that has been superseded by your 14/15 "make sure updated refs get updated", no? Mentioning that would help people realize that this patch would not cause regression on them, I would think.

On Wed, Nov 21, 2012 at 7:14 PM, Junio C Hamano <gitster@pobox.com> wrote:

This particular patch is not getting rid of that "workaround", if you can call it that, it's just making it work correctly.

There's absolutely no possibility of regression (that is known or anybody has mentioned).

The only argument that was put forward was that 'git fast-export ^master master' should throw: from :0

As it does now, because in the future, with another patch (that nobody is pursuing), it might do: from 8c7a786

Which as I have tried to explain; is equally useless.

There's no regression, nobody would be affected negatively by this because when there are no marks, nobody expects a 'from :0'; it's totally useless, and when there are marks, nobody expects an update when the user does '^uninteresting master' for the 'uninteresting' ref. And not even potential future users would be affected, because 'from 8c7a786' is not helpful either, even if the user wanted '^uninsteresting' to be updated (which they won't), the git SHA-1 is useless to a remote helper without marks.

Cheers.

On Sun, Nov 11, 2012 at 2:59 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

There is one case where this can cause problems. I'm about to send another patch series where I fix transport-helper to behave more properly, and in doing so it sends things such as '^old-master master new', and if new points to master, there's a problem. This means the user would have to do 'git push new', instead of 'git push --all'. The transport-helper could do the reset itself, but that would require parsing the marks. A simpler solution for this is proposed in the next patch series.

Cheers.

On Sun, Nov 11, 2012 at 2:59 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Since these are having some problems getting in, let me point out which I think are important, and which not.

This fixes a bug, but it's probably not hitting many people.

I don't care.

These I think are good.

Just cleanups.

I think it's good to catch more issues, but I don't care much.

These are good, but I could drop them.

Obvious and correct, but I don't care.

This is the important one. It fixes real issues quite visible on remote helpers.

This is nice, but can be dropped.

I don't see what are the chances of any of them getting merged, but at least 'fast-export: make sure updated refs get updated' should definitely go in. Please advice at which level I should drop the patches, because at this point it doesn't look like any of them are going in.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

I finished reading the series, and found them mostly sensible.

I'll send out comments on individual patches, and will push them out, interspersed with "fixup!" commits, later on 'pu' when I am done for the day, perhaps in 7 hours or so.

There is one thing I am not sure about with this series, though.

I can agree that the updates to fast-export will make remote-testgit script work better, but I cannot tell how big an impact the changes will have to people's existing use of fast-export. Some of them may be relying on the current behaviour (in other words, they may be relying on "existing bugs"), which may mean that this series will bring regression to them. I am still open to reasonable objections along the lines of "This script X uses fast-export and is broken when used with the updated behaviour." if there is any.

On Wed, Nov 21, 2012 at 8:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

We've discussed about this extensively, and I've asked the same; nobody put forward any. I've also thought long and hard; can't think of any.

Cheers.