Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

From: Paul Tan <pyokagan@gmail.com>
Date: Dec 8, 2016, 10:18 UTC
Message-ID: <CACRoPnRpZr=E6SW81Vg-2TiOr=RJo1YouAt5iZoE0CNBx-qesg@mail.gmail.com>
In-Reply-To: <CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com>

Hi Junio,

On Thu, Dec 8, 2016 at 4:48 AM, Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines

> On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The require_clean_work_tree() function calls hold_locked_index()
>> with die_on_error=0 to signal that it is OK if it fails to obtain
>> the lock, but unconditionally calls update_index_if_able(), which
>> will try to write into fd=-1.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---

Ah, sorry about this. I was indeed misled by the function naming and its comment ("do not complain if we can't"). Should have looked more closely at the other call sites.

> However I think the promise of that function is
> to take care of the fd == -1?

Hmm, to add on, looking at the three other call sites of this function, two of them (builtin/commit.c and builtin/describe.c) basically do:

    if (0 <= fd)
        update_index_if_able(...)

with that 0 <= fd conditional. With this patch it becomes three out of four. Perhaps the repeated use of this conditional is a sign that the 0 <= fd check could be built into update_index_if_able()? I think there is precedent for building in these kind of checks -- rollback_lock_file() also does not fail if the lock file was not successfully opened.

That said, the number of call sites is quite low so it's probably not worth doing this.

Thanks, Paul