Re: [PATCH v2 08/14] sequencer: lib'ify read_and_refresh_cache()

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Instead of dying there, let the caller high up in the callchain
> notice the error and handle it (by dying, still).
>
> There are two call sites of read_and_refresh_cache(), one of which is
> pick_commits(), whose callers were already prepared to do the right
> thing given an "error" return from it by an earlier patch, so the
> conversion is safe.
>
> The other one, sequencer_pick_revisions() was also prepared to relay
> an error return back to its caller in all remaining cases in an
> earlier patch.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index c006cae..e30aa82 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts)
>  	return 0;
>  }
>  
> -static void read_and_refresh_cache(struct replay_opts *opts)
> +static int read_and_refresh_cache(struct replay_opts *opts)
>  {
>  	static struct lock_file index_lock;
>  	int index_fd = hold_locked_index(&index_lock, 0);
>  	if (read_index_preload(&the_index, NULL) < 0)
> -		die(_("git %s: failed to read the index"), action_name(opts));
> +		return error(_("git %s: failed to read the index"),
> +			action_name(opts));
>  	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
>  	if (the_index.cache_changed && index_fd >= 0) {
>  		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> -			die(_("git %s: failed to refresh the index"), action_name(opts));
> +			return error(_("git %s: failed to refresh the index"),
> +				action_name(opts));
>  	}
>  	rollback_lock_file(&index_lock);
> +	return 0;
>  }

With the current set of callers, a caller that notices an error from
this function will immediately exit without doing any further
damage.

So in that sense, this is a "safe" conversion.

But is it a sensible conversion?  When the caller wants to do
anything else (e.g. clean-up and try something else, perhaps read
the index again), the caller can't, as the index is still locked,
because even though the code knows that the lock will not be
released until the process exit, it chose to return error without
releasing the lock.

For a file-scope static helper, that probably is sufficient.  But if
this can be reached from a public entry point in the API, the caller
of that entry point will find this not-so-useful, I would think.

I suspect doing the "right thing" to future-proof it may not be too
much more work.

	static int read_and_refresh_cache(struct replay_opts *opts)
        {
+        	int retval = 0; /* assume success */
		...
                if (read_idnex_preload(...) < 0) {
			retval = error(...);
                        goto finish;
		}
                refresh_index(...);
                if (...changed...) {
                	if (write_locked_index(...))
                        	retval = error(...);
		}
+	finish:
                rollback_lock_file(&index_lock);
                return retval;
	}

or something like that on top?