threads / patch / 28020

patchrebase -i: fix has_action

Subject: [PATCH] rebase -i: fix has_action

## tl;dr

9 messages between Aug 4, 2011 and Aug 5, 2011. Diffs are folded; open one to read it.

replies: 8people: 6as markdown or json

Noe Rubinstein· Aug 4, 2011, 09:39 UTC · lore

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(-)
Show changes to git-rebase--interactive.sh +1 −1
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
Sverre Rabbelier· Aug 4, 2011, 12:15 UTC · re: Noe Rubinstein · lore

Re: [PATCH] rebase -i: fix has_action

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
Junio C Hamano· Aug 4, 2011, 19:34 UTC · re: Noe Rubinstein · lore

Re: [PATCH] rebase -i: fix has_action

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?
Sverre Rabbelier· Aug 5, 2011, 12:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: fix has_action

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
Johannes Sixt· Aug 5, 2011, 12:46 UTC · re: Sverre Rabbelier · lore

Re: [PATCH] rebase -i: fix has_action

Am 8/5/2011 14:36, schrieb Sverre Rabbelier:
Show 12 quoted lines
> 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
Junio C Hamano· Aug 5, 2011, 16:59 UTC · re: Sverre Rabbelier · lore

Re: [PATCH] rebase -i: fix has_action

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 14 quoted lines
> 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.

Andrew Wong· Aug 5, 2011, 14:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase -i: fix has_action

On 08/04/2011 03:34 PM, Junio C Hamano wrote:
Show 8 quoted lines
> 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
Junio C Hamano· Aug 5, 2011, 17:01 UTC · re: Andrew Wong · lore

Re: [PATCH] rebase -i: fix has_action

Andrew Wong <andrew.w@sohovfx.com> writes:
Show 10 quoted lines
> 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.

Steffen Daode Nurpmeso· Aug 5, 2011, 14:47 UTC · re: Junio C Hamano · lore

What you can throw (on a Friday)

@ 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";
            }
        }
    }
    '

← back to recent threads