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

From: Stefan Beller <sbeller@google.com>
Date: Dec 7, 2016, 20:48 UTC
Message-ID: <CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com>
In-Reply-To: <20161207194105.25780-2-gitster@pobox.com>

On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> 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>
> ---

So my first question I had to answer was if we do the right thing here, i.e. if we could just fail instead. But we want to continue and just not write back the index, which is fine.

So we do not have to guard refresh_cache, but just call update_index_if_able conditionally.

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

    /*
    * Opportunistically update the index but do not complain if we can't
    */
    void update_index_if_able(struct index_state *istate, struct
lock_file *lockfile)
    {
        if ((istate->cache_changed || has_racy_timestamp(istate)) &&
            verify_index(istate) &&
            write_locked_index(istate, lockfile, COMMIT_LOCK))
                rollback_lock_file(lockfile);
    }

So I would expect that we'd rather fix the update_index_if_able instead by checking for the lockfile to be in the correct state?

Show 26 quoted lines

>  wt-status.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index a2e9d332d8..a715e71906 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)
>  int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
>  {
>         struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> -       int err = 0;
> +       int err = 0, fd;
>
> -       hold_locked_index(lock_file, 0);
> +       fd = hold_locked_index(lock_file, 0);
>         refresh_cache(REFRESH_QUIET);
> -       update_index_if_able(&the_index, lock_file);
> +       if (0 <= fd)
> +               update_index_if_able(&the_index, lock_file);
>         rollback_lock_file(lock_file);
>
>         if (has_unstaged_changes(ignore_submodules)) {
> --
> 2.11.0-274-g0631465056
>