Re: [PATCH] rebase -i: fix has_action

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2011, 16:59 UTC
Message-ID: <7v62mb4wwd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAGdFq_j2wRw-gB109VypZkG1u=fm7yynkn2-Gu8AzNpVOrun8w@mail.gmail.com>

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.