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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html