[PATCH 00/15] Lib'ify quite a few functions in sequencer.c

[PATCH 00/15] Lib'ify quite a few functions in sequencer.c

[Johannes Schindelin]

This patch series is one of the half dozen patch series left to move the
bulk of rebase -i into a builtin.

The purpose of this patch series is to switch the functions in
sequencer.c from die()ing to returning errors instead, as proper library
functions should do, to give callers a chance to clean up after an
error.


Johannes Schindelin (15):
  sequencer: lib'ify write_message()
  sequencer: lib'ify do_recursive_merge()
  sequencer: lib'ify do_pick_commit()
  sequencer: lib'ify prepare_revs()
  sequencer: lib'ify read_and_refresh_cache()
  sequencer: lib'ify read_populate_todo()
  sequencer: lib'ify read_populate_opts()
  sequencer: lib'ify walk_revs_populate_todo()
  sequencer: lib'ify create_seq_dir()
  sequencer: lib'ify save_head()
  sequencer: lib'ify save_todo()
  sequencer: lib'ify save_opts()
  sequencer: lib'ify sequencer_pick_revisions()
  sequencer: do not die() in do_pick_commit()
  sequencer: do not die() in fast_forward_to()

 sequencer.c | 168 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 101 insertions(+), 67 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v1
Fetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v1

-- 
2.10.0.rc1.99.gcd66998

base-commit: 2632c897f74b1cc9b5533f467da459b9ec725538

[PATCH 01/15] sequencer: lib'ify write_message()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 2e9c7d0..c75296c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 	}
 }
 
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
 {
 	static struct lock_file msg_file;
 
-	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
-					       LOCK_DIE_ON_ERROR);
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
+	if (msg_fd < 0)
+		return error_errno(_("Could not lock '%s'"), filename);
 	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s"), filename);
+		return error_errno(_("Could not write to %s"), filename);
 	strbuf_release(msgbuf);
 	if (commit_lock_file(&msg_file) < 0)
-		die(_("Error wrapping up %s."), filename);
+		return error(_("Error wrapping up %s."), filename);
+
+	return 0;
 }
 
 static struct tree *empty_tree(void)
@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 					 head, &msgbuf, opts);
 		if (res < 0)
 			return res;
-		write_message(&msgbuf, git_path_merge_msg());
+		res |= write_message(&msgbuf, git_path_merge_msg());
 	} else {
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
 
-		write_message(&msgbuf, git_path_merge_msg());
+		res = write_message(&msgbuf, git_path_merge_msg());
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+		res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
 					common, sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 01/15] sequencer: lib'ify write_message()

[Eric Sunshine]

On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
> -static void write_message(struct strbuf *msgbuf, const char *filename)
> +static int write_message(struct strbuf *msgbuf, const char *filename)
>  {
>         static struct lock_file msg_file;
>
> -       int msg_fd = hold_lock_file_for_update(&msg_file, filename,
> -                                              LOCK_DIE_ON_ERROR);
> +       int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
> +       if (msg_fd < 0)
> +               return error_errno(_("Could not lock '%s'"), filename);
>         if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> -               die_errno(_("Could not write to %s"), filename);
> +               return error_errno(_("Could not write to %s"), filename);

Does this need to rollback the lockfile before returning[*]?

[*] I'm not terribly familiar with the lockfile mechanism and I don't
have a lot of time to study it presently, so ignore me if this is a
stupid question.

>         strbuf_release(msgbuf);
>         if (commit_lock_file(&msg_file) < 0)
> -               die(_("Error wrapping up %s."), filename);
> +               return error(_("Error wrapping up %s."), filename);
> +
> +       return 0;
>  }

Re: [PATCH 01/15] sequencer: lib'ify write_message()

[Johannes Schindelin]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
> > -static void write_message(struct strbuf *msgbuf, const char *filename)
> > +static int write_message(struct strbuf *msgbuf, const char *filename)
> >  {
> >         static struct lock_file msg_file;
> >
> > -       int msg_fd = hold_lock_file_for_update(&msg_file, filename,
> > -                                              LOCK_DIE_ON_ERROR);
> > +       int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
> > +       if (msg_fd < 0)
> > +               return error_errno(_("Could not lock '%s'"), filename);
> >         if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> > -               die_errno(_("Could not write to %s"), filename);
> > +               return error_errno(_("Could not write to %s"), filename);
> 
> Does this need to rollback the lockfile before returning[*]?
> 
> [*] I'm not terribly familiar with the lockfile mechanism and I don't
> have a lot of time to study it presently, so ignore me if this is a
> stupid question.

Not a stupid question at all.

The way lockfiles work is that there is an atexit() handler that ensures
that uncommitted lockfiles get rolled back automatically. So it happens to
work correctly right now, because all (direct and transitive) callers
simply stop doing things as quickly as possible. That means that no
subsequent attempt is made to write to the same file.

Technically, I agree with you that it may look a bit cleaner to roll back
at this point. However, this is outside the purview of this here patch
series, as it would actually change the behavior (the previous die() would
rely on the atexit() handler to roll back the locked file, too). So I
think if this is to be changed, it has to be in its own, separate patch
series.

Ciao,
Dscho

[PATCH 02/15] sequencer: lib'ify do_recursive_merge()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index c75296c..0c8c955 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), action_name(opts));
+		return error(_("%s: Unable to write new index file"),
+			action_name(opts));
 	rollback_lock_file(&index_lock);
 
 	if (opts->signoff)
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 02/15] sequencer: lib'ify do_recursive_merge()

[Eric Sunshine]

On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>         if (active_cache_changed &&
>             write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
>                 /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -               die(_("%s: Unable to write new index file"), action_name(opts));
> +               return error(_("%s: Unable to write new index file"),
> +                       action_name(opts));

Does this need to rollback the lockfile before returning?

A cursory scan of read-cache.c:do_write_locked_index() seems to
indicate that lockfile disposition is not handled automatically in
case of error (unless I'm misreading).

>         rollback_lock_file(&index_lock);
>
>         if (opts->signoff)

Re: [PATCH 02/15] sequencer: lib'ify do_recursive_merge()

[Johannes Schindelin]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >         if (active_cache_changed &&
> >             write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> >                 /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> > -               die(_("%s: Unable to write new index file"), action_name(opts));
> > +               return error(_("%s: Unable to write new index file"),
> > +                       action_name(opts));
> 
> Does this need to rollback the lockfile before returning?
> 
> A cursory scan of read-cache.c:do_write_locked_index() seems to
> indicate that lockfile disposition is not handled automatically in
> case of error (unless I'm misreading).

As mentioned elsewhere in this thread: an atexit() handler is tasked with
rolling back uncommitted lockfiles.

Ciao,
Dscho

[PATCH 03/15] sequencer: lib'ify do_pick_commit()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 0c8c955..6ac2187 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		 * to work on.
 		 */
 		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
+			return error (_("Your index file is unmerged."));
 	} else {
 		unborn = get_sha1("HEAD", head);
 		if (unborn)
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 03/15] sequencer: lib'ify do_pick_commit()

[Junio C Hamano]

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

> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Instead of dying there, you let the caller high up in the callchain
to notice the error and handle it (by dying).  The eventual caller
of do_pick_commit() is sequencer_pick_revisions(), which already
relays an reported error from its helper functions (including this
one), and both of its two callers know how to react to a negative
return correctly.  So this makes do_pick_commit() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Good.

>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 0c8c955..6ac2187 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		 * to work on.
>  		 */
>  		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +			return error (_("Your index file is unmerged."));

While you are touching the line, it is a good idea to correct an
obvious style error like this one.  "Do one thing and one thing well
in a commit" is a good discipline, but it is absurd to take it to
the extreme.

>  	} else {
>  		unborn = get_sha1("HEAD", head);
>  		if (unborn)

Re: [PATCH 03/15] sequencer: lib'ify do_pick_commit()

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Aug 2016, Junio C Hamano wrote:

> >  		if (write_cache_as_tree(head, 0, NULL))
> > -			die (_("Your index file is unmerged."));
> > +			return error (_("Your index file is unmerged."));
> 
> While you are touching the line, it is a good idea to correct an
> obvious style error like this one.  "Do one thing and one thing well
> in a commit" is a good discipline, but it is absurd to take it to
> the extreme.

To be quite honest, I had to look *really* hard to see that space. It took
me literally 30 seconds to spot the style issue.

Fixed in v2,
Dscho

[PATCH 04/15] sequencer: lib'ify prepare_revs()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 6ac2187..b90294f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -621,7 +621,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	return res;
 }
 
-static void prepare_revs(struct replay_opts *opts)
+static int prepare_revs(struct replay_opts *opts)
 {
 	/*
 	 * picking (but not reverting) ranges (but not individual revisions)
@@ -631,10 +631,11 @@ static void prepare_revs(struct replay_opts *opts)
 		opts->revs->reverse ^= 1;
 
 	if (prepare_revision_walk(opts->revs))
-		die(_("revision walk setup failed"));
+		return error(_("revision walk setup failed"));
 
 	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
+		return error(_("empty commit set passed"));
+	return 0;
 }
 
 static void read_and_refresh_cache(struct replay_opts *opts)
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 04/15] sequencer: lib'ify prepare_revs()

[Junio C Hamano]

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

> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

I am still looking at sequencer.c in 'master', but I do not think
that the sole caller of this function, walk_revs_populate_todo(),
is prepared to act on an error return from this function and instead
it expects this to die() when in trouble.  And I do not think I saw
the function touched in the steps so far.

So this step smells like a fishy conversion to me.

>  sequencer.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 6ac2187..b90294f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -621,7 +621,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  	return res;
>  }
>  
> -static void prepare_revs(struct replay_opts *opts)
> +static int prepare_revs(struct replay_opts *opts)
>  {
>  	/*
>  	 * picking (but not reverting) ranges (but not individual revisions)
> @@ -631,10 +631,11 @@ static void prepare_revs(struct replay_opts *opts)
>  		opts->revs->reverse ^= 1;
>  
>  	if (prepare_revision_walk(opts->revs))
> -		die(_("revision walk setup failed"));
> +		return error(_("revision walk setup failed"));
>  
>  	if (!opts->revs->commits)
> -		die(_("empty commit set passed"));
> +		return error(_("empty commit set passed"));
> +	return 0;
>  }
>  
>  static void read_and_refresh_cache(struct replay_opts *opts)

Re: [PATCH 04/15] sequencer: lib'ify prepare_revs()

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> I am still looking at sequencer.c in 'master', but I do not think
> that the sole caller of this function, walk_revs_populate_todo(),
> is prepared to act on an error return from this function and instead
> it expects this to die() when in trouble.  And I do not think I saw
> the function touched in the steps so far.
> 
> So this step smells like a fishy conversion to me.

The fishy part is, of course, that I managed not to realize that the patch
libifiying walk_revs_populate_todo() came *after* this patch that libified
one of its callees, prepare_revs().

FWIW the same is true for sequencer_pick_revisions(), which was libified
way too late in the game.

This is once again a demonstration that a final patch series does *not*
reflect the process of developing it: in this case, the order of the
patches is the *opposite* of their development, as I libified functions
from the lowest, deepest call depth up to the top-level functions.

Fixed in v2,
Dscho

[PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

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 b90294f..a8c3a48 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -638,18 +638,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;
 }
 
 static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
@@ -977,7 +980,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);
@@ -1041,7 +1045,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

Re: [PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()

[Eric Sunshine]

On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -638,18 +638,21 @@ static int prepare_revs(struct replay_opts *opts)
> -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));

Do these two error returns need to rollback the lockfile?

>         }
>         rollback_lock_file(&index_lock);
> +       return 0;
>  }

Re: [PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()

[Johannes Schindelin]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -638,18 +638,21 @@ static int prepare_revs(struct replay_opts *opts)
> > -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));
> 
> Do these two error returns need to rollback the lockfile?

Here, too, the atexit() handler does the job, and again, this is not a
change in behavior.

Thanks for your review!
Dscho

Re: [PATCH 05/15] sequencer: lib'ify read_and_refresh_cache()

[Junio C Hamano]

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

> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Instead of dying there, you let the caller high up in the callchain
to notice the error and handle it (by dying).  There are two call
sites of read_and_refresh_cache(), one of which is pick_commits(),
whose callers you already verified that they are prepared to do the
right thing given an "error" return from it when you did 3/15, so
the conversion is safe.  The other one, sequencer_pick_revisions()
is also prepared to relay an error return back to its caller and you
made sure its callers are all safe when you did 3/15.

Good.

> diff --git a/sequencer.c b/sequencer.c
> index b90294f..a8c3a48 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -638,18 +638,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;
>  }
>  
>  static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> @@ -977,7 +980,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);
> @@ -1041,7 +1045,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

[PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a8c3a48..5f6b020 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -746,7 +746,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct commit_list **todo_list,
 			struct replay_opts *opts)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,
 
 	fd = open(git_path_todo_file(), O_RDONLY);
 	if (fd < 0)
-		die_errno(_("Could not open %s"), git_path_todo_file());
+		return error(_("Could not open %s (%s)"),
+			git_path_todo_file(), strerror(errno));
 	if (strbuf_read(&buf, fd, 0) < 0) {
 		close(fd);
 		strbuf_release(&buf);
-		die(_("Could not read %s."), git_path_todo_file());
+		return error(_("Could not read %s."), git_path_todo_file());
 	}
 	close(fd);
 
 	res = parse_insn_buffer(buf.buf, todo_list, opts);
 	strbuf_release(&buf);
 	if (res)
-		die(_("Unusable instruction sheet: %s"), git_path_todo_file());
+		return error(_("Unusable instruction sheet: %s"),
+			git_path_todo_file());
+	return 0;
 }
 
 static int populate_opts_cb(const char *key, const char *value, void *data)
@@ -1015,7 +1018,8 @@ static int sequencer_continue(struct replay_opts *opts)
 	if (!file_exists(git_path_todo_file()))
 		return continue_single_pick();
 	read_populate_opts(&opts);
-	read_populate_todo(&todo_list, opts);
+	if (read_populate_todo(&todo_list, opts))
+		return -1;
 
 	/* Verify that the conflict has been resolved */
 	if (file_exists(git_path_cherry_pick_head()) ||
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Eric Sunshine]

On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,
>
>         fd = open(git_path_todo_file(), O_RDONLY);
>         if (fd < 0)
> -               die_errno(_("Could not open %s"), git_path_todo_file());
> +               return error(_("Could not open %s (%s)"),
> +                       git_path_todo_file(), strerror(errno));

error_errno() perhaps?

>         if (strbuf_read(&buf, fd, 0) < 0) {
>                 close(fd);
>                 strbuf_release(&buf);
> -               die(_("Could not read %s."), git_path_todo_file());
> +               return error(_("Could not read %s."), git_path_todo_file());
>         }
>         close(fd);
>
>         res = parse_insn_buffer(buf.buf, todo_list, opts);
>         strbuf_release(&buf);
>         if (res)
> -               die(_("Unusable instruction sheet: %s"), git_path_todo_file());
> +               return error(_("Unusable instruction sheet: %s"),
> +                       git_path_todo_file());

Neither 'fd' nor 'buf' are leaked by these two new error returns. Good.

> +       return 0;
>  }

Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Johannes Schindelin]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,
> >
> >         fd = open(git_path_todo_file(), O_RDONLY);
> >         if (fd < 0)
> > -               die_errno(_("Could not open %s"), git_path_todo_file());
> > +               return error(_("Could not open %s (%s)"),
> > +                       git_path_todo_file(), strerror(errno));
> 
> error_errno() perhaps?

Absolutely!

Thanks,
Dscho

Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Junio C Hamano]

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

> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Instead of dying there, you let the caller high up in the callchain
to notice the error and handle it (by dying).

The only caller of read_populate_todo(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, you make it notice an
error return from this function.  So this is a safe conversion to
make read_populate_todo() callable from new callers that want it not
to die, without changing the external behaviour of anything
existing.

Good.

By the way, I am writing these as review comments because I do not
want to keep repeating this kind of analysis as a reviewer.  I am
demonstrating what should have been in the commit log message
instead, so that the reviewer does not have to spend extra time, if
the reviewer trusts the author's diligence well enough, to see if
the conversion makes sense.

Please follow the example when/if you have to reroll.  I want the
patches to show the evidence of careful analysis to reviewers so
that they can gauge the trustworthiness of the patches.  With this
round of patches, honestly, I cannot tell if it is a mechanical
substitution alone, or such a substitution followed by a careful
verification of the callers.

> diff --git a/sequencer.c b/sequencer.c
> index a8c3a48..5f6b020 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -746,7 +746,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
>  	return 0;
>  }
>  
> -static void read_populate_todo(struct commit_list **todo_list,
> +static int read_populate_todo(struct commit_list **todo_list,
>  			struct replay_opts *opts)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,
>  
>  	fd = open(git_path_todo_file(), O_RDONLY);
>  	if (fd < 0)
> -		die_errno(_("Could not open %s"), git_path_todo_file());
> +		return error(_("Could not open %s (%s)"),
> +			git_path_todo_file(), strerror(errno));
>  	if (strbuf_read(&buf, fd, 0) < 0) {
>  		close(fd);
>  		strbuf_release(&buf);
> -		die(_("Could not read %s."), git_path_todo_file());
> +		return error(_("Could not read %s."), git_path_todo_file());
>  	}
>  	close(fd);
>  
>  	res = parse_insn_buffer(buf.buf, todo_list, opts);
>  	strbuf_release(&buf);
>  	if (res)
> -		die(_("Unusable instruction sheet: %s"), git_path_todo_file());
> +		return error(_("Unusable instruction sheet: %s"),
> +			git_path_todo_file());
> +	return 0;
>  }
>  
>  static int populate_opts_cb(const char *key, const char *value, void *data)
> @@ -1015,7 +1018,8 @@ static int sequencer_continue(struct replay_opts *opts)
>  	if (!file_exists(git_path_todo_file()))
>  		return continue_single_pick();
>  	read_populate_opts(&opts);
> -	read_populate_todo(&todo_list, opts);
> +	if (read_populate_todo(&todo_list, opts))
> +		return -1;
>  
>  	/* Verify that the conflict has been resolved */
>  	if (file_exists(git_path_cherry_pick_head()) ||

Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> Instead of dying there, you let the caller high up in the callchain
> to notice the error and handle it (by dying).
> 
> The only caller of read_populate_todo(), sequencer_continue() can
> already return errors, so its caller must be already prepared to
> handle error returns, and with this step, you make it notice an
> error return from this function.  So this is a safe conversion to
> make read_populate_todo() callable from new callers that want it not
> to die, without changing the external behaviour of anything
> existing.
> 
> Good.
> 
> By the way, I am writing these as review comments because I do not
> want to keep repeating this kind of analysis as a reviewer.  I am
> demonstrating what should have been in the commit log message
> instead, so that the reviewer does not have to spend extra time, if
> the reviewer trusts the author's diligence well enough, to see if
> the conversion makes sense.
> 
> Please follow the example when/if you have to reroll.  I want the
> patches to show the evidence of careful analysis to reviewers so
> that they can gauge the trustworthiness of the patches.  With this
> round of patches, honestly, I cannot tell if it is a mechanical
> substitution alone, or such a substitution followed by a careful
> verification of the callers.

Duly noted.

I fixed up the patch order as well as the commit messages.

Now on to a request of my own: I am once again reminded that I have *no*
good mail client to serve me. (Before you suggest it: no, emacs won't cut
it for me, nor mutt. Thank you very much for your suggestion.) So it is
really annoying for me to scroll through quoted text, page after page,
only to find that none of it got a response after all.

In short: I would really appreciate it if you could cut quoted text after
your last response.

Thanks,
Dscho

P.S.: It's this mailing list thing again. I would *love* to switch to a
mail client more to my liking, but the ones I like all won't respect white
space in patch files that are pasted verbatim into the mail body.

Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Junio C Hamano]

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

> In short: I would really appreciate it if you could cut quoted text after
> your last response.

I think you are referring to the patch part in this case.

As I was not making point-by-point comments on the proposed commit
log message, quoting only that part and cutting the patch text would
have been pointless.  I could have cut the proposed log message and
left the patch in, though, because the comments were not about what
was in the proposed log message, but about what was not in it [*1*].

I left the patch part for other people's use, to make it easy for
them to see what I was saying was correct and appropriate for what
the patch does.  Removing that would not have made much sense.

But that is only true in this case.  I try to see if I can trim
quote more aggressively, but I would still err on the side of
over-quoting than under-quoting [*2*].


[Footnote]

*1* As to what was IN the proposed log message, I have one comment.
I do not think "To be truly useful" adds any value, as there is
nothing "truly" about what this series does.  The original was
"truly" useful for the purpose of the sequencer machinery and its
use in the current callers, just like with this series it becomes
"truly" useful for envisioned new callers that want to handle error
conditions themselves.  The change is making it "more useful" for
one different use case.  It may not be "truly" useful for other use
cases that sequencer machinery could be used even with the
"eradicate die and exit" change and other people may bring a new use
case that wants it to be even "more useful".  The fact that it may
not be directly usable by a new use case without further change does
not make the result of applying this proposed series less than
"truly useful".  The "truly" adjective implies absolute, but there
is nothing absolute in incremental improvements.  It is always
relative to the context within which the machinery was designed to
be used.  "To make it usable for callers that want to handle errors
themselves (instead of just dying and the calling process handle
it), let's turn die's and exit's to returning negative values." is
probably closer to what I would have expected.

*2* As I read the quoted part before sending my response out, erring
on the side not to underquote tends to avoid a mistake that invites:
"I think you misunderstood what I wrote in the part you snipped from
your quote; perhaps you skimmed it without fully reading."

[PATCH 07/15] sequencer: lib'ify read_populate_opts()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5f6b020..808cd53 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -806,12 +806,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	return 0;
 }
 
-static void read_populate_opts(struct replay_opts **opts_ptr)
+static int read_populate_opts(struct replay_opts **opts)
 {
 	if (!file_exists(git_path_opts_file()))
-		return;
-	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
-		die(_("Malformed options sheet: %s"), git_path_opts_file());
+		return 0;
+	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
+		return error(_("Malformed options sheet: %s"),
+			git_path_opts_file());
+	return 0;
 }
 
 static void walk_revs_populate_todo(struct commit_list **todo_list,
@@ -1017,8 +1019,8 @@ static int sequencer_continue(struct replay_opts *opts)
 
 	if (!file_exists(git_path_todo_file()))
 		return continue_single_pick();
-	read_populate_opts(&opts);
-	if (read_populate_todo(&todo_list, opts))
+	if (read_populate_opts(&opts) ||
+			read_populate_todo(&todo_list, opts))
 		return -1;
 
 	/* Verify that the conflict has been resolved */
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 07/15] sequencer: lib'ify read_populate_opts()

[Junio C Hamano]

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

> -static void read_populate_opts(struct replay_opts **opts_ptr)
> +static int read_populate_opts(struct replay_opts **opts)
>  {
>  	if (!file_exists(git_path_opts_file()))
> -		return;
> -	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
> -		die(_("Malformed options sheet: %s"), git_path_opts_file());
> +		return 0;
> +	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
> +		return error(_("Malformed options sheet: %s"),
> +			git_path_opts_file());
> +	return 0;

This may not be sufficient to avoid die(), unless we know that the
file we are reading is syntactically sound.  git_config_from_file()
will die in config.c::git_parse_source() when the config_source sets
die_on_error, and it is set in config.c::do_config_from_file().

The source we are reading from is created when the sequencer
machinery starts and is only written by save_opts() which is
called by the sequencer machinery using git_config_set*() calls,
so I think it is OK to assume that we won't hit errors that would
cause git_config_from_file() to die, at least for now.

Re: [PATCH 07/15] sequencer: lib'ify read_populate_opts()

[Johannes Schindelin]

Hi Junio,

On Fri, 26 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > -static void read_populate_opts(struct replay_opts **opts_ptr)
> > +static int read_populate_opts(struct replay_opts **opts)
> >  {
> >  	if (!file_exists(git_path_opts_file()))
> > -		return;
> > -	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
> > -		die(_("Malformed options sheet: %s"), git_path_opts_file());
> > +		return 0;
> > +	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
> > +		return error(_("Malformed options sheet: %s"),
> > +			git_path_opts_file());
> > +	return 0;
> 
> This may not be sufficient to avoid die(), unless we know that the
> file we are reading is syntactically sound.  git_config_from_file()
> will die in config.c::git_parse_source() when the config_source sets
> die_on_error, and it is set in config.c::do_config_from_file().
> 
> The source we are reading from is created when the sequencer
> machinery starts and is only written by save_opts() which is
> called by the sequencer machinery using git_config_set*() calls,
> so I think it is OK to assume that we won't hit errors that would
> cause git_config_from_file() to die, at least for now.

I amended the commit message.

Ciao,
Dscho

[PATCH 08/15] sequencer: lib'ify walk_revs_populate_todo()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 808cd53..1bcea84 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -816,17 +816,19 @@ static int read_populate_opts(struct replay_opts **opts)
 	return 0;
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static int walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct commit *commit;
 	struct commit_list **next;
 
-	prepare_revs(opts);
+	if (prepare_revs(opts))
+		return -1;
 
 	next = todo_list;
 	while ((commit = get_revision(opts->revs)))
 		next = commit_list_append(commit, next);
+	return 0;
 }
 
 static int create_seq_dir(void)
@@ -1111,8 +1113,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	 * progress
 	 */
 
-	walk_revs_populate_todo(&todo_list, opts);
-	if (create_seq_dir() < 0)
+	if (walk_revs_populate_todo(&todo_list, opts) ||
+			create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-- 
2.10.0.rc1.99.gcd66998

[PATCH 09/15] sequencer: lib'ify create_seq_dir()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1bcea84..1706ef4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -839,8 +839,8 @@ static int create_seq_dir(void)
 		return -1;
 	}
 	else if (mkdir(git_path_seq_dir(), 0777) < 0)
-		die_errno(_("Could not create sequencer directory %s"),
-			  git_path_seq_dir());
+		return error(_("Could not create sequencer directory %s (%s)"),
+			  git_path_seq_dir(), strerror(errno));
 	return 0;
 }
 
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 09/15] sequencer: lib'ify create_seq_dir()

[Eric Sunshine]

On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -839,8 +839,8 @@ static int create_seq_dir(void)
>                 return -1;
>         }
>         else if (mkdir(git_path_seq_dir(), 0777) < 0)
> -               die_errno(_("Could not create sequencer directory %s"),
> -                         git_path_seq_dir());
> +               return error(_("Could not create sequencer directory %s (%s)"),
> +                         git_path_seq_dir(), strerror(errno));

error_errno()?

>         return 0;
>  }

Re: [PATCH 09/15] sequencer: lib'ify create_seq_dir()

[Johannes Schindelin]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -839,8 +839,8 @@ static int create_seq_dir(void)
> >                 return -1;
> >         }
> >         else if (mkdir(git_path_seq_dir(), 0777) < 0)
> > -               die_errno(_("Could not create sequencer directory %s"),
> > -                         git_path_seq_dir());
> > +               return error(_("Could not create sequencer directory %s (%s)"),
> > +                         git_path_seq_dir(), strerror(errno));
> 
> error_errno()?

Yep!

Thanks,
Dscho

[PATCH 10/15] sequencer: lib'ify save_head()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

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 1706ef4..3e07aa0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -844,18 +844,22 @@ static int create_seq_dir(void)
 	return 0;
 }
 
-static void save_head(const char *head)
+static int save_head(const char *head)
 {
 	static struct lock_file head_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
+	if (fd < 0)
+		return error_errno(_("Could not lock HEAD"));
 	strbuf_addf(&buf, "%s\n", head);
 	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s"), git_path_head_file());
+		return error_errno(_("Could not write to %s"),
+				   git_path_head_file());
 	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), git_path_head_file());
+		return error(_("Error wrapping up %s."), git_path_head_file());
+	return 0;
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
@@ -1118,7 +1122,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-	save_head(sha1_to_hex(sha1));
+	if (save_head(sha1_to_hex(sha1)))
+		return -1;
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 10/15] sequencer: lib'ify save_head()

[Eric Sunshine]

On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -844,18 +844,22 @@ static int create_seq_dir(void)
> -static void save_head(const char *head)
> +static int save_head(const char *head)
>  {
>         static struct lock_file head_lock;
>         struct strbuf buf = STRBUF_INIT;
>         int fd;
>
> -       fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);
> +       fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
> +       if (fd < 0)
> +               return error_errno(_("Could not lock HEAD"));
>         strbuf_addf(&buf, "%s\n", head);
>         if (write_in_full(fd, buf.buf, buf.len) < 0)
> -               die_errno(_("Could not write to %s"), git_path_head_file());
> +               return error_errno(_("Could not write to %s"),
> +                                  git_path_head_file());

Does this need to rollback the lockfile before returning?

>         if (commit_lock_file(&head_lock) < 0)
> -               die(_("Error wrapping up %s."), git_path_head_file());
> +               return error(_("Error wrapping up %s."), git_path_head_file());
> +       return 0;
>  }

Re: [PATCH 10/15] sequencer: lib'ify save_head()

[Johannes Schindelin]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -844,18 +844,22 @@ static int create_seq_dir(void)
> > -static void save_head(const char *head)
> > +static int save_head(const char *head)
> >  {
> >         static struct lock_file head_lock;
> >         struct strbuf buf = STRBUF_INIT;
> >         int fd;
> >
> > -       fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);
> > +       fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
> > +       if (fd < 0)
> > +               return error_errno(_("Could not lock HEAD"));
> >         strbuf_addf(&buf, "%s\n", head);
> >         if (write_in_full(fd, buf.buf, buf.len) < 0)
> > -               die_errno(_("Could not write to %s"), git_path_head_file());
> > +               return error_errno(_("Could not write to %s"),
> > +                                  git_path_head_file());
> 
> Does this need to rollback the lockfile before returning?

Again, this is handled by the atexit() handler, as before.

Thanks!
Dscho

[PATCH 11/15] sequencer: lib'ify save_todo()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3e07aa0..17f2c8b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts)
 	return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 {
 	static struct lock_file todo_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
+	if (fd < 0)
+		return error_errno(_("Could not lock '%s'"),
+				   git_path_todo_file());
 	if (format_todo(&buf, todo_list, opts) < 0)
-		die(_("Could not format %s."), git_path_todo_file());
+		return error(_("Could not format %s."), git_path_todo_file());
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
-		die_errno(_("Could not write to %s"), git_path_todo_file());
+		return error_errno(_("Could not write to %s"),
+				   git_path_todo_file());
 	}
 	if (commit_lock_file(&todo_lock) < 0) {
 		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), git_path_todo_file());
+		return error(_("Error wrapping up %s."), git_path_todo_file());
 	}
 	strbuf_release(&buf);
+	return 0;
 }
 
 static void save_opts(struct replay_opts *opts)
@@ -995,7 +1000,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 		return -1;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
+		if (save_todo(cur, opts))
+			return -1;
 		res = do_pick_commit(cur->item, opts);
 		if (res)
 			return res;
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 11/15] sequencer: lib'ify save_todo()

[Eric Sunshine]

On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts)
> -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> +static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
>  {
>         static struct lock_file todo_lock;
>         struct strbuf buf = STRBUF_INIT;
>         int fd;
>
> -       fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
> +       fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
> +       if (fd < 0)
> +               return error_errno(_("Could not lock '%s'"),
> +                                  git_path_todo_file());
>         if (format_todo(&buf, todo_list, opts) < 0)
> -               die(_("Could not format %s."), git_path_todo_file());
> +               return error(_("Could not format %s."), git_path_todo_file());

format_todo() doesn't seem to make any promises about the state of the
strbuf upon error, so should this be releasing the strbuf before
returning?

>         if (write_in_full(fd, buf.buf, buf.len) < 0) {
>                 strbuf_release(&buf);
> -               die_errno(_("Could not write to %s"), git_path_todo_file());
> +               return error_errno(_("Could not write to %s"),
> +                                  git_path_todo_file());

Do the above two new error returns need to rollback the lockfile?

>         }
>         if (commit_lock_file(&todo_lock) < 0) {
>                 strbuf_release(&buf);
> -               die(_("Error wrapping up %s."), git_path_todo_file());
> +               return error(_("Error wrapping up %s."), git_path_todo_file());
>         }
>         strbuf_release(&buf);
> +       return 0;
>  }

Re: [PATCH 11/15] sequencer: lib'ify save_todo()

[Johannes Schindelin]

Hi Eric,

On Wed, 24 Aug 2016, Eric Sunshine wrote:

> On Tue, Aug 23, 2016 at 12:07 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -929,24 +929,29 @@ static int sequencer_rollback(struct replay_opts *opts)
> > -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> > +static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> >  {
> >         static struct lock_file todo_lock;
> >         struct strbuf buf = STRBUF_INIT;
> >         int fd;
> >
> > -       fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
> > +       fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
> > +       if (fd < 0)
> > +               return error_errno(_("Could not lock '%s'"),
> > +                                  git_path_todo_file());
> >         if (format_todo(&buf, todo_list, opts) < 0)
> > -               die(_("Could not format %s."), git_path_todo_file());
> > +               return error(_("Could not format %s."), git_path_todo_file());
> 
> format_todo() doesn't seem to make any promises about the state of the
> strbuf upon error, so should this be releasing the strbuf before
> returning?

Yes, it should. Thank you!

> >         if (write_in_full(fd, buf.buf, buf.len) < 0) {
> >                 strbuf_release(&buf);
> > -               die_errno(_("Could not write to %s"), git_path_todo_file());
> > +               return error_errno(_("Could not write to %s"),
> > +                                  git_path_todo_file());
> 
> Do the above two new error returns need to rollback the lockfile?

As before, atexit() handler to the rescue ;-)

Thanks,
Dscho

[PATCH 12/15] sequencer: lib'ify save_opts()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 17f2c8b..bac32ea 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -954,37 +954,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	return 0;
 }
 
-static void save_opts(struct replay_opts *opts)
+static int save_opts(struct replay_opts *opts)
 {
 	const char *opts_file = git_path_opts_file();
+	int res = 0;
 
 	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.no-commit", "true");
 	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.edit", "true");
 	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true");
 	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true");
 	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true");
 	if (opts->mainline) {
 		struct strbuf buf = STRBUF_INIT;
 		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+		res |= git_config_set_in_file_gently(opts_file, "options.mainline", buf.buf);
 		strbuf_release(&buf);
 	}
 	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+		res |= git_config_set_in_file_gently(opts_file, "options.strategy", opts->strategy);
 	if (opts->gpg_sign)
-		git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign);
+		res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign);
 	if (opts->xopts) {
 		int i;
 		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
+			res |= git_config_set_multivar_in_file_gently(opts_file,
 							"options.strategy-option",
 							opts->xopts[i], "^$", 0);
 	}
+	return res;
 }
 
 static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
@@ -1128,9 +1130,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-	if (save_head(sha1_to_hex(sha1)))
+	if (save_head(sha1_to_hex(sha1)) ||
+			save_opts(opts))
 		return -1;
-	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
 
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 12/15] sequencer: lib'ify save_opts()

[Junio C Hamano]

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

>  static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
> @@ -1128,9 +1130,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>  		return -1;
>  	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
>  		return error(_("Can't revert as initial commit"));
> -	if (save_head(sha1_to_hex(sha1)))
> +	if (save_head(sha1_to_hex(sha1)) ||
> +			save_opts(opts))
>  		return -1;
> -	save_opts(opts);

I think it is much easier to read to keep this on a single line.  It
would be more verbose but an even easier would be to keep these two
operations two separate steps, i.e.

        if (save_head())
                return -1;
        if (save_opts())
                return -1;

Re: [PATCH 12/15] sequencer: lib'ify save_opts()

[Johannes Schindelin]

Hi Junio,

On Fri, 26 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> >  static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
> > @@ -1128,9 +1130,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
> >  		return -1;
> >  	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
> >  		return error(_("Can't revert as initial commit"));
> > -	if (save_head(sha1_to_hex(sha1)))
> > +	if (save_head(sha1_to_hex(sha1)) ||
> > +			save_opts(opts))
> >  		return -1;
> > -	save_opts(opts);
> 
> I think it is much easier to read to keep this on a single line.  It
> would be more verbose but an even easier would be to keep these two
> operations two separate steps, i.e.
> 
>         if (save_head())
>                 return -1;
>         if (save_opts())
>                 return -1;

Done,
Dscho

[PATCH 13/15] sequencer: lib'ify sequencer_pick_revisions()

[Johannes Schindelin]

To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bac32ea..324463f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1093,10 +1093,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		if (!get_sha1(name, sha1)) {
 			if (!lookup_commit_reference_gently(sha1, 1)) {
 				enum object_type type = sha1_object_info(sha1, NULL);
-				die(_("%s: can't cherry-pick a %s"), name, typename(type));
+				return error(_("%s: can't cherry-pick a %s"),
+					name, typename(type));
 			}
 		} else
-			die(_("%s: bad revision"), name);
+			return error(_("%s: bad revision"), name);
 	}
 
 	/*
@@ -1112,10 +1113,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	    !opts->revs->cmdline.rev->flags) {
 		struct commit *cmit;
 		if (prepare_revision_walk(opts->revs))
-			die(_("revision walk setup failed"));
+			return error(_("revision walk setup failed"));
 		cmit = get_revision(opts->revs);
 		if (!cmit || get_revision(opts->revs))
-			die("BUG: expected exactly one commit from walk");
+			return error("BUG: expected exactly one commit from walk");
 		return single_pick(cmit, opts);
 	}
 
-- 
2.10.0.rc1.99.gcd66998

[PATCH 14/15] sequencer: do not die() in do_pick_commit()

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 324463f..c29de64 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -589,12 +589,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
-	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
-		update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&
+	    update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
+		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		res = -1;
+	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
+	    update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
+		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		res = -1;
 
 	if (res) {
 		error(opts->action == REPLAY_REVERT
-- 
2.10.0.rc1.99.gcd66998

[PATCH 15/15] sequencer: do not die() in fast_forward_to()

[Johannes Schindelin]

A fast-forward may fail e.g. when it would overwrite an untracked
file. We still must not exit() in that case: the sequencer is
supposedly providing a library function.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index c29de64..9ddd054 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
-		exit(128); /* the callee should have complained already */
+		return -1; /* the callee should have complained already */
 
 	strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts));
 
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 00/14] Lib'ify quite a few functions in sequencer.c

[Johannes Schindelin]

This patch series is one of the half dozen patch series left to move the
bulk of rebase -i into a builtin.

The purpose of this patch series is to switch the functions in
sequencer.c from die()ing to returning errors instead, as proper library
functions should do, to give callers a chance to clean up after an
error.

Changes since v1:

- two "return error()"s replacing "die_errno()"s were turned into "return
  error_errno()"s instead.

- an strbuf is now released when format_todo() failed (and may have left
  the strbuf with allocated memory).

- a superfluous space (which was inherited from the previous code) was
  fixed, while at it.

- fixed commit messages to report that callers of the libified functions
  are already libified.

- reordered patches to ensure that callers of libified functions are
  already libified.


Johannes Schindelin (14):
  sequencer: lib'ify sequencer_pick_revisions()
  sequencer: do not die() in do_pick_commit()
  sequencer: lib'ify write_message()
  sequencer: lib'ify do_recursive_merge()
  sequencer: lib'ify do_pick_commit()
  sequencer: lib'ify walk_revs_populate_todo()
  sequencer: lib'ify prepare_revs()
  sequencer: lib'ify read_and_refresh_cache()
  sequencer: lib'ify read_populate_todo()
  sequencer: lib'ify read_populate_opts()
  sequencer: lib'ify create_seq_dir()
  sequencer: lib'ify save_head()
  sequencer: lib'ify save_todo()
  sequencer: lib'ify save_opts()

 sequencer.c | 172 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 104 insertions(+), 68 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v2
Fetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v2

Interdiff vs v1:

 diff --git a/sequencer.c b/sequencer.c
 index caba11d..b6481bb 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
  		 * to work on.
  		 */
  		if (write_cache_as_tree(head, 0, NULL))
 -			return error (_("Your index file is unmerged."));
 +			return error(_("Your index file is unmerged."));
  	} else {
  		unborn = get_sha1("HEAD", head);
  		if (unborn)
 @@ -756,8 +756,8 @@ static int read_populate_todo(struct commit_list **todo_list,
  
  	fd = open(git_path_todo_file(), O_RDONLY);
  	if (fd < 0)
 -		return error(_("Could not open %s (%s)"),
 -			git_path_todo_file(), strerror(errno));
 +		return error_errno(_("Could not open %s"),
 +				   git_path_todo_file());
  	if (strbuf_read(&buf, fd, 0) < 0) {
  		close(fd);
  		strbuf_release(&buf);
 @@ -841,8 +841,8 @@ static int create_seq_dir(void)
  		return -1;
  	}
  	else if (mkdir(git_path_seq_dir(), 0777) < 0)
 -		return error(_("Could not create sequencer directory %s (%s)"),
 -			  git_path_seq_dir(), strerror(errno));
 +		return error_errno(_("Could not create sequencer directory %s"),
 +				   git_path_seq_dir());
  	return 0;
  }
  
 @@ -941,8 +941,10 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
  	if (fd < 0)
  		return error_errno(_("Could not lock '%s'"),
  				   git_path_todo_file());
 -	if (format_todo(&buf, todo_list, opts) < 0)
 +	if (format_todo(&buf, todo_list, opts) < 0) {
 +		strbuf_release(&buf);
  		return error(_("Could not format %s."), git_path_todo_file());
 +	}
  	if (write_in_full(fd, buf.buf, buf.len) < 0) {
  		strbuf_release(&buf);
  		return error_errno(_("Could not write to %s"),

-- 
2.10.0.rc1.99.gcd66998

base-commit: 5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286

[PATCH v2 01/14] sequencer: lib'ify sequencer_pick_revisions()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The function sequencer_pick_revisions() has only two callers,
cmd_revert() and cmd_cherry_pick(), both of which check the return
value and react appropriately upon errors.

So this is a safe conversion to make sequencer_pick_revisions()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3804fa9..76b1c52 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		if (!get_sha1(name, sha1)) {
 			if (!lookup_commit_reference_gently(sha1, 1)) {
 				enum object_type type = sha1_object_info(sha1, NULL);
-				die(_("%s: can't cherry-pick a %s"), name, typename(type));
+				return error(_("%s: can't cherry-pick a %s"),
+					name, typename(type));
 			}
 		} else
-			die(_("%s: bad revision"), name);
+			return error(_("%s: bad revision"), name);
 	}
 
 	/*
@@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	    !opts->revs->cmdline.rev->flags) {
 		struct commit *cmit;
 		if (prepare_revision_walk(opts->revs))
-			die(_("revision walk setup failed"));
+			return error(_("revision walk setup failed"));
 		cmit = get_revision(opts->revs);
 		if (!cmit || get_revision(opts->revs))
-			die("BUG: expected exactly one commit from walk");
+			return error("BUG: expected exactly one commit from walk");
 		return single_pick(cmit, opts);
 	}
 
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 02/14] sequencer: do not die() in do_pick_commit()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The eventual caller of do_pick_commit() is sequencer_pick_revisions(),
which already relays a reported error from its helper functions
(including this one), and both of its two callers know how to react to
a negative return correctly.

So this makes do_pick_commit() callable from new callers that want it
not to die, without changing the external behaviour of anything
existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 76b1c52..baf6b40 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
-	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
-		update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&
+	    update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
+		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		res = -1;
+	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
+	    update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
+		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		res = -1;
 
 	if (res) {
 		error(opts->action == REPLAY_REVERT
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 03/14] sequencer: lib'ify write_message()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of write_message(), do_pick_commit() already checks
the return value and passes it on to its callers, so its caller must
be already prepared to handle error returns, and with this step, we
make it notice an error return from this function.

So this is a safe conversion to make write_message() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index baf6b40..17bee60 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 	}
 }
 
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
 {
 	static struct lock_file msg_file;
 
-	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
-					       LOCK_DIE_ON_ERROR);
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
+	if (msg_fd < 0)
+		return error_errno(_("Could not lock '%s'"), filename);
 	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s"), filename);
+		return error_errno(_("Could not write to %s"), filename);
 	strbuf_release(msgbuf);
 	if (commit_lock_file(&msg_file) < 0)
-		die(_("Error wrapping up %s."), filename);
+		return error(_("Error wrapping up %s."), filename);
+
+	return 0;
 }
 
 static struct tree *empty_tree(void)
@@ -223,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
-		exit(128); /* the callee should have complained already */
+		return -1; /* the callee should have complained already */
 
 	strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts));
 
@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 					 head, &msgbuf, opts);
 		if (res < 0)
 			return res;
-		write_message(&msgbuf, git_path_merge_msg());
+		res |= write_message(&msgbuf, git_path_merge_msg());
 	} else {
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
 
-		write_message(&msgbuf, git_path_merge_msg());
+		res = write_message(&msgbuf, git_path_merge_msg());
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+		res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
 					common, sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH v2 03/14] sequencer: lib'ify write_message()

[Junio C Hamano]

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).
>
> The only caller of write_message(), do_pick_commit() already checks
> the return value and passes it on to its callers, so its caller must
> be already prepared to handle error returns, and with this step, we
> make it notice an error return from this function.
> ...
> @@ -223,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
>  
>  	read_cache();
>  	if (checkout_fast_forward(from, to, 1))
> -		exit(128); /* the callee should have complained already */
> +		return -1; /* the callee should have complained already */

This hunk does not seem to have anything to do with write_message()
conversion, other than that its only caller, do_pick_commit(), also
calls write_message().  checkout_fast_forward() itself can die when
it cannot write the index, though, so this hunk may deserve to be in
its own patch that teaches checkout_fast_forward() to instead return
an error if/when its caller desires.

With the updated message, the series has become far easier to review,
and the reordering them to sequencer-pick-revisions at the beginning
makes quite a lot of sense.

Re: [PATCH v2 03/14] sequencer: lib'ify write_message()

[Johannes Schindelin]

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).
> >
> > The only caller of write_message(), do_pick_commit() already checks
> > the return value and passes it on to its callers, so its caller must
> > be already prepared to handle error returns, and with this step, we
> > make it notice an error return from this function.
> > ...
> > @@ -223,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
> >  
> >  	read_cache();
> >  	if (checkout_fast_forward(from, to, 1))
> > -		exit(128); /* the callee should have complained already */
> > +		return -1; /* the callee should have complained already */
> 
> This hunk does not seem to have anything to do with write_message()
> conversion, other than that its only caller, do_pick_commit(), also
> calls write_message().

Darn. Sorry that this slipped through. I split it out into its own commit,
and fixed checkout_fast_forward_to() in yet another commit.

Ciao,
Dscho

[PATCH v2 04/14] sequencer: lib'ify do_recursive_merge()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of do_recursive_merge(), do_pick_commit() already
checks the return value and passes it on to its callers, so its caller
must be already prepared to handle error returns, and with this step,
we make it notice an error return from this function.

So this is a safe conversion to make do_recursive_merge() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 17bee60..f6cdf65 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), action_name(opts));
+		return error(_("%s: Unable to write new index file"),
+			action_name(opts));
 	rollback_lock_file(&index_lock);
 
 	if (opts->signoff)
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 05/14] sequencer: lib'ify do_pick_commit()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only two callers of do_pick_commit(), pick_commits() and
single_pick() already check the return value and pass it on to their
callers, so their callers must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make do_pick_commit() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.

While at it, remove the superfluous space.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index f6cdf65..7eef512 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		 * to work on.
 		 */
 		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
+			return error(_("Your index file is unmerged."));
 	} else {
 		unborn = get_sha1("HEAD", head);
 		if (unborn)
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH v2 05/14] sequencer: lib'ify do_pick_commit()

[Junio C Hamano]

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).
>
> The only two callers of do_pick_commit(), pick_commits() and
> single_pick() already check the return value and pass it on to their
> callers, so their callers must be already prepared to handle error
> returns, and with this step, we make it notice an error return from
> this function.
>
> So this is a safe conversion to make do_pick_commit() callable from
> new callers that want it not to die, without changing the external
> behaviour of anything existing.
>
> While at it, remove the superfluous space.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index f6cdf65..7eef512 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		 * to work on.
>  		 */
>  		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +			return error(_("Your index file is unmerged."));

Makes sense.

do_pick_commit() still calls fast_forward_to(), which was silently
half libified in 3/14 but can still die in checkout_fast_forward(),
though.

[PATCH v2 06/14] sequencer: lib'ify walk_revs_populate_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The function sequencer_pick_revisions() is the only caller of
walk_revs_populate_todo(), and it already returns errors
appropriately, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make walk_revs_populate_todo()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7eef512..ea2681e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), git_path_opts_file());
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static int walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct commit *commit;
 	struct commit_list **next;
 
-	prepare_revs(opts);
+	if (prepare_revs(opts))
+		return -1;
 
 	next = todo_list;
 	while ((commit = get_revision(opts->revs)))
 		next = commit_list_append(commit, next);
+	return 0;
 }
 
 static int create_seq_dir(void)
@@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	 * progress
 	 */
 
-	walk_revs_populate_todo(&todo_list, opts);
-	if (create_seq_dir() < 0)
+	if (walk_revs_populate_todo(&todo_list, opts) ||
+			create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 07/14] sequencer: lib'ify prepare_revs()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of prepare_revs(), walk_revs_populate_todo() was just
taught to return errors, after verifying that its callers are prepared
to handle error returns, and with this step, we make it notice an
error return from this function.

So this is a safe conversion to make prepare_revs() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ea2681e..c006cae 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -623,7 +623,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	return res;
 }
 
-static void prepare_revs(struct replay_opts *opts)
+static int prepare_revs(struct replay_opts *opts)
 {
 	/*
 	 * picking (but not reverting) ranges (but not individual revisions)
@@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts)
 		opts->revs->reverse ^= 1;
 
 	if (prepare_revision_walk(opts->revs))
-		die(_("revision walk setup failed"));
+		return error(_("revision walk setup failed"));
 
 	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
+		return error(_("empty commit set passed"));
+	return 0;
 }
 
 static void read_and_refresh_cache(struct replay_opts *opts)
-- 
2.10.0.rc1.99.gcd66998

[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 <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;
 }
 
 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

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

[Junio C Hamano]

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?

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

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

[Junio C Hamano]

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

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

We just fixed a similar "why are we allowing the 'if the_index
hasn't been read, read unconditionally from $GIT_INDEX_FILE" that is
reached by a codepath that is specifically designed to read from a
temporary index file while reviewing a separate topic, and that is
where my reaction "this is not very helpful for other callers" comes
from.

[PATCH v2 09/14] sequencer: lib'ify read_populate_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of read_populate_todo(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an
error return from this function.

So this is a safe conversion to make read_populate_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e30aa82..e11b24f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct commit_list **todo_list,
 			struct replay_opts *opts)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list **todo_list,
 
 	fd = open(git_path_todo_file(), O_RDONLY);
 	if (fd < 0)
-		die_errno(_("Could not open %s"), git_path_todo_file());
+		return error_errno(_("Could not open %s"),
+				   git_path_todo_file());
 	if (strbuf_read(&buf, fd, 0) < 0) {
 		close(fd);
 		strbuf_release(&buf);
-		die(_("Could not read %s."), git_path_todo_file());
+		return error(_("Could not read %s."), git_path_todo_file());
 	}
 	close(fd);
 
 	res = parse_insn_buffer(buf.buf, todo_list, opts);
 	strbuf_release(&buf);
 	if (res)
-		die(_("Unusable instruction sheet: %s"), git_path_todo_file());
+		return error(_("Unusable instruction sheet: %s"),
+			git_path_todo_file());
+	return 0;
 }
 
 static int populate_opts_cb(const char *key, const char *value, void *data)
@@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts)
 	if (!file_exists(git_path_todo_file()))
 		return continue_single_pick();
 	read_populate_opts(&opts);
-	read_populate_todo(&todo_list, opts);
+	if (read_populate_todo(&todo_list, opts))
+		return -1;
 
 	/* Verify that the conflict has been resolved */
 	if (file_exists(git_path_cherry_pick_head()) ||
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 10/14] sequencer: lib'ify read_populate_opts()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of read_populate_opts(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.

So this is a safe conversion to make read_populate_opts() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e11b24f..be6020a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -808,12 +808,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	return 0;
 }
 
-static void read_populate_opts(struct replay_opts **opts_ptr)
+static int read_populate_opts(struct replay_opts **opts)
 {
 	if (!file_exists(git_path_opts_file()))
-		return;
-	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
-		die(_("Malformed options sheet: %s"), git_path_opts_file());
+		return 0;
+	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
+		return error(_("Malformed options sheet: %s"),
+			git_path_opts_file());
+	return 0;
 }
 
 static int walk_revs_populate_todo(struct commit_list **todo_list,
@@ -1021,8 +1023,8 @@ static int sequencer_continue(struct replay_opts *opts)
 
 	if (!file_exists(git_path_todo_file()))
 		return continue_single_pick();
-	read_populate_opts(&opts);
-	if (read_populate_todo(&todo_list, opts))
+	if (read_populate_opts(&opts) ||
+			read_populate_todo(&todo_list, opts))
 		return -1;
 
 	/* Verify that the conflict has been resolved */
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()

[Junio C Hamano]

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).
>
> The only caller of read_populate_opts(), sequencer_continue() can
> already return errors, so its caller must be already prepared to
> handle error returns, and with this step, we make it notice an error
> return from this function.
>
> So this is a safe conversion to make read_populate_opts() callable
> from new callers that want it not to die, without changing the
> external behaviour of anything existing.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e11b24f..be6020a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -808,12 +808,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  	return 0;
>  }
>  
> -static void read_populate_opts(struct replay_opts **opts_ptr)
> +static int read_populate_opts(struct replay_opts **opts)
>  {
>  	if (!file_exists(git_path_opts_file()))
> -		return;
> -	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
> -		die(_("Malformed options sheet: %s"), git_path_opts_file());
> +		return 0;
> +	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
> +		return error(_("Malformed options sheet: %s"),
> +			git_path_opts_file());
> +	return 0;

As discussed, perhaps have a comment immediately before calling
config-from-file that says that the call could die when it is fed a
syntactically broken file, but we ignore it for now because we will
be writing the file we have written, or something?

>  }
>  
>  static int walk_revs_populate_todo(struct commit_list **todo_list,
> @@ -1021,8 +1023,8 @@ static int sequencer_continue(struct replay_opts *opts)
>  
>  	if (!file_exists(git_path_todo_file()))
>  		return continue_single_pick();
> -	read_populate_opts(&opts);
> -	if (read_populate_todo(&todo_list, opts))
> +	if (read_populate_opts(&opts) ||
> +			read_populate_todo(&todo_list, opts))
>  		return -1;
>  
>  	/* Verify that the conflict has been resolved */

Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
>> +		return error(_("Malformed options sheet: %s"),
>> +			git_path_opts_file());
>> +	return 0;
>
> As discussed, perhaps have a comment immediately before calling
> config-from-file that says that the call could die when it is fed a
> syntactically broken file, but we ignore it for now because we will
> be writing the file we have written, or something?

Obviously we will be _READING_ here ;-)

Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()

[Johannes Schindelin]

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).
> >
> > The only caller of read_populate_opts(), sequencer_continue() can
> > already return errors, so its caller must be already prepared to
> > handle error returns, and with this step, we make it notice an error
> > return from this function.
> >
> > So this is a safe conversion to make read_populate_opts() callable
> > from new callers that want it not to die, without changing the
> > external behaviour of anything existing.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index e11b24f..be6020a 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -808,12 +808,14 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
> >  	return 0;
> >  }
> >  
> > -static void read_populate_opts(struct replay_opts **opts_ptr)
> > +static int read_populate_opts(struct replay_opts **opts)
> >  {
> >  	if (!file_exists(git_path_opts_file()))
> > -		return;
> > -	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
> > -		die(_("Malformed options sheet: %s"), git_path_opts_file());
> > +		return 0;
> > +	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
> > +		return error(_("Malformed options sheet: %s"),
> > +			git_path_opts_file());
> > +	return 0;
> 
> As discussed, perhaps have a comment immediately before calling
> config-from-file that says that the call could die when it is fed a
> syntactically broken file, but we ignore it for now because we will
> be writing the file we have written, or something?

Sure. I added a code comment.

I still think that it is a serious mistake for library functions to die().
But I have no time to take care of git_parse_source() right now.

Ciao,
Dscho

Re: [PATCH v2 10/14] sequencer: lib'ify read_populate_opts()

[Junio C Hamano]

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

> I still think that it is a serious mistake for library functions to die().
> But I have no time to take care of git_parse_source() right now.

I think we already agreed on both points during the previous round
of review ;-)

[PATCH v2 11/14] sequencer: lib'ify create_seq_dir()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of create_seq_dir(), sequencer_pick_revisions() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.

So this is a safe conversion to make create_seq_dir() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index be6020a..9a1f0af 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -841,8 +841,8 @@ static int create_seq_dir(void)
 		return -1;
 	}
 	else if (mkdir(git_path_seq_dir(), 0777) < 0)
-		die_errno(_("Could not create sequencer directory %s"),
-			  git_path_seq_dir());
+		return error_errno(_("Could not create sequencer directory %s"),
+				   git_path_seq_dir());
 	return 0;
 }
 
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 12/14] sequencer: lib'ify save_head()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_head(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_head() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

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 9a1f0af..a819919 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -846,18 +846,22 @@ static int create_seq_dir(void)
 	return 0;
 }
 
-static void save_head(const char *head)
+static int save_head(const char *head)
 {
 	static struct lock_file head_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
+	if (fd < 0)
+		return error_errno(_("Could not lock HEAD"));
 	strbuf_addf(&buf, "%s\n", head);
 	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s"), git_path_head_file());
+		return error_errno(_("Could not write to %s"),
+				   git_path_head_file());
 	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), git_path_head_file());
+		return error(_("Error wrapping up %s."), git_path_head_file());
+	return 0;
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
@@ -1121,7 +1125,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-	save_head(sha1_to_hex(sha1));
+	if (save_head(sha1_to_hex(sha1)))
+		return -1;
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH v2 12/14] sequencer: lib'ify save_head()

[Junio C Hamano]

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

>  	strbuf_addf(&buf, "%s\n", head);
>  	if (write_in_full(fd, buf.buf, buf.len) < 0)
> -		die_errno(_("Could not write to %s"), git_path_head_file());
> +		return error_errno(_("Could not write to %s"),
> +				   git_path_head_file());

Same comment around a left-over lockfile applies to this.  An extra
rollback being minimally intrusive also applies here, I think.

>  	if (commit_lock_file(&head_lock) < 0)
> -		die(_("Error wrapping up %s."), git_path_head_file());
> +		return error(_("Error wrapping up %s."), git_path_head_file());
> +	return 0;
>  }

Re: [PATCH v2 12/14] sequencer: lib'ify save_head()

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> >  	strbuf_addf(&buf, "%s\n", head);
> >  	if (write_in_full(fd, buf.buf, buf.len) < 0)
> > -		die_errno(_("Could not write to %s"), git_path_head_file());
> > +		return error_errno(_("Could not write to %s"),
> > +				   git_path_head_file());
> 
> Same comment around a left-over lockfile applies to this.  An extra
> rollback being minimally intrusive also applies here, I think.

Sure. I added rollbacks in case of failure.

Ciao,
Dscho

[PATCH v2 13/14] sequencer: lib'ify save_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_todo(), pick_commits() can already return
errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a819919..3e877f1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -931,24 +931,31 @@ static int sequencer_rollback(struct replay_opts *opts)
 	return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 {
 	static struct lock_file todo_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
-		die(_("Could not format %s."), git_path_todo_file());
+	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
+	if (fd < 0)
+		return error_errno(_("Could not lock '%s'"),
+				   git_path_todo_file());
+	if (format_todo(&buf, todo_list, opts) < 0) {
+		strbuf_release(&buf);
+		return error(_("Could not format %s."), git_path_todo_file());
+	}
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
-		die_errno(_("Could not write to %s"), git_path_todo_file());
+		return error_errno(_("Could not write to %s"),
+				   git_path_todo_file());
 	}
 	if (commit_lock_file(&todo_lock) < 0) {
 		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), git_path_todo_file());
+		return error(_("Error wrapping up %s."), git_path_todo_file());
 	}
 	strbuf_release(&buf);
+	return 0;
 }
 
 static void save_opts(struct replay_opts *opts)
@@ -997,7 +1004,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 		return -1;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
+		if (save_todo(cur, opts))
+			return -1;
 		res = do_pick_commit(cur->item, opts);
 		if (res)
 			return res;
-- 
2.10.0.rc1.99.gcd66998

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

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_opts(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_opts() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3e877f1..b6481bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -958,37 +958,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	return 0;
 }
 
-static void save_opts(struct replay_opts *opts)
+static int save_opts(struct replay_opts *opts)
 {
 	const char *opts_file = git_path_opts_file();
+	int res = 0;
 
 	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.no-commit", "true");
 	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.edit", "true");
 	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true");
 	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true");
 	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true");
 	if (opts->mainline) {
 		struct strbuf buf = STRBUF_INIT;
 		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+		res |= git_config_set_in_file_gently(opts_file, "options.mainline", buf.buf);
 		strbuf_release(&buf);
 	}
 	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+		res |= git_config_set_in_file_gently(opts_file, "options.strategy", opts->strategy);
 	if (opts->gpg_sign)
-		git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign);
+		res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign);
 	if (opts->xopts) {
 		int i;
 		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
+			res |= git_config_set_multivar_in_file_gently(opts_file,
 							"options.strategy-option",
 							opts->xopts[i], "^$", 0);
 	}
+	return res;
 }
 
 static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
@@ -1133,9 +1135,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-	if (save_head(sha1_to_hex(sha1)))
+	if (save_head(sha1_to_hex(sha1)) ||
+			save_opts(opts))
 		return -1;
-	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
 
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH v2 00/14] Lib'ify quite a few functions in sequencer.c

[Junio C Hamano]

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

> This patch series is one of the half dozen patch series left to move the
> bulk of rebase -i into a builtin.

This was a lot easier to understand compared to the previous round,
and overall looked alright.

Thanks.

[PATCH v3 00/17] Lib'ify quite a few functions in sequencer.c

[Johannes Schindelin]

This patch series is one of the half dozen patch series left to move the
bulk of rebase -i into a builtin.

The purpose of this patch series is to switch the functions in
sequencer.c from die()ing to returning errors instead, as proper library
functions should do, to give callers a chance to clean up after an
error.

Changes since v2:

- the commit message of the read_populate_opts() patch now clarifies why
  we do not take care of git_config_from_file() possibly die()ing.

- the save_head() and the save_opts() conditionals are now separate, for
  improved readability.

- the fast_forward_to() libification now happens in its own commit.

- checkout_fast_forward_to() is now libified, too.

- added a code comment to clarify why we don't care about
  git_parse_source() being able to die().

- added rollbacks in case of failure in read_and_refresh_cache() and
  save_head().


Johannes Schindelin (17):
  sequencer: lib'ify sequencer_pick_revisions()
  sequencer: do not die() in do_pick_commit()
  sequencer: lib'ify write_message()
  sequencer: lib'ify do_recursive_merge()
  sequencer: lib'ify do_pick_commit()
  sequencer: lib'ify walk_revs_populate_todo()
  sequencer: lib'ify prepare_revs()
  sequencer: lib'ify read_and_refresh_cache()
  sequencer: lib'ify read_populate_todo()
  sequencer: lib'ify read_populate_opts()
  sequencer: lib'ify create_seq_dir()
  sequencer: lib'ify save_head()
  sequencer: lib'ify save_todo()
  sequencer: lib'ify save_opts()
  sequencer: lib'ify fast_forward_to()
  lib'ify checkout_fast_forward_to()
  sequencer: ensure to release the lock when we could not read the index

 merge.c     |   9 ++-
 sequencer.c | 197 ++++++++++++++++++++++++++++++++++++++----------------------
 2 files changed, 131 insertions(+), 75 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v3
Fetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v3

Interdiff vs v2:

 diff --git a/merge.c b/merge.c
 index 5db7d56..23866c9 100644
 --- a/merge.c
 +++ b/merge.c
 @@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,
  
  	refresh_cache(REFRESH_QUIET);
  
 -	hold_locked_index(lock_file, 1);
 +	if (hold_locked_index(lock_file, 0) < 0)
 +		return -1;
  
  	memset(&trees, 0, sizeof(trees));
  	memset(&opts, 0, sizeof(opts));
 @@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,
  	}
  	if (unpack_trees(nr_trees, t, &opts))
  		return -1;
 -	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 -		die(_("unable to write new index file"));
 +	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {
 +		rollback_lock_file(lock_file);
 +		return error(_("unable to write new index file"));
 +	}
  	return 0;
  }
 diff --git a/sequencer.c b/sequencer.c
 index b6481bb..eec8a60 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -644,14 +644,18 @@ 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)
 +	if (read_index_preload(&the_index, NULL) < 0) {
 +		rollback_lock_file(&index_lock);
  		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))
 +		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
 +			rollback_lock_file(&index_lock);
  			return error(_("git %s: failed to refresh the index"),
  				action_name(opts));
 +		}
  	}
  	rollback_lock_file(&index_lock);
  	return 0;
 @@ -812,6 +816,12 @@ static int read_populate_opts(struct replay_opts **opts)
  {
  	if (!file_exists(git_path_opts_file()))
  		return 0;
 +	/*
 +	 * The function git_parse_source(), called from git_config_from_file(),
 +	 * may die() in case of a syntactically incorrect file. We do not care
 +	 * about this case, though, because we wrote that file ourselves, so we
 +	 * are pretty certain that it is syntactically correct.
 +	 */
  	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
  		return error(_("Malformed options sheet: %s"),
  			git_path_opts_file());
 @@ -853,14 +863,20 @@ static int save_head(const char *head)
  	int fd;
  
  	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
 -	if (fd < 0)
 +	if (fd < 0) {
 +		rollback_lock_file(&head_lock);
  		return error_errno(_("Could not lock HEAD"));
 +	}
  	strbuf_addf(&buf, "%s\n", head);
 -	if (write_in_full(fd, buf.buf, buf.len) < 0)
 +	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 +		rollback_lock_file(&head_lock);
  		return error_errno(_("Could not write to %s"),
  				   git_path_head_file());
 -	if (commit_lock_file(&head_lock) < 0)
 +	}
 +	if (commit_lock_file(&head_lock) < 0) {
 +		rollback_lock_file(&head_lock);
  		return error(_("Error wrapping up %s."), git_path_head_file());
 +	}
  	return 0;
  }
  
 @@ -1135,8 +1151,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
  		return -1;
  	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
  		return error(_("Can't revert as initial commit"));
 -	if (save_head(sha1_to_hex(sha1)) ||
 -			save_opts(opts))
 +	if (save_head(sha1_to_hex(sha1)))
 +		return -1;
 +	if (save_opts(opts))
  		return -1;
  	return pick_commits(todo_list, opts);
  }

-- 
2.10.0.windows.1.10.g803177d

base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b

[PATCH v3 01/17] sequencer: lib'ify sequencer_pick_revisions()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The function sequencer_pick_revisions() has only two callers,
cmd_revert() and cmd_cherry_pick(), both of which check the return
value and react appropriately upon errors.

So this is a safe conversion to make sequencer_pick_revisions()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3804fa9..76b1c52 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		if (!get_sha1(name, sha1)) {
 			if (!lookup_commit_reference_gently(sha1, 1)) {
 				enum object_type type = sha1_object_info(sha1, NULL);
-				die(_("%s: can't cherry-pick a %s"), name, typename(type));
+				return error(_("%s: can't cherry-pick a %s"),
+					name, typename(type));
 			}
 		} else
-			die(_("%s: bad revision"), name);
+			return error(_("%s: bad revision"), name);
 	}
 
 	/*
@@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	    !opts->revs->cmdline.rev->flags) {
 		struct commit *cmit;
 		if (prepare_revision_walk(opts->revs))
-			die(_("revision walk setup failed"));
+			return error(_("revision walk setup failed"));
 		cmit = get_revision(opts->revs);
 		if (!cmit || get_revision(opts->revs))
-			die("BUG: expected exactly one commit from walk");
+			return error("BUG: expected exactly one commit from walk");
 		return single_pick(cmit, opts);
 	}
 
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 02/17] sequencer: do not die() in do_pick_commit()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The eventual caller of do_pick_commit() is sequencer_pick_revisions(),
which already relays a reported error from its helper functions
(including this one), and both of its two callers know how to react to
a negative return correctly.

So this makes do_pick_commit() callable from new callers that want it
not to die, without changing the external behaviour of anything
existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 76b1c52..baf6b40 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
-	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
-		update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&
+	    update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
+		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		res = -1;
+	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
+	    update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
+		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+		res = -1;
 
 	if (res) {
 		error(opts->action == REPLAY_REVERT
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 03/17] sequencer: lib'ify write_message()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of write_message(), do_pick_commit() already checks
the return value and passes it on to its callers, so its caller must
be already prepared to handle error returns, and with this step, we
make it notice an error return from this function.

So this is a safe conversion to make write_message() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index baf6b40..ec85fe7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 	}
 }
 
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
 {
 	static struct lock_file msg_file;
 
-	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
-					       LOCK_DIE_ON_ERROR);
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
+	if (msg_fd < 0)
+		return error_errno(_("Could not lock '%s'"), filename);
 	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s"), filename);
+		return error_errno(_("Could not write to %s"), filename);
 	strbuf_release(msgbuf);
 	if (commit_lock_file(&msg_file) < 0)
-		die(_("Error wrapping up %s."), filename);
+		return error(_("Error wrapping up %s."), filename);
+
+	return 0;
 }
 
 static struct tree *empty_tree(void)
@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 					 head, &msgbuf, opts);
 		if (res < 0)
 			return res;
-		write_message(&msgbuf, git_path_merge_msg());
+		res |= write_message(&msgbuf, git_path_merge_msg());
 	} else {
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
 
-		write_message(&msgbuf, git_path_merge_msg());
+		res = write_message(&msgbuf, git_path_merge_msg());
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+		res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
 					common, sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 04/17] sequencer: lib'ify do_recursive_merge()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of do_recursive_merge(), do_pick_commit() already
checks the return value and passes it on to its callers, so its caller
must be already prepared to handle error returns, and with this step,
we make it notice an error return from this function.

So this is a safe conversion to make do_recursive_merge() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index ec85fe7..eb70091 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), action_name(opts));
+		return error(_("%s: Unable to write new index file"),
+			action_name(opts));
 	rollback_lock_file(&index_lock);
 
 	if (opts->signoff)
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 05/17] sequencer: lib'ify do_pick_commit()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only two callers of do_pick_commit(), pick_commits() and
single_pick() already check the return value and pass it on to their
callers, so their callers must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make do_pick_commit() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.

While at it, remove the superfluous space.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index eb70091..96b9ae1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		 * to work on.
 		 */
 		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
+			return error(_("Your index file is unmerged."));
 	} else {
 		unborn = get_sha1("HEAD", head);
 		if (unborn)
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 06/17] sequencer: lib'ify walk_revs_populate_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The function sequencer_pick_revisions() is the only caller of
walk_revs_populate_todo(), and it already returns errors
appropriately, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make walk_revs_populate_todo()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 96b9ae1..ab599e0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), git_path_opts_file());
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static int walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct commit *commit;
 	struct commit_list **next;
 
-	prepare_revs(opts);
+	if (prepare_revs(opts))
+		return -1;
 
 	next = todo_list;
 	while ((commit = get_revision(opts->revs)))
 		next = commit_list_append(commit, next);
+	return 0;
 }
 
 static int create_seq_dir(void)
@@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	 * progress
 	 */
 
-	walk_revs_populate_todo(&todo_list, opts);
-	if (create_seq_dir() < 0)
+	if (walk_revs_populate_todo(&todo_list, opts) ||
+			create_seq_dir() < 0)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 07/17] sequencer: lib'ify prepare_revs()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of prepare_revs(), walk_revs_populate_todo() was just
taught to return errors, after verifying that its callers are prepared
to handle error returns, and with this step, we make it notice an
error return from this function.

So this is a safe conversion to make prepare_revs() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ab599e0..7fd0f99 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -623,7 +623,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	return res;
 }
 
-static void prepare_revs(struct replay_opts *opts)
+static int prepare_revs(struct replay_opts *opts)
 {
 	/*
 	 * picking (but not reverting) ranges (but not individual revisions)
@@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts)
 		opts->revs->reverse ^= 1;
 
 	if (prepare_revision_walk(opts->revs))
-		die(_("revision walk setup failed"));
+		return error(_("revision walk setup failed"));
 
 	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
+		return error(_("empty commit set passed"));
+	return 0;
 }
 
 static void read_and_refresh_cache(struct replay_opts *opts)
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 08/17] 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 <johannes.schindelin@gmx.de>
---
 sequencer.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7fd0f99..631b75d 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;
 }
 
 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.windows.1.10.g803177d

[PATCH v3 09/17] sequencer: lib'ify read_populate_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of read_populate_todo(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an
error return from this function.

So this is a safe conversion to make read_populate_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 631b75d..c73cdfd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 	return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct commit_list **todo_list,
 			struct replay_opts *opts)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list **todo_list,
 
 	fd = open(git_path_todo_file(), O_RDONLY);
 	if (fd < 0)
-		die_errno(_("Could not open %s"), git_path_todo_file());
+		return error_errno(_("Could not open %s"),
+				   git_path_todo_file());
 	if (strbuf_read(&buf, fd, 0) < 0) {
 		close(fd);
 		strbuf_release(&buf);
-		die(_("Could not read %s."), git_path_todo_file());
+		return error(_("Could not read %s."), git_path_todo_file());
 	}
 	close(fd);
 
 	res = parse_insn_buffer(buf.buf, todo_list, opts);
 	strbuf_release(&buf);
 	if (res)
-		die(_("Unusable instruction sheet: %s"), git_path_todo_file());
+		return error(_("Unusable instruction sheet: %s"),
+			git_path_todo_file());
+	return 0;
 }
 
 static int populate_opts_cb(const char *key, const char *value, void *data)
@@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts)
 	if (!file_exists(git_path_todo_file()))
 		return continue_single_pick();
 	read_populate_opts(&opts);
-	read_populate_todo(&todo_list, opts);
+	if (read_populate_todo(&todo_list, opts))
+		return -1;
 
 	/* Verify that the conflict has been resolved */
 	if (file_exists(git_path_cherry_pick_head()) ||
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 10/17] sequencer: lib'ify read_populate_opts()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of read_populate_opts(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.

So this is a safe conversion to make read_populate_opts() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Note that the function git_config_from_file(), called from
read_populate_opts(), can currently still die() (in git_parse_source(),
because the do_config_from_file() function sets die_on_error = 1). We do
not try to fix that here, as it would have larger ramifications on the
config code, and we also assume that we write the opts file
programmatically, hence any parse errors would be bugs.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c73cdfd..1614efb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -808,12 +808,20 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	return 0;
 }
 
-static void read_populate_opts(struct replay_opts **opts_ptr)
+static int read_populate_opts(struct replay_opts **opts)
 {
 	if (!file_exists(git_path_opts_file()))
-		return;
-	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
-		die(_("Malformed options sheet: %s"), git_path_opts_file());
+		return 0;
+	/*
+	 * The function git_parse_source(), called from git_config_from_file(),
+	 * may die() in case of a syntactically incorrect file. We do not care
+	 * about this case, though, because we wrote that file ourselves, so we
+	 * are pretty certain that it is syntactically correct.
+	 */
+	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
+		return error(_("Malformed options sheet: %s"),
+			git_path_opts_file());
+	return 0;
 }
 
 static int walk_revs_populate_todo(struct commit_list **todo_list,
@@ -1021,8 +1029,8 @@ static int sequencer_continue(struct replay_opts *opts)
 
 	if (!file_exists(git_path_todo_file()))
 		return continue_single_pick();
-	read_populate_opts(&opts);
-	if (read_populate_todo(&todo_list, opts))
+	if (read_populate_opts(&opts) ||
+			read_populate_todo(&todo_list, opts))
 		return -1;
 
 	/* Verify that the conflict has been resolved */
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 11/17] sequencer: lib'ify create_seq_dir()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of create_seq_dir(), sequencer_pick_revisions() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.

So this is a safe conversion to make create_seq_dir() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1614efb..eb9c473 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -847,8 +847,8 @@ static int create_seq_dir(void)
 		return -1;
 	}
 	else if (mkdir(git_path_seq_dir(), 0777) < 0)
-		die_errno(_("Could not create sequencer directory %s"),
-			  git_path_seq_dir());
+		return error_errno(_("Could not create sequencer directory %s"),
+				   git_path_seq_dir());
 	return 0;
 }
 
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 12/17] sequencer: lib'ify save_head()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_head(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_head() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index eb9c473..7a1561e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -852,18 +852,28 @@ static int create_seq_dir(void)
 	return 0;
 }
 
-static void save_head(const char *head)
+static int save_head(const char *head)
 {
 	static struct lock_file head_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
+	if (fd < 0) {
+		rollback_lock_file(&head_lock);
+		return error_errno(_("Could not lock HEAD"));
+	}
 	strbuf_addf(&buf, "%s\n", head);
-	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s"), git_path_head_file());
-	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), git_path_head_file());
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		rollback_lock_file(&head_lock);
+		return error_errno(_("Could not write to %s"),
+				   git_path_head_file());
+	}
+	if (commit_lock_file(&head_lock) < 0) {
+		rollback_lock_file(&head_lock);
+		return error(_("Error wrapping up %s."), git_path_head_file());
+	}
+	return 0;
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
@@ -1127,7 +1137,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
 		return error(_("Can't revert as initial commit"));
-	save_head(sha1_to_hex(sha1));
+	if (save_head(sha1_to_hex(sha1)))
+		return -1;
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 13/17] sequencer: lib'ify save_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_todo(), pick_commits() can already return
errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7a1561e..32c53bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -943,24 +943,31 @@ static int sequencer_rollback(struct replay_opts *opts)
 	return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 {
 	static struct lock_file todo_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
-		die(_("Could not format %s."), git_path_todo_file());
+	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
+	if (fd < 0)
+		return error_errno(_("Could not lock '%s'"),
+				   git_path_todo_file());
+	if (format_todo(&buf, todo_list, opts) < 0) {
+		strbuf_release(&buf);
+		return error(_("Could not format %s."), git_path_todo_file());
+	}
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
-		die_errno(_("Could not write to %s"), git_path_todo_file());
+		return error_errno(_("Could not write to %s"),
+				   git_path_todo_file());
 	}
 	if (commit_lock_file(&todo_lock) < 0) {
 		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), git_path_todo_file());
+		return error(_("Error wrapping up %s."), git_path_todo_file());
 	}
 	strbuf_release(&buf);
+	return 0;
 }
 
 static void save_opts(struct replay_opts *opts)
@@ -1009,7 +1016,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 		return -1;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
+		if (save_todo(cur, opts))
+			return -1;
 		res = do_pick_commit(cur->item, opts);
 		if (res)
 			return res;
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 14/17] sequencer: lib'ify save_opts()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_opts(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_opts() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 32c53bb..021ddf3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -970,37 +970,39 @@ static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	return 0;
 }
 
-static void save_opts(struct replay_opts *opts)
+static int save_opts(struct replay_opts *opts)
 {
 	const char *opts_file = git_path_opts_file();
+	int res = 0;
 
 	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.no-commit", "true");
 	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.edit", "true");
 	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true");
 	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true");
 	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
+		res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true");
 	if (opts->mainline) {
 		struct strbuf buf = STRBUF_INIT;
 		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+		res |= git_config_set_in_file_gently(opts_file, "options.mainline", buf.buf);
 		strbuf_release(&buf);
 	}
 	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+		res |= git_config_set_in_file_gently(opts_file, "options.strategy", opts->strategy);
 	if (opts->gpg_sign)
-		git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign);
+		res |= git_config_set_in_file_gently(opts_file, "options.gpg-sign", opts->gpg_sign);
 	if (opts->xopts) {
 		int i;
 		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
+			res |= git_config_set_multivar_in_file_gently(opts_file,
 							"options.strategy-option",
 							opts->xopts[i], "^$", 0);
 	}
+	return res;
 }
 
 static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
@@ -1147,7 +1149,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return error(_("Can't revert as initial commit"));
 	if (save_head(sha1_to_hex(sha1)))
 		return -1;
-	save_opts(opts);
+	if (save_opts(opts))
+		return -1;
 	return pick_commits(todo_list, opts);
 }
 
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 15/17] sequencer: lib'ify fast_forward_to()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of fast_forward_to(), do_pick_commit() already checks
the return value and passes it on to its callers, so its caller must
be already prepared to handle error returns, and with this step, we
make it notice an error return from this function.

So this is a safe conversion to make fast_forward_to() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 021ddf3..d92a632 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -226,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
-		exit(128); /* the callee should have complained already */
+		return -1; /* the callee should have complained already */
 
 	strbuf_addf(&sb, _("%s: fast-forward"), action_name(opts));
 
-- 
2.10.0.windows.1.10.g803177d

[PATCH v3 16/17] lib'ify checkout_fast_forward_to()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only callers of checkout_fast_forward_to(), cmd_merge(),
pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),
already check the return value and handle it appropriately. With this
step, we make it notice an error return from this function.

So this is a safe conversion to make checkout_fast_forward_to()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 merge.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/merge.c b/merge.c
index 5db7d56..23866c9 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,
 
 	refresh_cache(REFRESH_QUIET);
 
-	hold_locked_index(lock_file, 1);
+	if (hold_locked_index(lock_file, 0) < 0)
+		return -1;
 
 	memset(&trees, 0, sizeof(trees));
 	memset(&opts, 0, sizeof(opts));
@@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,
 	}
 	if (unpack_trees(nr_trees, t, &opts))
 		return -1;
-	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
-		die(_("unable to write new index file"));
+	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {
+		rollback_lock_file(lock_file);
+		return error(_("unable to write new index file"));
+	}
 	return 0;
 }
-- 
2.10.0.windows.1.10.g803177d

Re: [PATCH v3 16/17] lib'ify checkout_fast_forward_to()

[Junio C Hamano]

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).
>
> The only callers of checkout_fast_forward_to(), cmd_merge(),
> pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),
> already check the return value and handle it appropriately. With this
> step, we make it notice an error return from this function.
>
> So this is a safe conversion to make checkout_fast_forward_to()
> callable from new callers that want it not to die, without changing
> the external behaviour of anything existing.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

I'll retitle this to

    sequencer: lib'ify chckout_fast_forward()

and checkout_fast_forward_to() in the second paragraph to match the
reality.  Other than that, the above analysis matches what I see in
the code and the libification done here looks correct.

Thanks.

>  merge.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/merge.c b/merge.c
> index 5db7d56..23866c9 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,
>  
>  	refresh_cache(REFRESH_QUIET);
>  
> -	hold_locked_index(lock_file, 1);
> +	if (hold_locked_index(lock_file, 0) < 0)
> +		return -1;
>  
>  	memset(&trees, 0, sizeof(trees));
>  	memset(&opts, 0, sizeof(opts));
> @@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,
>  	}
>  	if (unpack_trees(nr_trees, t, &opts))
>  		return -1;
> -	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
> -		die(_("unable to write new index file"));
> +	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {
> +		rollback_lock_file(lock_file);
> +		return error(_("unable to write new index file"));
> +	}
>  	return 0;
>  }

Re: [PATCH v3 16/17] lib'ify checkout_fast_forward_to()

[Johannes Schindelin]

Hi Junio,

On Fri, 9 Sep 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).
> >
> > The only callers of checkout_fast_forward_to(), cmd_merge(),
> > pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),
> > already check the return value and handle it appropriately. With this
> > step, we make it notice an error return from this function.
> >
> > So this is a safe conversion to make checkout_fast_forward_to()
> > callable from new callers that want it not to die, without changing
> > the external behaviour of anything existing.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> I'll retitle this to
> 
>     sequencer: lib'ify chckout_fast_forward()
> 
> and checkout_fast_forward_to() in the second paragraph to match the
> reality.  Other than that, the above analysis matches what I see in
> the code and the libification done here looks correct.

Good catch. Please also fix it in the third paragraph. (I changed it also
locally, in case a v4 is required.)

Ciao,
Dscho

[PATCH v3 17/17] sequencer: ensure to release the lock when we could not read the index

[Johannes Schindelin]

A future caller of read_and_refresh_cache() may want to do more than just
print some helpful advice in case of failure.

Suggested by Junio Hamano.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d92a632..eec8a60 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -644,14 +644,18 @@ 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)
+	if (read_index_preload(&the_index, NULL) < 0) {
+		rollback_lock_file(&index_lock);
 		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))
+		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
+			rollback_lock_file(&index_lock);
 			return error(_("git %s: failed to refresh the index"),
 				action_name(opts));
+		}
 	}
 	rollback_lock_file(&index_lock);
 	return 0;
-- 
2.10.0.windows.1.10.g803177d

Re: [PATCH v3 17/17] sequencer: ensure to release the lock when we could not read the index

[Junio C Hamano]

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

> A future caller of read_and_refresh_cache() may want to do more than just
> print some helpful advice in case of failure.

I recall commenting on unreleased locks on other parts of the series
but didn't see this.  Looks good.  Thanks for being thorough.

> diff --git a/sequencer.c b/sequencer.c
> index d92a632..eec8a60 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -644,14 +644,18 @@ 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)
> +	if (read_index_preload(&the_index, NULL) < 0) {
> +		rollback_lock_file(&index_lock);
>  		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))
> +		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
> +			rollback_lock_file(&index_lock);
>  			return error(_("git %s: failed to refresh the index"),
>  				action_name(opts));
> +		}
>  	}
>  	rollback_lock_file(&index_lock);
>  	return 0;