git rebase -- a suggestion

7 messages, 4 authors, 2016-06-15 · open the first message on its own page

git rebase -- a suggestion

From: Robin Burchell <hidden>
Date: 2016-06-15 22:45:26

Hi,

This is my first mail to this list, so I hope I'm not breaking any
form of ettiquette, etc. If I do step on any toes, feel free to bop me
on the head with a rubber mallet, or steer me in the right direction.

That over, I have a suggestion for `git rebase', from the perspective
of a newcomer.

I've been using git instead of svn (and various other VCS) now for
about a month, and am finding it quite a refreshing change.

I have also recently started a collaborative project exclusively with
git (well, pulling changes from a git-svn repo I don't control) which
has been a valuable ..learning experience.

With this in mind, I'd like to mention exactly what I did.

Upstream had issued a new commit, so I, not knowing the possible
dangers used git-svn rebase to pull in the new changes to our tree.

This "appeared" to work fine, but alarm bells were already going off
in my head before I typed the command (I didn't know at the time I
could merge svn trees like I could normal git branches) as I knew that
rebase rewrote history, and I saw it do this to about 300 commits.

It promptly made merging absolute hell with the other few members of
my team, as it would.

Granted - this is a mistake on my part, and probably a common newbie
one, but something that came to mind when thinking about it later:
would it perhaps be an idea to have a way to mark a tree 'public', and
disallow rebase *unless* --force was passed, or it was a public tree?

(Then again, the alternative might be more 'intelligent' for new
users: start off with branches defaulting to private, and marking them
public, disallowing use of rebase, etc).

Thoughts, feedback, etc are welcome.

-- 
Robin Burchell

Re: git rebase -- a suggestion

From: Nanako Shiraishi <hidden>
Date: 2016-06-15 22:45:26

Documentation/git-rebase.txt talks about pre-rebase hook, but
it appears that Documentation/git-hooks.txt does not have corresponding
entry for it.

Signed-off-by: Nanako Shiraishi <redacted>
---
 "Robin Burchell" [off-list ref] writes:

 > would it perhaps be an idea to have a way to mark a tree 'public', and
 > disallow rebase *unless* --force was passed, or it was a public tree?

 Documentation/githooks.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 046a2a7..567ec03 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt

@@ -130,6 +130,13 @@ parameter, and is invoked after a commit is made.
 This hook is meant primarily for notification, and cannot affect
 the outcome of 'git-commit'.
 
+pre-rebase
+----------
+
+This hook is called by 'git-rebase' and can be used to prevent a branch
+from getting rebased.
+
+
 post-checkout
 -----------

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

[PATCH] Teach rebase -i to honor pre-rebase hook

From: Nanako Shiraishi <hidden>
Date: 2016-06-15 22:45:27

The original git-rebase honored pre-rebase hook so that public branches
can be protected from getting rebased, but rebase --interactive ignored
the hook entirely.  This fixes it.

Signed-off-by: Nanako Shiraishi <redacted>
---
 git-rebase--interactive.sh |   11 ++++
 git-rebase.sh              |   18 ++++---
 t/t3409-rebase-hook.sh     |  126 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+), 7 deletions(-)
 create mode 100755 t/t3409-rebase-hook.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index edb6ec6..3350f90 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -65,6 +65,16 @@ output () {
 	esac
 }
 
+run_pre_rebase_hook () {
+	if test -x "$GIT_DIR/hooks/pre-rebase"
+	then
+		"$GIT_DIR/hooks/pre-rebase" ${1+"$@"} || {
+			echo >&2 "The pre-rebase hook refused to rebase."
+			exit 1
+		}
+	fi
+}
+
 require_clean_work_tree () {
 	# test if working tree is dirty
 	git rev-parse --verify HEAD > /dev/null &&

@@ -507,6 +517,7 @@ first and then run 'git rebase --continue' again."
 		;;
 	--)
 		shift
+		run_pre_rebase_hook ${1+"$@"}
 		test $# -eq 1 -o $# -eq 2 || usage
 		test -d "$DOTEST" &&
 			die "Interactive rebase already started"

diff --git a/git-rebase.sh b/git-rebase.sh
index 528b604..a30d40c 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -144,6 +144,16 @@ is_interactive () {
 	done && test -n "$1"
 }
 
+run_pre_rebase_hook () {
+	if test -x "$GIT_DIR/hooks/pre-rebase"
+	then
+		"$GIT_DIR/hooks/pre-rebase" ${1+"$@"} || {
+			echo >&2 "The pre-rebase hook refused to rebase."
+			exit 1
+		}
+	fi
+}
+
 test -f "$GIT_DIR"/rebase-apply/applying &&
 	die 'It looks like git-am is in progress. Cannot rebase.'

@@ -320,13 +330,7 @@ onto_name=${newbase-"$upstream_name"}
 onto=$(git rev-parse --verify "${onto_name}^0") || exit
 
 # If a hook exists, give it a chance to interrupt
-if test -x "$GIT_DIR/hooks/pre-rebase"
-then
-	"$GIT_DIR/hooks/pre-rebase" ${1+"$@"} || {
-		echo >&2 "The pre-rebase hook refused to rebase."
-		exit 1
-	}
-fi
+run_pre_rebase_hook ${1+"$@"}
 
 # If the branch to rebase is given, that is the branch we will rebase
 # $branch_name -- branch being rebased, or HEAD (already detached)

diff --git a/t/t3409-rebase-hook.sh b/t/t3409-rebase-hook.sh
new file mode 100755
index 0000000..bc93dda
--- /dev/null
+++ b/t/t3409-rebase-hook.sh

@@ -0,0 +1,126 @@
+#!/bin/sh
+
+test_description='git rebase with its hook(s)'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo hello >file &&
+	git add file &&
+	test_tick &&
+	git commit -m initial &&
+	echo goodbye >file &&
+	git add file &&
+	test_tick &&
+	git commit -m second &&
+	git checkout -b side HEAD^ &&
+	echo world >git &&
+	git add git &&
+	test_tick &&
+	git commit -m side &&
+	git checkout master &&
+	git log --pretty=oneline --abbrev-commit --graph --all &&
+	git branch test side
+'
+
+test_expect_success 'rebase' '
+	git checkout test &&
+	git reset --hard side &&
+	git rebase master &&
+	test "z$(cat git)" = zworld
+'
+
+test_expect_success 'rebase -i' '
+	git checkout test &&
+	git reset --hard side &&
+	EDITOR=true git rebase -i master &&
+	test "z$(cat git)" = zworld
+'
+
+test_expect_success 'setup pre-rebase hook' '
+	mkdir -p .git/hooks &&
+	cat >.git/hooks/pre-rebase <<EOF &&
+#!$SHELL_PATH
+echo "\$1,\$2" >.git/PRE-REBASE-INPUT
+EOF
+	chmod +x .git/hooks/pre-rebase
+'
+
+test_expect_success 'pre-rebase hook gets correct input (1)' '
+	git checkout test &&
+	git reset --hard side &&
+	git rebase master &&
+	test "z$(cat git)" = zworld &&
+	test "z$(cat .git/PRE-REBASE-INPUT)" = zmaster,
+
+'
+
+test_expect_success 'pre-rebase hook gets correct input (2)' '
+	git checkout test &&
+	git reset --hard side &&
+	git rebase master test &&
+	test "z$(cat git)" = zworld &&
+	test "z$(cat .git/PRE-REBASE-INPUT)" = zmaster,test
+'
+
+test_expect_success 'pre-rebase hook gets correct input (3)' '
+	git checkout test &&
+	git reset --hard side &&
+	git checkout master &&
+	git rebase master test &&
+	test "z$(cat git)" = zworld &&
+	test "z$(cat .git/PRE-REBASE-INPUT)" = zmaster,test
+'
+
+test_expect_success 'pre-rebase hook gets correct input (4)' '
+	git checkout test &&
+	git reset --hard side &&
+	EDITOR=true git rebase -i master &&
+	test "z$(cat git)" = zworld &&
+	test "z$(cat .git/PRE-REBASE-INPUT)" = zmaster,
+
+'
+
+test_expect_success 'pre-rebase hook gets correct input (5)' '
+	git checkout test &&
+	git reset --hard side &&
+	EDITOR=true git rebase -i master test &&
+	test "z$(cat git)" = zworld &&
+	test "z$(cat .git/PRE-REBASE-INPUT)" = zmaster,test
+'
+
+test_expect_success 'pre-rebase hook gets correct input (6)' '
+	git checkout test &&
+	git reset --hard side &&
+	git checkout master &&
+	EDITOR=true git rebase -i master test &&
+	test "z$(cat git)" = zworld &&
+	test "z$(cat .git/PRE-REBASE-INPUT)" = zmaster,test
+'
+
+test_expect_success 'setup pre-rebase hook that fails' '
+	mkdir -p .git/hooks &&
+	cat >.git/hooks/pre-rebase <<EOF &&
+#!$SHELL_PATH
+false
+EOF
+	chmod +x .git/hooks/pre-rebase
+'
+
+test_expect_success 'pre-rebase hook stops rebase (1)' '
+	git checkout test &&
+	git reset --hard side &&
+	test_must_fail git rebase master &&
+	test "z$(git symbolic-ref HEAD)" = zrefs/heads/test &&
+	test 0 = $(git rev-list HEAD...side | wc -l)
+'
+
+test_expect_success 'pre-rebase hook stops rebase (2)' '
+	git checkout test &&
+	git reset --hard side &&
+	EDITOR=true test_must_fail git rebase -i master &&
+	test "z$(git symbolic-ref HEAD)" = zrefs/heads/test &&
+	test 0 = $(git rev-list HEAD...side | wc -l)
+'
+
+test_done

-- 
1.6.0.2

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

[PATCH] rebase --no-verify

From: Nanako Shiraishi <hidden>
Date: 2016-06-15 22:45:27

It is sometimes desirable to disable the safety net of pre-rebase hook
when the user knows what he is doing (for example, when the original
changes on the branch have not been shown to the public yet).

This teaches --no-verify option to git-rebase, which is similar to the way
pre-commit hook is bypassed by git-commit.

Signed-off-by: Nanako Shiraishi <redacted>
---

    It probably is better to fix "rebase -i" to share more code with the main
    "rebase" script to avoid duplicated run-pre-rebase-hook function, but it
    is beyond what I can do right now.  Perhaps people more smart and
    beautiful than me can help (^_^;)

 git-rebase--interactive.sh |   10 +++++++++-
 git-rebase.sh              |    7 ++++++-
 t/t3409-rebase-hook.sh     |   16 ++++++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3350f90..b0d757d 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -26,6 +26,7 @@ i,interactive      always used (no-op)
 continue           continue rebasing process
 abort              abort rebasing process and restore original branch
 skip               skip current patch and continue rebasing process
+no-verify          override pre-rebase hook from stopping the operation
 "
 
 . git-sh-setup

@@ -41,6 +42,7 @@ PRESERVE_MERGES=
 STRATEGY=
 ONTO=
 VERBOSE=
+OK_TO_SKIP_PRE_REBASE=
 
 GIT_CHERRY_PICK_HELP="  After resolving the conflicts,
 mark the corrected paths with 'git add <paths>', and

@@ -66,7 +68,8 @@ output () {
 }
 
 run_pre_rebase_hook () {
-	if test -x "$GIT_DIR/hooks/pre-rebase"
+	if test -z "$OK_TO_SKIP_PRE_REBASE" &&
+	   test -x "$GIT_DIR/hooks/pre-rebase"
 	then
 		"$GIT_DIR/hooks/pre-rebase" ${1+"$@"} || {
 			echo >&2 "The pre-rebase hook refused to rebase."

@@ -416,6 +419,11 @@ get_saved_options () {
 while test $# != 0
 do
 	case "$1" in
+	--no-verify)
+		OK_TO_SKIP_PRE_REBASE=yes
+		;;
+	--verify)
+		;;
 	--continue)
 		is_standalone "$@" || usage
 		get_saved_options

diff --git a/git-rebase.sh b/git-rebase.sh
index a30d40c..f2742aa 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -34,6 +34,7 @@ set_reflog_action rebase
 require_work_tree
 cd_to_toplevel
 
+OK_TO_SKIP_PRE_REBASE=
 RESOLVEMSG="
 When you have resolved this problem run \"git rebase --continue\".
 If you would prefer to skip this patch, instead run \"git rebase --skip\".

@@ -145,7 +146,8 @@ is_interactive () {
 }
 
 run_pre_rebase_hook () {
-	if test -x "$GIT_DIR/hooks/pre-rebase"
+	if test -z "$OK_TO_SKIP_PRE_REBASE" &&
+	   test -x "$GIT_DIR/hooks/pre-rebase"
 	then
 		"$GIT_DIR/hooks/pre-rebase" ${1+"$@"} || {
 			echo >&2 "The pre-rebase hook refused to rebase."

@@ -170,6 +172,9 @@ fi
 while test $# != 0
 do
 	case "$1" in
+	--no-verify)
+		OK_TO_SKIP_PRE_REBASE=yes
+		;;
 	--continue)
 		test -d "$dotest" -o -d "$GIT_DIR"/rebase-apply ||
 			die "No rebase in progress?"

diff --git a/t/t3409-rebase-hook.sh b/t/t3409-rebase-hook.sh
index bc93dda..1f1b850 100755
--- a/t/t3409-rebase-hook.sh
+++ b/t/t3409-rebase-hook.sh

@@ -123,4 +123,20 @@ test_expect_success 'pre-rebase hook stops rebase (2)' '
 	test 0 = $(git rev-list HEAD...side | wc -l)
 '
 
+test_expect_success 'rebase --no-verify overrides pre-rebase (1)' '
+	git checkout test &&
+	git reset --hard side &&
+	git rebase --no-verify master &&
+	test "z$(git symbolic-ref HEAD)" = zrefs/heads/test &&
+	test "z$(cat git)" = zworld
+'
+
+test_expect_success 'rebase --no-verify overrides pre-rebase (2)' '
+	git checkout test &&
+	git reset --hard side &&
+	EDITOR=true git rebase --no-verify -i master &&
+	test "z$(git symbolic-ref HEAD)" = zrefs/heads/test &&
+	test "z$(cat git)" = zworld
+'
+
 test_done

-- 
1.6.0.2

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] rebase --no-verify

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:45:27

Nanako Shiraishi [off-list ref] wrote:

It is sometimes desirable to disable the safety net of pre-rebase hook
when the user knows what he is doing (for example, when the original
changes on the branch have not been shown to the public yet).

This teaches --no-verify option to git-rebase, which is similar to the way
pre-commit hook is bypassed by git-commit.

Looks good.

    It probably is better to fix "rebase -i" to share more code with the main
    "rebase" script to avoid duplicated run-pre-rebase-hook function, but it
    is beyond what I can do right now.  Perhaps people more smart and
    beautiful than me can help (^_^;)

True.  But its already a mess.  git-sequencer is probably the
right approach to merge it all together.

 git-rebase--interactive.sh |   10 +++++++++-
 git-rebase.sh              |    7 ++++++-
 t/t3409-rebase-hook.sh     |   16 ++++++++++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

Docs?

-- 
Shawn.

Re: [PATCH] rebase --no-verify

From: Stephan Beyer <hidden>
Date: 2016-06-15 22:45:27

Hi,

Shawn O. Pearce wrote:

quoted

    It probably is better to fix "rebase -i" to share more code with the main
    "rebase" script to avoid duplicated run-pre-rebase-hook function, but it
    is beyond what I can do right now.  Perhaps people more smart and
    beautiful than me can help (^_^;)

True.  But its already a mess.  git-sequencer is probably the
right approach to merge it all together.

Hmm, I don't think I like the pre-rebase hook in sequencer. The user
scripts (git-rebase--interactive.sh and git-rebase.sh) should run them;
that's ok.

I think, for the moment it is ok to have the code duplicated.  After
sequencer has merged into master[1], I will probably take a look at
merging git-rebase.sh and git-rebase--interactive.sh if somebody
else is interested in it and if there is a good way to achieve that.

Regards,
  Stephan

Footnotes:
 1. For the *very* interested ones of you,
 	http://repo.or.cz/w/git/sbeyer.git
    is the way to go. seq-builtin-dev is the active development branch
    and git's master is frequently merged into it. seq-builtin-rfc^ is
    an approach to possible patchsets (for review).

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

Re: [PATCH] rebase --no-verify

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:45:27

Stephan Beyer [off-list ref] wrote:

Shawn O. Pearce wrote:

quoted

    It probably is better to fix "rebase -i" to share more code with the main
    "rebase" script to avoid duplicated run-pre-rebase-hook function, but it
    is beyond what I can do right now.  Perhaps people more smart and
    beautiful than me can help (^_^;)

True.  But its already a mess.  git-sequencer is probably the
right approach to merge it all together.

Hmm, I don't think I like the pre-rebase hook in sequencer. The user
scripts (git-rebase--interactive.sh and git-rebase.sh) should run them;
that's ok.

Sorry, my remark wasn't about the rebase hook as much as it was
that there is a good chunk of code duplicated between the two
rebase implementations and all of them were implemented through
git-sequencer its likely they could all collapse into a single
common "git rebase" wrapper script which just sets up the call
to git-sequencer.

So yea, I do agree, the pre-rebase hook should be in rebase, not
git-sequencer, but git-sequencer probably offers a great way to
get the different rebase implementations combined together.

I think, for the moment it is ok to have the code duplicated.  After
sequencer has merged into master[1], I will probably take a look at
merging git-rebase.sh and git-rebase--interactive.sh if somebody
else is interested in it and if there is a good way to achieve that.

Yup, exactly my thoughts.  I just didn't express them well.

-- 
Shawn.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help