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

Michael Haggerty <[email protected]>:
> 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?

Agreed.  I considered writing it this way, but I was trying to be conservative
and alter existing behavior as little as possible.

> 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.

Thanks.
 
> 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.

That seems reasonable.

> 4. Are all of the combinations of setting/not setting --blobfile and
> --dumpfile sensible?
> 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?

I don't expect to ever use and don't care about the mixed cases.  I
think whatever choice minimizes overall code complexity would be the
right one here.

> 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.

You're welcome.

> 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.

My main objective in this change was not to rearrange the internals, but to
banish Subversion-specific property setters from the generated manual pages
for other back ends.

I think from what you say that each VCS back end will have to declare 
a set of supported property setters.  The manual-page generation should 
then use that to emit the right things into a "PROPERTY SETTERS" section
(note the de-Subversionization of the title).

> 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.

Yes.

> 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.

Agreed.  This would fix a minor wart that I noticed but didn't try to
address, which is the cvs2svn-specificness of the temp directory name.

> 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.

This one is probably most efficiently done by you.  I am at present
up to my ears in trying to get parsecvs to work again.

> 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.

Emailing patches is easy enough for me.  I don't yet know the code well enough
that I'd request commit privileges.

I'll follow up with RFCs on some other changes I'm thinking about.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

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

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