Re: [PATCH 1/8] am: suppress error output from a conditional

From: Junio C Hamano <gitster@pobox.com>
Date: May 12, 2013, 03:09 UTC
Message-ID: <7vwqr4vqpv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0mtJU_fWK7A2BXN3f5VRT4Qh69PE0nfQaTJ6GU1Z5=wig@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 7 quoted lines

> Junio C Hamano wrote:
>> If you are introducing "dotest exists but next/last may not be
>> present" as a valid new state, it probably should check the presence
>> and/or absence of them explicitly,
>
> Um, a test -f preceding the cat?  Why, when cat implicitly checks
> existence anyway?

With that logic, 'test -d "$dotest"' on the first line would also be redundant, as we read from "$dotest/last" and that will catch an error.

It is more about what is conceptually right. You need to think what the condition _means_.

We protect the "remove $dotest directory" with the precondition you are changing, but what is that precondition really trying to say? If $dotest does not exist, obviously we do not need to remove it, but what is the essence of that sereis of tests?

It is what the comment says:

    "We have finished applying all the patches in them"

Earlier, presense of $dotest _must_ have meant that last and next should exist (otherwise you have a corrupt state and we did want to see error messages), and the check original did was perfectly fine.

Now, a mere presense of $dotest does _not_ mean "we have finished applying all the patches", because sometimes you create it without having anything to put in last or next yet. That is why it starts to make sense to do

        if test -d "$dotest" &&
           test -f "$dotest/last" &&
           test -f "$dotest/next" &&
           last=$(cat "$dotest/last") &&
           ...