[PATCH] rebase -i: remove undocumented '--verify' flag

[PATCH] rebase -i: remove undocumented '--verify' flag

[Martin von Zweigbergk]

Remove the undocumented and unused '--verify' flag from interactive
rebase.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
This tiny and almost pointless patch has been in my refactor-rebase
branch for quite some time. As it's not really related to the
refactoring, I'm sending it off separately instead.

 git-rebase--interactive.sh |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a27952d..f7171e8 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -726,8 +726,6 @@ do
 	--no-verify)
 		OK_TO_SKIP_PRE_REBASE=yes
 		;;
-	--verify)
-		;;
 	--continue)
 		is_standalone "$@" || usage
 		get_saved_options
-- 
1.7.3.2.190.gfb4ae

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Matthieu Moy]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> Remove the undocumented and unused '--verify' flag from interactive
> rebase.

I don't think this change is good. If a command has a --no-whatever
flag, one expects the --whatever flag to exist too, even if it's a
no-op.

>  	--no-verify)
>  		OK_TO_SKIP_PRE_REBASE=yes
>  		;;

--no-verify exists, so

> -	--verify)
> -		;;

--verify exists too.

I think a better change would be to add a comment like

--verify)
	# no-op, exists because --no-verify exists too.
	;;

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Thomas Rast]

Matthieu Moy wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> 
> > Remove the undocumented and unused '--verify' flag from interactive
> > rebase.
> 
> I don't think this change is good. If a command has a --no-whatever
> flag, one expects the --whatever flag to exist too, even if it's a
> no-op.
[...]
> I think a better change would be to add a comment like
> 
> --verify)
> 	# no-op, exists because --no-verify exists too.

Shouldn't that be

  OK_TO_SKIP_PRE_REBASE=

instead, so that it undoes the effect of an earlier --no-verify?

> 	;;
-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Matthieu Moy]

Thomas Rast <trast@student.ethz.ch> writes:

> Matthieu Moy wrote:
>> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>> 
>> > Remove the undocumented and unused '--verify' flag from interactive
>> > rebase.
>> 
>> I don't think this change is good. If a command has a --no-whatever
>> flag, one expects the --whatever flag to exist too, even if it's a
>> no-op.
> [...]
>> I think a better change would be to add a comment like
>> 
>> --verify)
>> 	# no-op, exists because --no-verify exists too.
>
> Shouldn't that be
>
>   OK_TO_SKIP_PRE_REBASE=
>
> instead, so that it undoes the effect of an earlier --no-verify?

Yes, right. Useful when an alias contains --no-whatever in particular.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Martin von Zweigbergk]

On Mon, 22 Nov 2010, Matthieu Moy wrote:

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> Matthieu Moy wrote:
>>> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>>>
>>>> Remove the undocumented and unused '--verify' flag from interactive
>>>> rebase.
>>>
>>> I don't think this change is good. If a command has a --no-whatever
>>> flag, one expects the --whatever flag to exist too, even if it's a
>>> no-op.
>> [...]
>>> I think a better change would be to add a comment like
>>>
>>> --verify)
>>> 	# no-op, exists because --no-verify exists too.
>>
>> Shouldn't that be
>>
>>   OK_TO_SKIP_PRE_REBASE=
>>
>> instead, so that it undoes the effect of an earlier --no-verify?
>
> Yes, right. Useful when an alias contains --no-whatever in particular.


Alright, how about something like this instead?

(I hope this is the correct way of including a patch. I have only used
'git send-email before'. I noticed that Jeff seems to remove the first
three lines and put a '-- 8> --' before, but others do not. What does
the mysterious header mean?)


----
From 90c14fe48ab921ae60000e4f9de02f97f867e273 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
Date: Mon, 22 Nov 2010 20:42:50 +0100
Subject: [PATCH] rebase: support --verify

Interactive rebase allows the '--verify' option to be passed, but it will
be ignored. Implement proper support for the option for both interactive
and non-interactive rebase by making it override any previous
'--no-verify'.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
  Documentation/git-rebase.txt |    4 ++++
  git-rebase--interactive.sh   |    2 ++
  git-rebase.sh                |    3 +++
  3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f3753a8..1f5ce74 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -279,6 +279,10 @@ which makes little sense.
  --no-verify::
  	This option bypasses the pre-rebase hook.  See also linkgit:githooks[5].

+--verify::
+	Allows the pre-rebase hook to run, which is the default.  This option can
+	be used to override --no-verify.  See also linkgit:githooks[5].
+
  -C<n>::
  	Ensure at least <n> lines of surrounding context match before
  	and after each change.  When fewer lines of surrounding
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a27952d..4eabe54 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,6 +28,7 @@ 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
+verify             allow pre-rebase hook to run
  root               rebase all reachable commmits up to the root(s)
  autosquash         move commits that begin with squash!/fixup! under -i
  "
@@ -727,6 +728,7 @@ do
  		OK_TO_SKIP_PRE_REBASE=yes
  		;;
  	--verify)
+		OK_TO_SKIP_PRE_REBASE=
  		;;
  	--continue)
  		is_standalone "$@" || usage
diff --git a/git-rebase.sh b/git-rebase.sh
index 3d194b1..595fca2 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -206,6 +206,9 @@ do
  	--no-verify)
  		OK_TO_SKIP_PRE_REBASE=yes
  		;;
+	--verify)
+		OK_TO_SKIP_PRE_REBASE=
+		;;
  	--continue)
  		test -d "$dotest" -o -d "$GIT_DIR"/rebase-apply ||
  			die "No rebase in progress?"
-- 
1.7.3.2.190.gfb4ae

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Kevin Ballard]

On Nov 22, 2010, at 12:21 PM, Martin von Zweigbergk wrote:

> (I hope this is the correct way of including a patch. I have only used
> 'git send-email before'. I noticed that Jeff seems to remove the first
> three lines and put a '-- 8> --' before, but others do not. What does
> the mysterious header mean?)

It's actually 8< or >8, and it's a little ASCII icon of a pair of scissors.
If a line consists mainly of dashes and scissors then `git am --scissors`
can split the mail on that line and treat the rest of the body after that
line as a patch.

-Kevin Ballard

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Jeff King]

On Mon, Nov 22, 2010 at 06:24:10PM -0800, Kevin Ballard wrote:

> On Nov 22, 2010, at 12:21 PM, Martin von Zweigbergk wrote:
> 
> > (I hope this is the correct way of including a patch. I have only used
> > 'git send-email before'. I noticed that Jeff seems to remove the first
> > three lines and put a '-- 8> --' before, but others do not. What does
> > the mysterious header mean?)
> 
> It's actually 8< or >8, and it's a little ASCII icon of a pair of scissors.
> If a line consists mainly of dashes and scissors then `git am --scissors`
> can split the mail on that line and treat the rest of the body after that
> line as a patch.

Yep. I don't know if Junio actually uses --scissors these days. Before
it existed, he would just snip the parts above the marker manually,
which was not a big deal because he reads his mail in emacs. If that is
still the case, then it doesn't really matter what the marker is, as
long as he recognizes it, and it is not "---", which is the special
marker for splitting commit message from cover letter.

That being said, people other than Junio may apply your patch, so if you
are going to use such a marker, making it look scissor-like is probably
the best thing to do.

-Peff

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That being said, people other than Junio may apply your patch, so if you
> are going to use such a marker, making it look scissor-like is probably
> the best thing to do.

FWIW, I do run "am -sc3" from inside Emacs.  It is a good idea to use what
is already accepted by the tool than coming up with a random convention of
your own anyway, so if somebody really wants to do "introductory comments
in discussion and then a patch", "-- >8 --" without quotes alone on a line
would be an appropriate thing to use.

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> (I hope this is the correct way of including a patch. I have only used
> 'git send-email before'. I noticed that Jeff seems to remove the first
> three lines and put a '-- 8> --' before, but others do not. What does
> the mysterious header mean?)
> -- >8 --
> Subject: [PATCH] rebase: support --verify
>
> Interactive rebase allows the '--verify' option to be passed, but it will
> be ignored. Implement proper support for the option for both interactive
> and non-interactive rebase by making it override any previous
> '--no-verify'.
>
> Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>

Sounds like a sane thing to do.

> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f3753a8..1f5ce74 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -279,6 +279,10 @@ which makes little sense.
>   --no-verify::
>   	This option bypasses the pre-rebase hook.  See also linkgit:githooks[5].
>
> +--verify::
> +	Allows the pre-rebase hook to run, which is the default.  This option can
> +	be used to override --no-verify.  See also linkgit:githooks[5].
> +
>   -C<n>::
>   	Ensure at least <n> lines of surrounding context match before
>   	and after each change.  When fewer lines of surrounding
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index a27952d..4eabe54 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -28,6 +28,7 @@ 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
> +verify             allow pre-rebase hook to run

Somehow this patch seems severely whitespace mangled---please check your
MUA.  I think I've seen Alpine send patches sanely; there should be a way
to tell it to behave.

No need to resend; I can unmunge this patch by hand.

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Martin von Zweigbergk]

On Tue, Nov 23, 2010 at 3:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Somehow this patch seems severely whitespace mangled---please check your
> MUA.  I think I've seen Alpine send patches sanely; there should be a way
> to tell it to behave.

Hmm... It must be the format=flowed stuff, which should be turned off
according to SubmittingPatches. It is apparently on by default in
Alpine. I have now turned it off, so it will hopefully look
better next time. I tried sending the patch to myself, but I couldn't
see any difference either in Gmail or in Alpine. As far as I can
understand, this option only affects the presentation. I didn't see
anything wrong with the raw message.

Sorry about the inconvenience :-(.

> No need to resend; I can unmunge this patch by hand.

Thanks.

Re: [PATCH] rebase -i: remove undocumented '--verify' flag

[Martin von Zweigbergk]

On Mon, Nov 22, 2010 at 8:14 AM, Thomas Rast <trast@student.ethz.ch> wrote:
> Matthieu Moy wrote:
>> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>>
>> > Remove the undocumented and unused '--verify' flag from interactive
>> > rebase.
>>
>> I don't think this change is good. If a command has a --no-whatever
>> flag, one expects the --whatever flag to exist too, even if it's a
>> no-op.
> [...]
>> I think a better change would be to add a comment like
>>
>> --verify)
>>       # no-op, exists because --no-verify exists too.
>
> Shouldn't that be
>
>  OK_TO_SKIP_PRE_REBASE=
>
> instead, so that it undoes the effect of an earlier --no-verify?
>

Yes. But because it did not work like that, it was not documented and it
was only accepted by interactive rebase, I thought it was best to just
remove it.

However, I do understand Matthieu's point about having a '--whatever'
option for every '--no-whatever' option. So, if that is the general
opinion, I wouldn't mind adding the flag to non-interactive rebase as
well. As long as it is consistent, it would simplify things for the user
(and, which is more important to me right now :-), for me while
refactoring this code.)

/Martin