threads / patch / 8716

patch, 3 partsrebase -i: several cleanups

Subject: [PATCH 1/3] rebase -i: several cleanups

## tl;dr

6 messages between Jun 25, 2007 and Jun 27, 2007. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Johannes Schindelin· Jun 25, 2007, 17:56 UTC · lore

Support "--verbose" in addition to "-v", show short names in the list comment, clean up if there is nothing to do, and add several "test_ticks" in the test script.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-rebase--interactive.sh    |   19 +++++++++++++++----
 t/t3404-rebase-interactive.sh |    2 ++
 2 files changed, 17 insertions(+), 4 deletions(-)

Show changes to 2 files +17 −4

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ab36572..b95fe86 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -60,6 +60,11 @@ die_with_patch () {
 	die "$2"
 }
 
+die_abort () {
+	rm -rf "$DOTEST" 2> /dev/null
+	die "$1"
+}
+
 pick_one () {
 	case "$1" in -n) sha1=$2 ;; *) sha1=$1 ;; esac
 	git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
@@ -212,7 +217,7 @@ do
 	-C*)
 		die "Interactive rebase uses merge, so $1 does not make sense"
 		;;
-	-v)
+	-v|--verbose)
 		VERBOSE=t
 		;;
 	-i|--interactive)
@@ -264,8 +269,11 @@ do
 		echo $ONTO > "$DOTEST"/onto
 		test t = "$VERBOSE" && : > "$DOTEST"/verbose
 
+		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
+		SHORTHEAD=$(git rev-parse --short $HEAD)
+		SHORTONTO=$(git rev-parse --short $ONTO)
 		cat > "$TODO" << EOF
-# Rebasing $UPSTREAM..$HEAD onto $ONTO
+# Rebasing $SHORTUPSTREAM)..$SHORTHEAD onto $SHORTONTO
 #
 # Commands:
 #  pick = use commit
@@ -277,13 +285,16 @@ EOF
 			sed "s/^/pick /" >> "$TODO"
 
 		test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
-			die "Nothing to do"
+			die_abort "Nothing to do"
 
 		cp "$TODO" "$TODO".backup
 		${VISUAL:-${EDITOR:-vi}} "$TODO" ||
 			die "Could not execute editor"
 
-		git reset --hard $ONTO && do_rest
+		test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
+			die_abort "Nothing to do"
+
+		git checkout $ONTO && do_rest
 	esac
 	shift
 done
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 48aa8ea..19a3a8e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -140,6 +140,7 @@ test_expect_success 'abort' '
 test_expect_success 'retain authorship' '
 	echo A > file7 &&
 	git add file7 &&
+	test_tick &&
 	GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
 	git tag twerp &&
 	git rebase -i --onto master HEAD^ &&
@@ -149,6 +150,7 @@ test_expect_success 'retain authorship' '
 test_expect_success 'squash' '
 	git reset --hard twerp &&
 	echo B > file7 &&
+	test_tick &&
 	GIT_AUTHOR_NAME="Nitfol" git commit -m "nitfol" file7 &&
 	echo "******************************" &&
 	FAKE_LINES="1 squash 2" git rebase -i --onto master HEAD~2 &&

-- 
1.5.2.2.3172.ge55a1-dirty

Junio C Hamano· Jun 27, 2007, 01:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/3] rebase -i: several cleanups

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 22 quoted lines

> Support "--verbose" in addition to "-v", show short names in the list
> comment, clean up if there is nothing to do, and add several "test_ticks"
> in the test script.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  git-rebase--interactive.sh    |   19 +++++++++++++++----
>  t/t3404-rebase-interactive.sh |    2 ++
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ab36572..b95fe86 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -60,6 +60,11 @@ die_with_patch () {
>  	die "$2"
>  }
>  
> +die_abort () {
> +	rm -rf "$DOTEST" 2> /dev/null
> +	die "$1"
> +}

Why "2>/dev/null" here?

Show 10 quoted lines

> @@ -264,8 +269,11 @@ do
>  		echo $ONTO > "$DOTEST"/onto
>  		test t = "$VERBOSE" && : > "$DOTEST"/verbose
>  
> +		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
> +		SHORTHEAD=$(git rev-parse --short $HEAD)
> +		SHORTONTO=$(git rev-parse --short $ONTO)
>  		cat > "$TODO" << EOF
> -# Rebasing $UPSTREAM..$HEAD onto $ONTO
> +# Rebasing $SHORTUPSTREAM)..$SHORTHEAD onto $SHORTONTO

What is this close-paren about?  Is it a typo?

Johannes Schindelin· Jun 27, 2007, 01:17 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] rebase -i: several cleanups

Hi,

On Tue, 26 Jun 2007, Junio C Hamano wrote:

Show 26 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Support "--verbose" in addition to "-v", show short names in the list
> > comment, clean up if there is nothing to do, and add several "test_ticks"
> > in the test script.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  git-rebase--interactive.sh    |   19 +++++++++++++++----
> >  t/t3404-rebase-interactive.sh |    2 ++
> >  2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index ab36572..b95fe86 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -60,6 +60,11 @@ die_with_patch () {
> >  	die "$2"
> >  }
> >  
> > +die_abort () {
> > +	rm -rf "$DOTEST" 2> /dev/null
> > +	die "$1"
> > +}
> 
> Why "2>/dev/null" here?

Just to be sure. If it does not exist, it's no error. No sense alarming the user.

Show 12 quoted lines

> > @@ -264,8 +269,11 @@ do
> >  		echo $ONTO > "$DOTEST"/onto
> >  		test t = "$VERBOSE" && : > "$DOTEST"/verbose
> >  
> > +		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
> > +		SHORTHEAD=$(git rev-parse --short $HEAD)
> > +		SHORTONTO=$(git rev-parse --short $ONTO)
> >  		cat > "$TODO" << EOF
> > -# Rebasing $UPSTREAM..$HEAD onto $ONTO
> > +# Rebasing $SHORTUPSTREAM)..$SHORTHEAD onto $SHORTONTO
> 
> What is this close-paren about?  Is it a typo?

Darn. It _is_ a typo. I overlooked it _both_ when typing _and_ when looking at the result.

Sorry.

Ciao, Dscho

Junio C Hamano· Jun 27, 2007, 01:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/3] rebase -i: several cleanups

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 9 quoted lines

>> > +die_abort () {
>> > +	rm -rf "$DOTEST" 2> /dev/null
>> > +	die "$1"
>> > +}
>> 
>> Why "2>/dev/null" here?
>
> Just to be sure. If it does not exist, it's no error. No sense alarming 
> the user.

Yeah, but isn't that exactly why you have "-f" there? On the other hand, if $DOTEST exists but for some reason couldn't be removed, we probably would want to know about it.

    $ rm -fr no-such
    $ mkdir no-such
    $ echo >no-such/file
    $ rm -fr no-such
    $ mkdir no-such
    $ echo >no-such/file
    $ chmod a-w no-such
    $ rm -fr no-such
    rm: cannot remove `no-such/file': Permission denied

Johannes Schindelin· Jun 27, 2007, 10:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] rebase -i: several cleanups

Hi,

On Tue, 26 Jun 2007, Junio C Hamano wrote:

Show 25 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > +die_abort () {
> >> > +	rm -rf "$DOTEST" 2> /dev/null
> >> > +	die "$1"
> >> > +}
> >> 
> >> Why "2>/dev/null" here?
> >
> > Just to be sure. If it does not exist, it's no error. No sense alarming 
> > the user.
> 
> Yeah, but isn't that exactly why you have "-f" there?  On the
> other hand, if $DOTEST exists but for some reason couldn't be
> removed, we probably would want to know about it.
> 
>     $ rm -fr no-such
>     $ mkdir no-such
>     $ echo >no-such/file
>     $ rm -fr no-such
>     $ mkdir no-such
>     $ echo >no-such/file
>     $ chmod a-w no-such
>     $ rm -fr no-such
>     rm: cannot remove `no-such/file': Permission denied

Good point.

Okay, let's leave the 2> /dev/null out.

Ciao, Dscho

Junio C Hamano· Jun 27, 2007, 02:00 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/3] rebase -i: several cleanups

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 15 quoted lines

>> > @@ -264,8 +269,11 @@ do
>> >  		echo $ONTO > "$DOTEST"/onto
>> >  		test t = "$VERBOSE" && : > "$DOTEST"/verbose
>> >  
>> > +		SHORTUPSTREAM=$(git rev-parse --short $UPSTREAM)
>> > +		SHORTHEAD=$(git rev-parse --short $HEAD)
>> > +		SHORTONTO=$(git rev-parse --short $ONTO)
>> >  		cat > "$TODO" << EOF
>> > -# Rebasing $UPSTREAM..$HEAD onto $ONTO
>> > +# Rebasing $SHORTUPSTREAM)..$SHORTHEAD onto $SHORTONTO
>> 
>> What is this close-paren about?  Is it a typo?
>
> Darn. It _is_ a typo. I overlooked it _both_ when typing _and_ when 
> looking at the result.

No worries; will fix up. I just wanted to make sure that I did not know the notation that had some meaning.