Re: [PATCH] git-svn: make batch mode optional for git-cat-file

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2015, 18:25 UTC
Message-ID: <xmqqeghraauu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442843498-22908-1-git-send-email-vleschuk@accesssoftek.com>

Victor Leschuk <vleschuk@gmail.com> writes:

> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
> ---

Before the S-o-b line is a good place to explain why this is a good change to have. Please use it.

Show 13 quoted lines

>  git-svn.perl |  1 +
>  perl/Git.pm  | 41 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 36f7240..b793c26 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' => \$Git::SVN::_follow_parent,
>  		'use-log-author' => \$Git::SVN::_use_log_author,
>  		'add-author-from' => \$Git::SVN::_add_author_from,
>  		'localtime' => \$Git::SVN::_localtime,
> +		'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },

An option whose name begins with no- looks somewhat strange. You can even say --no-no-cat-file-batch from the command line, I suspect.

Why not give an option 'cat-file-batch' that sets the variable $Git::cat_file_batch to false, and initialize the variable to true to keep existing users who do not pass the option happy?

Show 20 quoted lines

>  		%remote_opts );
>  
>  my ($_trunk, @_tags, @_branches, $_stdlayout);
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 19ef081..69e5293 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);
>  use Time::Local qw(timegm);
>  }
>  
> +our $no_cat_file_batch = 0;
>  
>  =head1 CONSTRUCTORS
>  
> @@ -1012,6 +1013,10 @@ returns the number of bytes printed.
>  =cut
>  
>  sub cat_blob {
> +	(1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);

Discard "1 ==" here. You are clearly using the variable as a boolean, so writing this as

	$cat_file_batch ? _cat_blob_batch(@_) : _cat_blob_cmd(@_);

or better yet

	if ($cat_file_batch) {
        	_cat_blob_batch(@_);
	} else {
        	_cat_blob_cmd(@_);
	}

would be more natural.

Show 12 quoted lines

> +}
> +
> +sub _cat_blob_batch {
>  	my ($self, $sha1, $fh) = @_;
>  
>  	$self->_open_cat_blob_if_needed();
> @@ -1072,7 +1077,7 @@ sub cat_blob {
>  sub _open_cat_blob_if_needed {
>  	my ($self) = @_;
>  
> -	return if defined($self->{cat_blob_pid});
> +	return if ( defined($self->{cat_blob_pid}) || 1 == $no_cat_file_batch );

Likewise.

	return if (!$cat_file_batch);
	return if defined($self->{cat_blob_pid});

> +sub _cat_blob_cmd {
> +	my ($self, $sha1, $fh) = @_;
> +...

The biggest thing that is missing from this patch is the explanation of why this is a good thing to do. The batch interface was invented because people found that it was wasteful to spawn a new cat-file process every time the contents of a blob is needed and wanted to avoid it, and this new feature gives the user a way to tell Git to do things in a "wasteful" way, so there must be a reason why the user would want to use the "wasteful" way, perhaps work around some other issue. Without explaining that in the documentation what that issue is, i.e. telling users who reads "git svn --help" when and why the option might help them, nobody would use the feature to benefit from it.

I wonder if "cat-file --batch" is leaky and bloats after running for a while. If that is the case, I have to wonder if "never do batch" like this patch does is a sensible way forward. Instead, "recycle and renew the process after running it for N requests" (and ideally auto-adjust that N without being told by the user) might be a better way to do what you are trying to achieve, but as I already said, I cannot read the motivation behind this change that is not explained, so...