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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 29 Aug 2016, Junio C Hamano wrote:

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

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