[PATCH] prohibit "svn dcommit" on remote-tracking-branches

[PATCH] prohibit "svn dcommit" on remote-tracking-branches

[Christian Engwer]

A branch should either be a local copy of an svn branch, or a remote
tracking branch. After a "git svn dcommit" a remote tracking branch
could not be synced with the git remote due to the rebase that occured
during the dcommit. Thus we check for a remote entry in the git config
for the current branch and prohibit the "dcommit" if such an entry
exists.
---
 git-svn.perl |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 4334b95..f9c8440 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -759,6 +759,23 @@ sub cmd_dcommit {
 		'Cannot dcommit with a dirty index.  Commit your changes first, '
 		. "or stash them with `git stash'.\n";
 	$head ||= 'HEAD';
+    my ($branches,$ctx) = eval { command_output_pipe('branch') };
+    my $branch = undef;
+    while (<$branches>) {
+        chomp;
+        if (s/^\* +//)
+        {
+            $branch = $_;
+        }
+    }
+	command_close_pipe($branches, $ctx);
+
+    my $remote = eval { command_oneline('config', '--get',
+                                      "branch.${branch}.remote") };
+	if ($remote) {
+		die "You specified a non-svn remote for branch ${branch}.\n".
+            "To change this setting, modify \"branch.${branch}.remote\" using git config.\n";
+	}
 
 	my $old_head;
 	if ($head ne 'HEAD') {
-- 
1.7.9.1

Re: [PATCH] prohibit "svn dcommit" on remote-tracking-branches

[Eric Wong]

Christian Engwer <christian.engwer@wwu.de> wrote:
> A branch should either be a local copy of an svn branch, or a remote
> tracking branch. After a "git svn dcommit" a remote tracking branch
> could not be synced with the git remote due to the rebase that occured
> during the dcommit. Thus we check for a remote entry in the git config
> for the current branch and prohibit the "dcommit" if such an entry
> exists.

Should there be an option to force/override this?

git-svn predates remote tracking branches, and I've never gotten in the
habit using remote tracking branches.  I'll wait for others to chime
in...

> +        if (s/^\* +//)
> +        {

style, opening brace should be on the same line as the if/while/for:

	if (...) {

Also, indentation should be with hard tabs.  (Basically follow existing
style conventions when you see them).  Thanks.

Re: [PATCH] prohibit "svn dcommit" on remote-tracking-branches

[Christian Engwer]

Hi,

we had a litte discussion on this topic in the debian bug-tracker:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=667488

> > A branch should either be a local copy of an svn branch, or a remote
> > tracking branch. After a "git svn dcommit" a remote tracking branch
> > could not be synced with the git remote due to the rebase that occured
> > during the dcommit. Thus we check for a remote entry in the git config
> > for the current branch and prohibit the "dcommit" if such an entry
> > exists.
> 
> Should there be an option to force/override this?

The suggestion was implement this as a hook, instead of being the
default behavior. An other option is to use the proposed syntax and
allow an explicit overwrite by setting the commiturl, or to add a
completely new option, in order to avoid confusion.

Opinions?

> git-svn predates remote tracking branches, and I've never gotten in the
> habit using remote tracking branches.  I'll wait for others to chime
> in...
> 
> > +        if (s/^\* +//)
> > +        {
> 
> style, opening brace should be on the same line as the if/while/for:
> 
> 	if (...) {
> 
> Also, indentation should be with hard tabs.  (Basically follow existing
> style conventions when you see them).  Thanks.

I'll update the patch.

Cheers
Christian

Re: [PATCH] prohibit "svn dcommit" on remote-tracking-branches

[Christian Engwer]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

Hi!

On Tue, Apr 10, 2012 at 09:17:32PM +0000, Eric Wong wrote:
> Christian Engwer <christian.engwer@wwu.de> wrote:
> > A branch should either be a local copy of an svn branch, or a remote
> > tracking branch. After a "git svn dcommit" a remote tracking branch
> > could not be synced with the git remote due to the rebase that occured
> > during the dcommit. Thus we check for a remote entry in the git config
> > for the current branch and prohibit the "dcommit" if such an entry
> > exists.
> 
> Should there be an option to force/override this?

As stated there is the alternative idea to use pre-dcommit-hook.

I prepared an updated version which behaves as follows:

a) prohibit commit if a remote-tracking branch is used
b) allow commit if --commit-url is used
c) allow commit if the allow-dcommit flag is true in .git/config

Please give comments...

<snip/>

Cheers
Christian


[-- Attachment #2: 0001-prohibit-svn-dcommit-on-remote-tracking-branches.patch --]
[-- Type: text/x-diff, Size: 2058 bytes --]

>From 1e1e151c358b9a8c472e70eaf7aa3f6855554f6c Mon Sep 17 00:00:00 2001
From: Christian Engwer <christian.engwer@wwu.de>
Date: Sun, 15 Apr 2012 19:27:55 +0200
Subject: [PATCH] prohibit "svn dcommit" on remote-tracking-branches

A branch should either be a local copy of an svn branch, or a remote
tracking branch. After a "git svn dcommit" a remote tracking branch
could not be synced with the git remote due to the rebase that occured
during the dcommit. Thus we check for a remote entry in the git config
for the current branch and prohibit the "dcommit" if such an entry
exists.

This behaviour can be overwritten by either providing an explicit commit url
on the command line via "--commit-url" or by setting branch.${branch}.allow-dcommit
to true.
---
 git-svn.perl |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/git-svn.perl b/git-svn.perl
index 4334b95..4a334b9 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -759,6 +759,30 @@ sub cmd_dcommit {
 		'Cannot dcommit with a dirty index.  Commit your changes first, '
 		. "or stash them with `git stash'.\n";
 	$head ||= 'HEAD';
+	my ($branches,$ctx) = eval { command_output_pipe('branch') };
+	my $branch = undef;
+	while (<$branches>) {
+		chomp;
+		if (s/^\* +//) {
+			$branch = $_;
+		}
+	}
+	command_close_pipe($branches, $ctx);
+
+	my $allowdcommit = eval { command_oneline('config', '--get',
+										      "branch.${branch}.allow-dcommit") };
+
+	if ((! defined $_commit_url) and $allowdcommit ne 'true') {
+		my $remote = eval { command_oneline('config', '--get',
+										  "branch.${branch}.remote") };
+		if ($remote) {
+			die "You specified a non-svn remote for branch ${branch}.\n".
+				"To commit to an svn repository, you can either remove the\n".
+				"  \"branch.${branch}.remote\" entry, or explicitly set a commit url\n".
+				"  on the command-line via \"--commit-url\" or override the bahavior\n".
+				"  by setting \"branch.${branch}.allow-dcommit = true\" via git config";
+		}
+	}
 
 	my $old_head;
 	if ($head ne 'HEAD') {
-- 
1.7.9.5

Re: [PATCH] prohibit "svn dcommit" on remote-tracking-branches

[Jonathan Nieder]

(dropping cc to debian bug)
Hi Christian,

Christian Engwer wrote:

> a) prohibit commit if a remote-tracking branch is used
> b) allow commit if --commit-url is used
> c) allow commit if the allow-dcommit flag is true in .git/config

As I mentioned before, I really do want to be able to dcommit from my
current branch, even if it is not called "master" and is set up,
intentionally or not, to pull from a remote git repository.  I would
be a bit bewildered if git required me to say "yes, please do perform
the action I already requested of you" in order to do that, and I do
not think I am the only one.

On the other hand, I believe it would be useful to be able to prevent
_all_ rebasing on some branches to avoid fat-finger accidents.  It
would be especially useful for new users who do not know how to
recover.  This includes the rebasing performed using "git rebase" or
"git reset" by "git svn rebase".

Therefore I would be much happier to see git svn learn a pre-dcommit
hook (see githooks(5)) that can handle multiple situations than the
check that follows the particular policy you have specified above.

That said.  I am reluctant to make the following comment because
I really would love to have that pre-dcommit hook and I do not want to
take away a use case for it. ;-)

Until some interested person introduces a pre-dcommit hook, would it
make sense in your deployment to add a wrapper around "git svn
dcommit" (let's call it git-commit-to-svn) on the $PATH that performs
the check itself, so the user experience would be as follows?

    $ git commit-to-svn
    fatal: this branch is set up to push to git, not to svn
    hint: switch to a new branch before pushing
    hint: or use "git commit-to-svn --force" to override this check

That already works.  It does not require any change to git.  What do
you think?

Just my two cents,
Jonathan