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

2 messages, 2 authors, 2016-08-26 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-08-25 22:53:37

Johannes Schindelin [off-list ref] writes:
To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <redacted>
---
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.
quoted hunk
 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()

From: Johannes Schindelin <hidden>
Date: 2016-08-26 13:41:07

Hi Junio,

On Thu, 25 Aug 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
To be truly useful, the sequencer should never die() but always return
an error.

Signed-off-by: Johannes Schindelin <redacted>
---
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help