Re: Fwd: vcs-svn and friends

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 15, 2011, 10:01 UTC
Message-ID: <20110915100106.GB2328@elie>
In-Reply-To: <CAFfmPPOBZ6cXG51mDHbj2VRDzjvH46Q7=_LvUWeMq0SGR40S1g@mail.gmail.com>

David Michael Barr wrote:

Show 6 quoted lines

> 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