Fwd: vcs-svn and friends

Fwd: vcs-svn and friends

[David Michael Barr]

Hi,

Thanks to the work of Dmitry, we now have a simple front-end
that exercises the yet unmerged changes to vcs-svn that Jonathan
and I authored a few months ago. I think there's still some work
to be done before we can bless an integrated branch for inclusion.
I'd like to bring attention to just how far we have diverged; see the
email below.

--
David Barr

---------- Forwarded message ----------
From: David Michael Barr <davidbarr@google.com>
Date: Thu, Sep 15, 2011 at 11:37 AM
Subject: Re: vcs-svn and friends
To: Dmitry Ivankov <divanorama@gmail.com>
Cc: Jonathan Nieder <jrnieder@gmail.com>


We now have 56 interesting commits pending:

vcs-svn: add fast_export_note to create notes
vcs-svn,svn-fe: add --incremental option
vcs-svn,svn-fe: allow to disable 'progress' lines
vcs-svn,svn-fe: convert REPORT_FILENO to an option
vcs-svn,svn-fe: allow to specify dump destination ref
vcs-svn: move commit parameters logic to svndump.c
vcs-svn: make svndump_init parameters a struct
svn-fe,test-svn-fe: use parse-options
fast-import: allow top directory as an argument for some commands
fast-import: be saner with temporary trees
svn-fe: reuse import-marks in remote-svn-alpha
svn-fe: import incrementally in svn-remote-alpha
svn-fe: write svnrev notes in remote-svn-alpha
svn-fe: use proper refspec in remote-svn-alpha
svn-fe: use svn-fe --no-progress in remote-svn-alpha
svn-fe: add a test for remote-svn-alpha
svn-fe: allow svnadmin instead of svnrdump in remote-svn-alpha
svn-fe: avoid error on no-op imports in remote-svn-alpha
svn-fe: add man target to Makefile
svn-fe: use svnrdump --quiet in remote-svn-alpha
vcs-svn: reset first_commit_done in fast_export_init
svn-fe: use tabs to indent in remote helper script
svn-fe: do not rely on /bin/env utility to launch remote helper
Add alpha version of remote-svn helper
Arrange a backflow pipe from fast-importer to remote helper stdin
vcs-svn: do not initialize report_buffer twice
vcs-svn: avoid hangs from corrupt deltas
vcs-svn: guard against overflow when computing preimage length
vcs-svn: cap number of bytes read from sliding view
test-svn-fe: split off "test-svn-fe -d" into a separate function
vcs-svn: implement text-delta handling
vcs-svn: let deltas use data from preimage
vcs-svn: let deltas use data from postimage
vcs-svn: verify that deltas consume all inline data
vcs-svn: implement copyfrom_data delta instruction
vcs-svn: read instructions from deltas
vcs-svn: read inline data from deltas
vcs-svn: read the preimage when applying deltas
vcs-svn: parse svndiff0 window header
vcs-svn: skeleton of an svn delta parser
vcs-svn: make buffer_read_binary API more convenient
vcs-svn: learn to maintain a sliding view of a file
Makefile: list one vcs-svn/xdiff object or header per line
vcs-svn: avoid using ls command twice
vcs-svn: drop obj_pool
vcs-svn: drop treap
vcs-svn: drop string_pool
vcs-svn: pass paths through to fast-import
vcs-svn: use mark from previous import for parent commit
vcs-svn: handle filenames with dq correctly
vcs-svn: quote paths correctly for ls command
vcs-svn: eliminate repo_tree structure
vcs-svn: add a comment before each commit
vcs-svn: save marks for imported commits
vcs-svn: use higher mark numbers for blobs
vcs-svn: set up channel to read fast-import cat-blob response

There are a lot of svn-fe tests failing on my integration branch.
One upside is that gph/master..db/svn-fe-pu only contains
relevant commits. I think a little more polish is needed before
we can suggest a pull to jch. In particular, I think we should
include remote-svn-alpha and the test should work out of the
box.

--
David Barr

Re: Fwd: vcs-svn and friends

[Jonathan Nieder]

David Michael Barr wrote:

> Thanks to the work of Dmitry, we now have a simple front-end
> that exercises the yet unmerged changes to vcs-svn that Jonathan
> and I authored a few months ago. I think there's still some work
> to be done before we can bless an integrated branch for inclusion.
> I'd like to bring attention to just how far we have diverged; see the
> email below.

Quick thoughts:

 - everything up to c5bcbcdc looks good to me (as you might expect)

 - later patches seem to be missing your sign-off.  Is this deliberate
   (as in: "withholding sign-off as a hint that these haven't received
   their final review yet") or an oversight?

 - f4472ae61 ("fast-import: be saner with temporary trees"): when I
   looked it over in $gmane/178043, I acked the patch, but not the
   change description.  Re-reading the patch, I've completely forgotten
   what it does and the commit message doesn't help.  What user-visible
   effect would the patch have, if any?

   Reading over $gmane/178043, I learn:

    - new_tree_entry() returns a tree entry from a stack of trees used
      as temporaries.  Initializing them before use is indeed the
      caller's responsibility.

    - parse_ls() uses the following idiom to retrieve content named by
      a tree-ish and path within it:

	struct tree_entry result = {0};
	struct tree_entry *root;

	root = new_tree_entry();
	hashcpy(root->versions[1].sha1, treeish_name);
	load_tree(root);
	tree_content_get(root, path, &result);
	release_tree_entry(root);

      This method that populates "root" only to free it moments later
      is somewhat wasteful --- it would be nicer to stop parsing each
      tree when the appropriate entry is found, which would speed up
      commands in the input stream like

	ls 78a7c87aabc78acb7887c89a98c87ca87ca8ca89 a/a/a/a/a/a/a

      when the relevant trees have many entries.  Oh well.

      This patch is about a detail in that sequence --- the temporary
      tree entry "root" just mentioned has uninitialized fields, such
      as versions[0].sha1.  Nobody accesses them, though, and the result
      from tree_content_get() which is the important thing has no
      uninitialized fields.  So this patch is about futureproofing or
      code clarity rather than an actual functional change.

   Would it be possible to suggest a new change description that
   clarifies that?

 - 3bba32e9 ("fast-import: allow top directory as an argument for some
   commands"): I'm not sure what the motivation is --- is this just
   about the principle of least surprise, or did it come up in practice
   somewhere?

   The change description could use some examples and a reference to
   the earlier related work it seems to be inspired by ("fast-import:
   Allow filemodify to set the root").  It would also be nice to
   update the manpage to document the change at the same time.

 - e9e480e7 ("vcs-svn,svn-fe: convert REPORT_FILENO to an option") has
   nested quote marks in the test.

   The motivating comment "Moreover it may require noticeable effort
   to setup this descriptor, if number 3 is already taken for example"
   is unjustified --- system("foo 3>wherever") or { fork();
   dup2(wherever, 3); execlp("foo", ...) } does not look noticeably
   difficult to me, though maybe there is some unexplained detail that
   makes this require more effort in some circumstance.

Ok, I notice I am starting to nitpick.  Better to make a global
comment: the change descriptions do not currently motivate each change
in a straightforward way.  I'd be glad to help with that by providing
feedback and examples where appropriate if help is needed.  I believe
that fixing this can make other pieces that need fixing easier to find
when they exist.

Thanks for your help,
Jonathan

Re: Fwd: vcs-svn and friends

[Dmitry Ivankov]

>  - 3bba32e9 ("fast-import: allow top directory as an argument for some
>   commands"): I'm not sure what the motivation is --- is this just
>   about the principle of least surprise, or did it come up in practice
>   somewhere?
(to ease one's reading, commands are ls, copy and move top directory)

Haven't seen them in practice. It seemed possible with svn import: if there were
no branches at start, and then someone did svn mv . trunk. But it
turns out that my
svn client doesn't allow such move. So more like a least surprise purpose.

Re: vcs-svn and friends

[Stephen Bash]

----- Original Message -----
> From: "David Michael Barr" <davidbarr@google.com>
> Sent: Wednesday, September 14, 2011 9:53:53 PM
> Subject: Fwd: vcs-svn and friends
> 
> Thanks to the work of Dmitry, we now have a simple front-end
> that exercises the yet unmerged changes to vcs-svn that Jonathan
> and I authored a few months ago.

For those of us interested but out of the loop, does this mean you have a working example where I can point it at a SVN repo and see what happens?  Having done our SVN to Git conversion last year, I know our repo has a lot of the common SVN screw cases (non-branching copies, partial merges, mis-merges, *lots* of retagging, changes committed to tags, etc.) so if it's relatively easy to setup a test I'm happy to run one.

Thanks,
Stephen

Re: vcs-svn and friends

[Jonathan Nieder]

Stephen Bash wrote:

> For those of us interested but out of the loop, does this mean you
> have a working example where I can point it at a SVN repo and see
> what happens?  Having done our SVN to Git conversion last year, I
> know our repo has a lot of the common SVN screw cases (non-branching
> copies, partial merges, mis-merges, *lots* of retagging, changes
> committed to tags, etc.) so if it's relatively easy to setup a test
> I'm happy to run one.

Thanks.  It's very bare-bones at the moment: it just imports each
revision as a whole tree, with no branch and merge tracking at all.
So it would be very interesting to get this basic stuff out into the
wild and then add some code implementing those things for you to break
on top of it.

Re: vcs-svn and friends

[Stephen Bash]

----- Original Message -----
> From: "Jonathan Nieder" <jrnieder@gmail.com>
> Sent: Thursday, September 15, 2011 4:48:15 PM
> Subject: Re: vcs-svn and friends
>
> Stephen Bash wrote:
> 
> > For those of us interested but out of the loop, does this mean you
> > have a working example where I can point it at a SVN repo and see
> > what happens? Having done our SVN to Git conversion last year, I
> > know our repo has a lot of the common SVN screw cases (non-branching
> > copies, partial merges, mis-merges, *lots* of retagging, changes
> > committed to tags, etc.) so if it's relatively easy to setup a test
> > I'm happy to run one.
> 
> Thanks. It's very bare-bones at the moment: it just imports each
> revision as a whole tree, with no branch and merge tracking at all.
> So it would be very interesting to get this basic stuff out into the
> wild and then add some code implementing those things for you to break
> on top of it.

Okay, that matches my last known state of the project, guess I just got optimistic ;)  

Since last summer I've been meaning to take a crack at svn-filter-root.py, but it keeps sliding off the priority list.  If you guys don't beat me to it, maybe I'll eventually get around to it...

Thanks,
Stephen

Re: Fwd: vcs-svn and friends

[Jonathan Nieder]

Dmitry Ivankov wrote:

>>  - 3bba32e9 ("fast-import: allow top directory as an argument for some
>>   commands"): I'm not sure what the motivation is --- is this just
>>   about the principle of least surprise, or did it come up in practice
>>   somewhere?
>
> (to ease one's reading, commands are ls, copy and move top directory)
>
> Haven't seen them in practice. It seemed possible with svn import: if there were
> no branches at start, and then someone did svn mv . trunk. But it
> turns out that my
> svn client doesn't allow such move. So more like a least surprise purpose.

With that information in mind, it sounds like a reasonable change in
principle (though I haven't looked over the code at all).  Could you
propose a log message explaining it (both the original motivation and
the actual impact)?

Thanks,
Jonathan

Re: Fwd: vcs-svn and friends

[Dmitry Ivankov]

On Thu, Sep 15, 2011 at 7:00 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
>>  - 3bba32e9 ("fast-import: allow top directory as an argument for some
>>   commands"): I'm not sure what the motivation is --- is this just
>>   about the principle of least surprise, or did it come up in practice
>>   somewhere?
> (to ease one's reading, commands are ls, copy and move top directory)
>
> Haven't seen them in practice. It seemed possible with svn import: if there were
> no branches at start, and then someone did svn mv . trunk. But it
> turns out that my
> svn client doesn't allow such move. So more like a least surprise purpose.
I think now that this commit should go separately if at all.
Especially considering
my other activity on fast-import (and thus possible merge conflicts) that isn't
strictly necessary for vcs-svn and friends.

Re: Fwd: vcs-svn and friends

[Dmitry Ivankov]

On Fri, Sep 23, 2011 at 7:27 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
> On Thu, Sep 15, 2011 at 7:00 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:
>>>  - 3bba32e9 ("fast-import: allow top directory as an argument for some
>>>   commands"): I'm not sure what the motivation is --- is this just
>>>   about the principle of least surprise, or did it come up in practice
>>>   somewhere?
>> (to ease one's reading, commands are ls, copy and move top directory)
>>
>> Haven't seen them in practice. It seemed possible with svn import: if there were
>> no branches at start, and then someone did svn mv . trunk. But it
>> turns out that my
>> svn client doesn't allow such move. So more like a least surprise purpose.
> I think now that this commit should go separately if at all.
> Especially considering
> my other activity on fast-import (and thus possible merge conflicts) that isn't
> strictly necessary for vcs-svn and friends.
And the same for 3bba32e9^ "fast-import: be saner with temporary trees", which
is only needed for 3bba32e9.

Re: Fwd: vcs-svn and friends

[Jonathan Nieder]

Dmitry Ivankov wrote:
> On Thu, Sep 15, 2011 at 7:00 PM, Dmitry Ivankov <divanorama@gmail.com> wrote:

>> Haven't seen them in practice. It seemed possible with svn import:
>> if there were no branches at start, and then someone did svn mv .
>> trunk. But it turns out that my svn client doesn't allow such
>> move. So more like a least surprise purpose.
>
> I think now that this commit should go separately if at all.
> Especially considering my other activity on fast-import (and thus
> possible merge conflicts) that isn't strictly necessary for vcs-svn
> and friends.

Well, I think it's a good patch, and merge conflicts are really not a
big deal when one has a good patch, especially when it's clear how to
test that the tool still works after resolving.  It's just missing a
clear description...