Hi Junio,
On Mon, 29 Aug 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> 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@xxxxxx>
> > ---
> > 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.
It depends what the caller wants to do. The case about which I care most
is when some helpful advice should be printed (see e.g. 3be18b4 (t5520:
verify that `pull --rebase` shows the helpful advice when failing,
2016-07-26)). Those callers do not need to care, as the atexit() handler
will clean up the lock file.
However, I am sympathetic to your angle, even if I do not expect any such
caller to arise anytime soon.
> 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?
I settled for rolling back in the if() clauses.
Ciao,
Dscho