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

Junio C Hamano  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()

[Junio C Hamano]

Johannes Schindelin  writes:

> Instead of dying there, let the caller high up in the callchain notice
> the error and handle it (by dying, still).
>
> 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 
> ---
>  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();
> - if (read_populate_todo(_list, opts))
> + if (read_populate_opts() ||
> + read_populate_todo(_list, opts))
>   return -1;
>  
>   /* Verify that the conflict has been resolved */

[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 
---
 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();
-   if (read_populate_todo(_list, opts))
+   if (read_populate_opts() ||
+   read_populate_todo(_list, opts))
return -1;
 
/* Verify that the conflict has been resolved */
-- 
2.10.0.rc1.99.gcd66998


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