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

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  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 
> > ---
> >  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(_lock, 0);
> > if (read_index_preload(_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(_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, 
> > NULL);
> > if (the_index.cache_changed && index_fd >= 0) {
> > if (write_locked_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(_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(_lock);
> return retval;
>   }
> 
> or something like that on top?

I settled for rolling back in the if() clauses.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin  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 
> ---
>  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(_lock, 0);
>   if (read_index_preload(_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(_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, 
> NULL);
>   if (the_index.cache_changed && index_fd >= 0) {
>   if (write_locked_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(_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(_lock);
return retval;
}

or something like that on top?

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

[Johannes Schindelin]

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 
---
 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(_lock, 0);
if (read_index_preload(_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(_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, 
NULL);
if (the_index.cache_changed && index_fd >= 0) {
if (write_locked_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(_lock);
+   return 0;
 }
 
 static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
@@ -981,7 +984,8 @@ static int pick_commits(struct commit_list *todo_list, 
struct replay_opts *opts)
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit));
-   read_and_refresh_cache(opts);
+   if (read_and_refresh_cache(opts))
+   return -1;
 
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
@@ -1045,7 +1049,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (opts->subcommand == REPLAY_NONE)
assert(opts->revs);
 
-   read_and_refresh_cache(opts);
+   if (read_and_refresh_cache(opts))
+   return -1;
 
/*
 * Decide what to do depending on the arguments; a fresh
-- 
2.10.0.rc1.99.gcd66998


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html