[PATCH] rebase -i: fix has_action

[PATCH] rebase -i: fix has_action

[Noe Rubinstein]

When doing git rebase -i, removing all actions in the todo list is
supposed to result in aborting the rebase. However, if there are spaces
at the beginning of an empty line, has_action returns true and the
rebase therefore removes all commits. This is probably not what a user
leaving a space on an empty line expects.

This patch fixes the bug by changing has_action to grep any line
containing anything that is not a space nor a #.

Signed-off-by: Noe Rubinstein <nrubinstein@proformatique.com>
---
 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c6ba7c1..bed79af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -149,7 +149,7 @@ die_abort () {
 }
 
 has_action () {
-	sane_grep '^[^#]' "$1" >/dev/null
+	sane_grep '^[^#[:space:]]' "$1" >/dev/null
 }
 
 # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
-- 
Noé Rubinstein
Avencall - XiVO IPBX OpenHardware
10 bis, rue Lucien VOILIN - 92800 Puteaux
Tél. : +33 (0)1 41 38 99 60 ext 123
Fax. : +33 (0)1 41 38 99 70

Re: [PATCH] rebase -i: fix has_action

[Sverre Rabbelier]

Heya,

On Thu, Aug 4, 2011 at 11:39, Noe Rubinstein
<nrubinstein@proformatique.com> wrote:
> This patch fixes the bug by changing has_action to grep any line
> containing anything that is not a space nor a #.

Probably maint-worthy? Should this have a test too?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] rebase -i: fix has_action

[Junio C Hamano]

Noe Rubinstein <nrubinstein@proformatique.com> writes:

> When doing git rebase -i, removing all actions in the todo list is
> supposed to result in aborting the rebase.

I thought it was meant to be more like "removing all _lines_", and the
grep was a half-assed attempt to ignore lines that are clearly comments.
Checking the size of the insn sheet might be a better change in that
sense, as that would not leave any ambiguity:

	has_action () {
	  test -s "$1"
	}

> This patch fixes the bug by changing has_action to grep any line
> containing anything that is not a space nor a #.

First of all, I do not think it is a "fixes the bug". I can buy "makes
things safer by detecting user errors", of course.

More importantly, I do not think you are grepping "any line containing
anything that is not a space nor a hash". You are instead grepping lines
that do not begin with a hash or a whitespace, no?

>  has_action () {
> -	sane_grep '^[^#]' "$1" >/dev/null
> +	sane_grep '^[^#[:space:]]' "$1" >/dev/null
>  }

We tend to avoid [:character class:] to accomodate older implementations
of grep.

We earlier asked "do we have any line that begins with a character that is
not a hash '#'?"  but now we say "do we have any line that begins with a
character that is not a hash nor a space?".

If a user fat-fingers an unnecessary space into a blank line, that line
certainly will be excluded. But if the user fat-fingers ^X^I (or >> for vi
users), all lines begin with whitespace and they now get ignored?

How about removing the unnecessary negation from the logic and directly
ask what we really want to know?

That is, "Do we have a line that is _not_ comment?"

	has_action () {
          sane_grep -v -e '^#' -e '^[   ]*$' "$1" >/dev/null
	}

Hmm?

Re: [PATCH] rebase -i: fix has_action

[Sverre Rabbelier]

Heya,

On Thu, Aug 4, 2011 at 21:34, Junio C Hamano <gitster@pobox.com> wrote:
>        has_action () {
>          test -s "$1"
>        }

>        has_action () {
>          sane_grep -v -e '^#' -e '^[   ]*$' "$1" >/dev/null
>        }

I think the former more correctly checks what the function name
implies, is there any downside to that which makes you suggest this
second approach?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] rebase -i: fix has_action

[Johannes Sixt]

Am 8/5/2011 14:36, schrieb Sverre Rabbelier:
> On Thu, Aug 4, 2011 at 21:34, Junio C Hamano <gitster@pobox.com> wrote:
>>        has_action () {
>>          test -s "$1"
>>        }
> 
>>        has_action () {
>>          sane_grep -v -e '^#' -e '^[   ]*$' "$1" >/dev/null
>>        }
> 
> I think the former more correctly checks what the function name
> implies, is there any downside to that which makes you suggest this
> second approach?

Yes. There might be editors where it is difficult to edit a non-empty file
so that it becomes empty. I recall this was a problem for me with vi
before I got intimately acquainted with it.

-- Hannes

Re: [PATCH] rebase -i: fix has_action

[Andrew Wong]

On 08/04/2011 03:34 PM, Junio C Hamano wrote:
> How about removing the unnecessary negation from the logic and directly
> ask what we really want to know?
>
> That is, "Do we have a line that is _not_ comment?"
>
> 	has_action () {
>           sane_grep -v -e '^#' -e '^[   ]*$' "$1" >/dev/null
> 	}
How about also including comments that begins with spaces? i.e.

    has_action () {
        sane_grep -v -e '^[   ]*#' -e '^[   ]*$' "$1" >/dev/null
    }

Also, is [   ] supposed to be a space and a hard tab? They just seem to
be three spaces in my email. We might need to watch out for the hard tab
getting expanded into spaces somewhere during the email process,
especially when applying the patch from email into code.

Andrew

What you can throw (on a Friday)

[Steffen Daode Nurpmeso]

@ Junio C Hamano <gitster@pobox.com> wrote (2011-08-04 21:34+0200):
> Noe Rubinstein <nrubinstein@proformatique.com> writes: [...]
> If a user fat-fingers an unnecessary [...]

Heh.  Note that one of the first mails i've received from this
list was the one which transported the annoyed gasp from
the-one-who-was-born-right-next-door-to-where-the-elks-live.
About scattered non-breaking spaces (U+A0).

Unfortunately that patch series has been thrown away!  And so
i think you underestimate the problem.  On my german keyboard,
f.e., i need to press ALT for all of these: []|{} (ALT + [5-9]).
!!!  Well, there *are* days where my thumb is fast enough to leave
ALT before i hit SPC ...  (But today it yet caused distress.)

After the second scissor i'll append my current pre-commit (and
thus pre-applypatch) (for nothing except to show that it's
a problem people really have to deal with).  But i already thought
about resurrecting your patch-series and reduce it to only the
whitespace check.  Because, being able to say

    exec git diff-index --check --cached $against --

instead would be much easier, because then you could simply state
and request from contributors "please adhere to the whitespace
policy of this project:"

    whitespace = trailing-space,tabwidth=4,tab-in-indent,yell-on-nbsp

> Hmm?

Well and while fooling around and getting more familiar with
git(1) i stumbled over some things which might be caused by bad
control flow instead of being desired behaviour.  After the first
scissor there is a test shell script which reproduces them.

Thanks for git(1) beside that, plastic dishes also break ...
Nice weekend all of you.

--Steffen
Ciao, sdaoden(*)(gmail.com)
ASCII ribbon campaign           ( ) More nuclear fission plants
  against HTML e-mail            X    can serve more coloured
    and proprietary attachments / \     and sounding animations

-- >8 --
#!/bin/sh

error() {
	echo >&2 Error: $*
}

add_file() {
    local f=$1
    echo $f > $f
    git add $f
    git commit -qm $f
}

origin() {
    rm -rf origin
    mkdir origin
    cd origin
    git init -q
    add_file eins
    add_file zwei
    add_file drei
    add_file vier
    add_file fuenf
    git checkout -qb devel
    add_file devel-one
    add_file devel-two
    git checkout -q master
    git tag -m vT1 vT1 HEAD
    cd ..
}

badbad_tagopt() {
    echo
    echo
    echo '1. echo git fetch will --prune away branches if --tags is set'
    echo '   (even if done so through remote.XY.tagopt config).'
    echo '   But nice: it works well again if --all is also given.'
    echo
	work() {
        echo - Am using fetch -q --prune $1 $2
		rm -rf tr1
		mkdir tr1
		cd tr1
		git init -q
		git remote add -t master -t devel -m master origin ../origin

		git fetch -q --prune $1 $2

		git branch -a | grep -F 'origin/master' || error no master branch
		git branch -a | grep -F 'origin/devel' || error no devel branch
		cd ..
		rm -rf tr1
	}


	work '' ''
	work '--tags' ''
    work '--tags' '--all'
}

lazy_ref() {
    echo
    echo
    echo 2. If you do drop/there is no remote.XY.fetch of a branch,
    echo '   but configure the remote.XY.push entry, then after'
    echo '   git push the local ref is not updated, even though'
    echo '   the push succeeded and correctly updated the target repo.'
    echo
	work() {
        echo - remote.origin.fetch will include devel branch: $#
        cp -R origin origin.save
		rm -rf tr1
		mkdir tr1
		cd tr1
		git init -q
		git remote add -t master -m master -t devel origin ../origin
        git config --local remote.origin.push \
                           +refs/heads/master:refs/heads/master
        git config --local --add remote.origin.push \
                            +refs/heads/devel:refs/heads/devel
		git fetch -q --prune
        git checkout -q master
        git checkout -q devel
        add_file test-repo-devel-branch-file

        test $# == 0 &&
            git config --local --replace-all remote.origin.fetch \
                                 +refs/heads/master:refs/remotes/origin/master
        git push -q origin

        x=$(git show-ref --hash devel | sort -u | awk '{++l} END {print l}')
        test $x == 1 || error 'local-ref mismatch'
		cd ..
		rm -rf tr1 origin
        mv origin.save origin
	}

	work YesPlease
	work
}

cd $TMPDIR
mkdir workdir
cd workdir

origin

badbad_tagopt
lazy_ref

cd ..
rm -rf workdir
exit 0
-- >8 --
#!/bin/sh
#@ git(1) pre-commit hook for dummies

#if git rev-parse --verify HEAD >/dev/null 2>&1
#then
    against=HEAD
#else
    # Initial commit: diff against an empty tree object
#   against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
#fi

# Oh no, unfortunately not: exec git diff-index --check --cached $against --
git diff --cached $against | perl -e '
    # XXX 1st version, may not be able to swallow all possible diff output yet
    my ($estat, $l, $fname) = (0, undef, undef);

    for (;;) { last if stdin() =~ /^diff/o; }
    for (;;) { head(); hunk(); }

    sub stdin {
        $l = <STDIN>;
        exit($estat) unless $l;
        chomp($l);
        return $l;
    }

    sub head {
        # Skip anything, including options and entire rename and delete diffs,
        # until we see the ---/+++ line pair
        for (;;) {
            last if $l =~ /^---/o;
            stdin();
        }

        stdin();
        die "head, 1.: cannot parse diff!" unless $l =~ /^\+\+\+ /o;
        $fname = substr($l, 4);
        $fname = substr($fname, 2) if $fname =~ /^b\//o;
    }

    sub hunk() {
        stdin();
        die "hunk, 1.: cannot parse diff!" unless $l =~ /^@@ /o;
JHUNK:
        # regex shamelessly stolen from git(1), and modified
        $l =~ /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/;
        my $lno = $1 - 1;

        for (;;) {
            stdin();
            return if $l =~ /^diff/o;       # Different file?
            goto JHUNK if $l =~ /^@@ /o;    # Same file, different hunk?
            next if $l =~ /^-/o;            # Ignore removals

            ++$lno;
            next if $l =~ /^ /o;
            $l = substr($l, 1);

            if (index($l, "\xA0") != -1) {
                $estat = 1;
                print "$fname:$lno: non-breaking space (NBSP, U+A0).\n";
            }
            if ($l =~ /\s+$/o) {
                $estat = 1;
                print "$fname:$lno: trailing whitespace.\n";
            }
            if ($l =~ /^(\s+)/o && $1 =~ /\x09/o) {
                $estat = 1;
                print "$fname:$lno: tabulator in indent.\n";
            }
        }
    }
    '

Re: [PATCH] rebase -i: fix has_action

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Thu, Aug 4, 2011 at 21:34, Junio C Hamano <gitster@pobox.com> wrote:
>>        has_action () {
>>          test -s "$1"
>>        }
>
>>        has_action () {
>>          sane_grep -v -e '^#' -e '^[   ]*$' "$1" >/dev/null
>>        }
>
> I think the former more correctly checks what the function name
> implies, is there any downside to that which makes you suggest this
> second approach?

I vaguely recall the original reason we didn't do the most straightforward
thing was something like what J6t said already.

As we are not interested in _adding_ new feature, I would say that,
strictly speaking, this *should* become a two-patch series whose first one
uses

	sane_grep -v -e '^#' -e '^$' "$1" >/dev/null

that is, "do we have anything aside from comments and blanks?", which is
the original semantics, with Noe's "safety" change as the second patch in
the series that uses

	sane_grep -v -e '^#' -e '^[	 ]*$' "$1" >/dev/null

to say "let's count a line that solely consists of whitespaces also as a
blank".

But of course in practice it can and should be just a single patch that 
squashes these two "conceptually separate" steps.

Re: [PATCH] rebase -i: fix has_action

[Junio C Hamano]

Andrew Wong <andrew.w@sohovfx.com> writes:

> On 08/04/2011 03:34 PM, Junio C Hamano wrote:
>> How about removing the unnecessary negation from the logic and directly
>> ask what we really want to know?
>>
>> That is, "Do we have a line that is _not_ comment?"
>>
>> 	has_action () {
>>           sane_grep -v -e '^#' -e '^[   ]*$' "$1" >/dev/null
>> 	}
> How about also including comments that begins with spaces? i.e.

Not interested.

It would be _clear_ if you inserted extra space before '#'; Noe's issue is
that it is not clear if you have extra space on a blank line, which I am a
bit more sympathetic.