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

Eric,

I've made a number of the changes that we have discussed and pushed them
to the usual places.

On 01/08/2013 11:57 AM, Eric S. Raymond wrote:
> 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.

I have made this change.  Now AFAICT cvs2svn doesn't write anything to
stdout except when the --help, --man, or --help-passes options are
passed to it.

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

I'll leave this to you; let me know if you need more info.

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

I've removed some more explicit references to "cvs2svn" from the
documentation and options files for cvs2{git,hg,bzr} but I haven't made
any changes to the property setter option handling.  It is unlikely that
I will be able to get to this in the near future.

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

I have implemented this change.  Thus it might be possible to integrate
your "Default the repository path to the current directory" patch.  If
you think that patch is ready, please make sure it applies to master and
let me know that I should do the final review.

>> 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 wasn't offering commit privileges :-), only offering to accept your
patches via GitHub pull requests or the like.  But emails are fine, too.

> I'll follow up with RFCs on some other changes I'm thinking about.

I've been very busy but will try to reply when I can.

As I mentioned, my changes are pushed to the cvs2svn SVN repo and to the
git repos mentioned above.  While I was working, I tried to update your
patches to remove the parts that have already been committed or have
been made unnecessary by my changes.  I pushed the remnants to github
under the branch "esr-patches"; maybe they will be useful to you for
your next iteration.

But please note that the first of these "remnant" patches still includes
some changes to logging that are no longer needed now that I have
changed logging to always write to sys.stderr; that part also still has
to be rewritten to record the stream/don't stream bit somewhere else and
manage the new temporary files using the ArtifactManager.  Let me know
if you need any more hints about how to go about this.

Yours,
Michael

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

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

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