Re: Interface and documentation fixups for cvs2{svn,git,hg.bzr}.

Thanks for the patches!

On 01/07/2013 12:16 PM, Eric S. Raymond wrote:
> These are all backward-compatible.  The main theme is reducing the
> number and complexity of command-line options required to invoke the
> tool.  I've also improved the documentation and removed almost all
> Subversion-related fossils from the way the non-cvs2svn manual pages
> are generated.
> 
> It's possible the second patch could be generalized to make the --username
> option optional for all target systems.

Comments and discussion:

Patch 1:

1. I think it would be OK to change the log module to write *all* of its
output to stderr, unconditionally (which would simplify your change).
This information is not meant for computer consumption and I haven't
heard of anybody trying to parse it.  It seems to me that the only thing
that would be lost would be the separation between informational
messages and errors (though even here the user would have to redirect
one or both of the streams to notice any difference).  And anyway I
think we are pretty good about repeating errors at the end of the output
anyway, so they would remain pretty obvious.  What do you think?

2. I split the changes to run_options.py out of the first patch, as they
are not controversial and not necessarily connected to the other changes
in that patch.  This part is already committed and pushed.

3. I think that the temporary files should be managed via
ArtifactManager.  This class ensures that temporary files are written to
the user-selected temporary directory and deleted as soon as they are
not needed anymore (overrideable via the --skip-cleanup option).  On the
plus side, using the ArtifactManager means that you can use constant
filenames for these files if the user has not set other options.  I
suggest that if the user does *not* specify the --blobfile and
--dumpfile command-line options, then None is stored to those fields in
the OutputOptions class to distinguish "use specific file" vs. "use
temporary file and write to stdout at end of run" (rather than misusing
logger.is_streaming_set() for this purpose).  Then on use, if the values
are None then use the standard filename via the artifact manager.

It also evades the minor problem that tempfile.mkstemp() opens the file
whose name it returns but you never close the file.

4. Are all of the combinations of setting/not setting --blobfile and
--dumpfile sensible?  I would imagine the following behavior:

* --blobfile and --dumpfile set: current behavior

* Neither --blobfile nor --dumpfile set: write blobs to temporary file;
copy blobs to stdout at beginning of OutputPass, then during OutputPass
write dump directly to stdout.  Please note that this is a bit different
than what you implemented, which writes the dump via a temporary file
and copies both to stdout at the end of the pass.  It has the advantage
that "git fast-import" can do part of its work in parallel with
OutputPass rather than have to wait until OutputPass is done.

* --blobfile unset, --dumpfile set: write blobs to temporary file; at
beginning of OutputPass copy blobs to dumpfile, then during OutputPass
write rest of dump to dumpfile.  This could be marginally useful because
all of the output would end up in the same file, though the same could
be accomplished by leaving both --blobfile and --dumpfile unset and
redirecting the output to the desired file.

* --blobfile set, --dumpfile unset: this combination seems less useful,
because it would require the user to load the blobfile contents into
"git fast-import" between time time it is generated and OutputPass.

All in all, I think it would be OK (and clearer) to require --blobfile
and --dumpfile to be both set or both unset and not support the mixed
cases.  What do you think?


Patch 2:

I just committed and pushed a slightly different change that sets a
default username for all of the DVCSs.  Thanks for the good idea.


Patch 3:

Although much of the current code and documentation still implies that
the property setter options are only interesting to set Subversion
properties on files in cvs2svn conversions, in fact the same mechanism
is now used to keep track of file-specific information for all of the
backends.  See doc/properties.txt and in particular the descriptions of
the _keyword_handling and _eol_fix properties, which are useful for git
conversions.

These properties can be set via the same very flexible "PropertySetter"
machinery that is used to set Subversion properties.

It would definitely be good to make this section of the command-line
options less Subversion-specific, and some of the PropertySetters are
probably useless for non-cvs2svn conversions.  But moving the whole
section to SVNRunOptions is a step too far; the separation will require
more thought.


Patch 4:

I like the idea, but there is a detail that would have to be addressed
first.

By default, cvs2svn stores temporary files is a subdirectory
"cvs2svn-tmp" of the current directory (changeable via the --tmpdir
option).  This questionable policy predates me.  Your change would
"encourage" users to run cvs2xxx from within their CVS repository, which
would result in the temporary directory being placed there.  I think it
is obvious that this is not such a good idea.

Although the error is not your change but rather the current policy, I
think we should change the tempfile policy before implementing your
change.  Presumably we should follow the standard convention of writing
tempfiles to $TMP.  Concretely, if the user has not specified a
--tmpdir, we should probably create a temporary directory using Python's
tempfile.mkdtemp(prefix=progname).  This temporary directory would of
course have to be recorded in the Context so that later passes know
where to find the files from earlier passes.

Please let me know if you want to work on the tmpdir change; if not, I
will try to get to it sometime in the next days.


Thanks again for the patches!  By the way, if it is more convenient for
you to submit patches via git (e.g., I maintain git-svn mirrors of trunk
at github:mhagger/cvs2svn and at repor.or.gz:cvs2svn), that's fine with me.

Michael

-- 
Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

------------------------------------------------------
http://cvs2svn.tigris.org/ds/viewMessage.do?dsForumId=1667&dsMessageId=3041766

To unsubscribe from this discussion, e-mail: [[email protected]].