Re: [PATCH] rebase -i: fix has_action

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2011, 19:34 UTC
Message-ID: <7vliv93r9g.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1312450780-5021-1-git-send-email-nrubinstein@proformatique.com>

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?