public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Eric Wong <normalperson@yhbt•net>
To: Florian Ragwitz <rafl@debian•org>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] Add git-svn branch to allow branch creation in SVN repositories
Date: Sat, 6 Sep 2008 19:33:16 -0700	[thread overview]
Message-ID: <20080907023316.GA12392@untitled> (raw)
In-Reply-To: <1220614447-3751-1-git-send-email-rafl@debian.org>

Hi Florian,

Cool.  Some people have been asking for this feature.

I never implemented it myself because I've been unsure if it's
philosophically proper.  Ut's functionally redundant as svn(1) does the
same thing w/o needing a working copy.  But I'll accept your patch with
some minor modifications (comments inline).

Florian Ragwitz <rafl@debian•org> wrote:
> --- a/Documentation/git-svn.txt
> +++ b/Documentation/git-svn.txt
> @@ -149,6 +149,18 @@ and have no uncommitted changes.
>  	is very strongly discouraged.
>  --
>  
> +'branch'::
> +    Create a branch in the SVN repository.

Perhaps a "tag" command that is short for "branch -t" would
be helpful, too.

> +-t;;
> +--tag;;
> +    Create a tag by using the tags_subdir instead of the branches_subdir
> +    specified during git svn init.
> +
> diff --git a/git-svn.perl b/git-svn.perl
> index 7a1d26d..55a2052 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -537,6 +541,30 @@ sub cmd_dcommit {
>  	unlink $gs->{index};
>  }

Coding style things:

1. Don't pad declarations with extra whitespace.
2. No lines over 80 characters (assuming tabs are 8 chars wide)

> +sub cmd_branch {
> +	my $branch_name = shift or die "branch name required\n";
> +	my $head        = shift || 'HEAD';
> +
> +	my ($src, $rev, undef, $gs) = working_head_info($head);
> +
> +	my $remote      = Git::SVN::read_all_remotes()->{svn};

> +	my ($lft, $rgt) = @{ $remote->{ $_tag ? 'tags' : 'branches' }->{path} }{qw/left right/};
> +	my $dst         = join '/', $remote->{url}, $lft, $branch_name, ($rgt || ());

This doesn't appear to fail gracefully for SVN repositories
that don't follow the standard trunk, branches, tags layout.

Can you please ensure that it does and print an error message?

> +
> +	my $ctx = SVN::Client->new(
> +		auth    => Git::SVN::Ra::_auth_providers(),
> +		log_msg => sub { ${ $_[0] } = $_message || 'Create branch ' . $branch_name },
> +	);
> +
> +	eval {
> +		$ctx->ls($dst, 'HEAD', 0);
> +	} and die "branch ${branch_name} already exists\n";

Can you print the URLs being copied and add a --dry-run/-n
option (like dcommit).

> +	$ctx->copy($src, $rev, $dst);
> +
> +	$gs->fetch_all;
> +}
> +
>  sub cmd_find_rev {
>  	my $revision_or_hash = shift or die "SVN or git revision required ",
>  	                                    "as a command-line argument\n";

Can you also add a simple test case so we don't break it in the future?
Thanks.

-- 
Eric Wong

next prev parent reply	other threads:[~2008-09-07  2:34 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-09-05 11:34 [PATCH] Add git-svn branch to allow branch creation in SVN repositories Florian Ragwitz
2008-09-07  2:33 ` Eric Wong [this message]
  -- strict thread matches above, loose matches on Subject: below --
2008-09-11 14:20 Florian Ragwitz
2008-09-18  6:55 ` Eric Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080907023316.GA12392@untitled \
    --to=normalperson@yhbt$(echo .)net \
    --cc=git@vger$(echo .)kernel.org \
    --cc=rafl@debian$(echo .)org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox