[2/3] add read_author_script() to libgit

Commit Message

Phillip Wood Sept. 12, 2018, 10:10 a.m. UTC

From: Phillip Wood <[e-mail]>

Add read_author_script() to sequencer.c based on the implementation in
builtin/am.c and update read_am_author_script() to use
read_author_script(). The sequencer code that reads the author script
will be updated in the next commit.

Signed-off-by: Phillip Wood <[e-mail]>
---
 builtin/am.c | 57 ++++-------------------------------------------
 sequencer.c  | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 sequencer.h  |  3 +++
 3 files changed, 69 insertions(+), 53 deletions(-)

Comments

Eric Sunshine Sept. 13, 2018, 11:49 p.m. UTC | #1

On Wed, Sep 12, 2018 at 6:11 AM Phillip Wood <[e-mail]> wrote:
> Add read_author_script() to sequencer.c based on the implementation in
> builtin/am.c and update read_am_author_script() to use
> read_author_script(). The sequencer code that reads the author script
> will be updated in the next commit.
>
> Signed-off-by: Phillip Wood <[e-mail]>
> ---
> diff --git a/builtin/am.c b/builtin/am.c
> @@ -305,39 +279,16 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)
>  static int read_am_author_script(struct am_state *state)
>  {

This function returns 'int'...

>         const char *filename = am_path(state, "author-script");
> +       if (read_author_script(filename, &state->author_name,
> +                              &state->author_email, &state->author_date, 1))
> +               exit(128);

...but only ever exit()s...

> +       return 0;

... or returns 0.

>  }

It's a little disturbing that it now invokes exit() directly rather
than calling die() since die() may involve extra functionality before
actually exiting.

What if, instead of exit()ing directly, you drop the conditional and
instead return the value of read_author_script():

    return read_author_script(...);

and let the caller deal with the zero or non-zero return value as
usual? (True, you'd get two error messages upon failure rather than
just one, but that's not necessarily a bad thing.)

A possibly valid response is that such a change is outside the scope
of this series since the original code shared this odd architecture of
sometimes returning 0, sometimes -1, and sometimes die()ing.

Phillip Wood Oct. 10, 2018, 10:14 a.m. UTC | #2

Hi Eric

Thanks for looking at this series, sorry it has taken so long for me to
reply.

On 14/09/2018 00:49, Eric Sunshine wrote:
> On Wed, Sep 12, 2018 at 6:11 AM Phillip Wood <[e-mail]> wrote:
>> Add read_author_script() to sequencer.c based on the implementation in
>> builtin/am.c and update read_am_author_script() to use
>> read_author_script(). The sequencer code that reads the author script
>> will be updated in the next commit.
>>
>> Signed-off-by: Phillip Wood <[e-mail]>
>> ---
>> diff --git a/builtin/am.c b/builtin/am.c
>> @@ -305,39 +279,16 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)
>>  static int read_am_author_script(struct am_state *state)
>>  {
> 
> This function returns 'int'...
> 
>>         const char *filename = am_path(state, "author-script");
>> +       if (read_author_script(filename, &state->author_name,
>> +                              &state->author_email, &state->author_date, 1))
>> +               exit(128);
> 
> ...but only ever exit()s...
> 
>> +       return 0;
> 
> ... or returns 0.
> 
>>  }
> 
> It's a little disturbing that it now invokes exit() directly rather
> than calling die() since die() may involve extra functionality before
> actually exiting.
> 
> What if, instead of exit()ing directly, you drop the conditional and
> instead return the value of read_author_script():
> 
>     return read_author_script(...);
> 
> and let the caller deal with the zero or non-zero return value as
> usual? (True, you'd get two error messages upon failure rather than
> just one, but that's not necessarily a bad thing.)
> 
> A possibly valid response is that such a change is outside the scope
> of this series since the original code shared this odd architecture of
> sometimes returning 0, sometimes -1, and sometimes die()ing.

My aim was to try and to keep the changes to a minimum as this patch
isn't about changing the odd architecture of the original. I could add a
follow up patch that cleans things up as you suggest.

Best Wishes

Phillip

Eric Sunshine Oct. 10, 2018, 10:22 a.m. UTC | #3

On Wed, Oct 10, 2018 at 6:14 AM Phillip Wood <[e-mail]> wrote:
> On 14/09/2018 00:49, Eric Sunshine wrote:
> > What if, instead of exit()ing directly, you drop the conditional and
> > instead return the value of read_author_script():
> >
> >     return read_author_script(...);
> >
> > and let the caller deal with the zero or non-zero return value as
> > usual? (True, you'd get two error messages upon failure rather than
> > just one, but that's not necessarily a bad thing.)
> >
> > A possibly valid response is that such a change is outside the scope
> > of this series since the original code shared this odd architecture of
> > sometimes returning 0, sometimes -1, and sometimes die()ing.
>
> My aim was to try and to keep the changes to a minimum as this patch
> isn't about changing the odd architecture of the original. I could add a
> follow up patch that cleans things up as you suggest.

The code already had that weird mix of return(s) and die(), hence the
state is no worse after this patch than before. So, a cleanup patch
later in the series, a follow up series, or doing nothing at all are
all reasonable approaches. I don't insist on it for this patch series.
Thanks.

Patch

diff --git a/builtin/am.c b/builtin/am.c
index 8c165f747b..aa5de0ee73 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -260,32 +260,6 @@  static int read_state_file(struct strbuf *sb, const struct am_state *state,
 	die_errno(_("could not read '%s'"), am_path(state, file));
 }
 
-/**
- * Take a series of KEY='VALUE' lines where VALUE part is
- * sq-quoted, and append <KEY, VALUE> at the end of the string list
- */
-static int parse_key_value_squoted(char *buf, struct string_list *list)
-{
-	while (*buf) {
-		struct string_list_item *item;
-		char *np;
-		char *cp = strchr(buf, '=');
-		if (!cp)
-			return -1;
-		np = strchrnul(cp, '\n');
-		*cp++ = '\0';
-		item = string_list_append(list, buf);
-
-		buf = np + (*np == '\n');
-		*np = '\0';
-		cp = sq_dequote(cp);
-		if (!cp)
-			return -1;
-		item->util = xstrdup(cp);
-	}
-	return 0;
-}
-
 /**
  * Reads and parses the state directory's "author-script" file, and sets
  * state->author_name, state->author_email and state->author_date accordingly.
@@ -305,39 +279,16 @@  static int parse_key_value_squoted(char *buf, struct string_list *list)
 static int read_am_author_script(struct am_state *state)
 {
 	const char *filename = am_path(state, "author-script");
-	struct strbuf buf = STRBUF_INIT;
-	struct string_list kv = STRING_LIST_INIT_DUP;
-	int retval = -1; /* assume failure */
-	int fd;
 
 	assert(!state->author_name);
 	assert(!state->author_email);
 	assert(!state->author_date);
 
-	fd = open(filename, O_RDONLY);
-	if (fd < 0) {
-		if (errno == ENOENT)
-			return 0;
-		die_errno(_("could not open '%s' for reading"), filename);
-	}
-	strbuf_read(&buf, fd, 0);
-	close(fd);
-	if (parse_key_value_squoted(buf.buf, &kv))
-		goto finish;
+	if (read_author_script(filename, &state->author_name,
+			       &state->author_email, &state->author_date, 1))
+		exit(128);
 
-	if (kv.nr != 3 ||
-	    strcmp(kv.items[0].string, "GIT_AUTHOR_NAME") ||
-	    strcmp(kv.items[1].string, "GIT_AUTHOR_EMAIL") ||
-	    strcmp(kv.items[2].string, "GIT_AUTHOR_DATE"))
-		goto finish;
-	state->author_name = kv.items[0].util;
-	state->author_email = kv.items[1].util;
-	state->author_date = kv.items[2].util;
-	retval = 0;
-finish:
-	string_list_clear(&kv, !!retval);
-	strbuf_release(&buf);
-	return retval;
+	return 0;
 }
 
 /**
diff --git a/sequencer.c b/sequencer.c
index dc2c58d464..5d0ff8f1c1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -660,6 +660,68 @@  static int write_author_script(const char *message)
 	return res;
 }
 
+/**
+ * Take a series of KEY='VALUE' lines where VALUE part is
+ * sq-quoted, and append <KEY, VALUE> at the end of the string list
+ */
+static int parse_key_value_squoted(char *buf, struct string_list *list)
+{
+	while (*buf) {
+		struct string_list_item *item;
+		char *np;
+		char *cp = strchr(buf, '=');
+		if (!cp)
+			return -1;
+		np = strchrnul(cp, '\n');
+		*cp++ = '\0';
+		item = string_list_append(list, buf);
+
+		buf = np + (*np == '\n');
+		*np = '\0';
+		cp = sq_dequote(cp);
+		if (!cp)
+			return -1;
+		item->util = xstrdup(cp);
+	}
+	return 0;
+}
+
+int read_author_script(const char *path, char **name, char **email, char **date,
+		       int allow_missing)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct string_list kv = STRING_LIST_INIT_DUP;
+	int retval = -1;
+
+	if (strbuf_read_file(&buf, path, 256) <= 0) {
+		strbuf_release(&buf);
+		if (errno == ENOENT && allow_missing)
+			return 0;
+		else
+			return error_errno(_("could not open '%s' for reading"),
+					   path);
+	}
+
+	if (parse_key_value_squoted(buf.buf, &kv)) {
+		error(_("unable to parse '%s'"), path);
+		goto finish;
+	}
+	if (kv.nr != 3 ||
+	    strcmp(kv.items[0].string, "GIT_AUTHOR_NAME") ||
+	    strcmp(kv.items[1].string, "GIT_AUTHOR_EMAIL") ||
+	    strcmp(kv.items[2].string, "GIT_AUTHOR_DATE")) {
+		error(_("unable to parse '%s'"), path);
+		goto finish;
+	}
+	*name = kv.items[0].util;
+	*email = kv.items[1].util;
+	*date = kv.items[2].util;
+	retval = 0;
+finish:
+	string_list_clear(&kv, !!retval);
+	strbuf_release(&buf);
+	return retval;
+}
 
 /*
  * write_author_script() used to fail to terminate the last line with a "'" and
diff --git a/sequencer.h b/sequencer.h
index c751c9d6e4..3713f955f5 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -107,4 +107,7 @@  void commit_post_rewrite(const struct commit *current_head,
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
 void print_commit_summary(const char *prefix, const struct object_id *oid,
 			  unsigned int flags);
+
+int read_author_script(const char *path, char **name, char **email, char **date,
+		       int allow_missing);
 #endif