am/rebase: share read_author_script()
cover am/rebase: share read_author_script()
From: Phillip Wood <[email protected]>
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (3):
am: rename read_author_script()
add read_author_script() to libgit
sequencer: use read_author_script()
builtin/am.c | 61 ++------------------ sequencer.c | 160 ++++++++++++++++++++++++++++----------------------- sequencer.h | 3 + 3 files changed, 96 insertions(+), 128 deletions(-)
Signature · 1 line
2.18.0
PATCH 1/3 am: rename read_author_script()
From: Phillip Wood <[email protected]>
Rename read_author_script() in preparation for adding a shared read_author_script() function to libgit.
--- builtin/am.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/am.c +2 -2
@@ -302,7 +302,7 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)* script, and thus if the file differs from what this function expects, it is * better to bail out than to do something that the user does not expect. */ -static int read_author_script (struct am_state *state) +static int read_am_author_script (struct am_state *state) {const char *filename =am_path (state,"author-script" );struct strbuf buf =STRBUF_INIT ;
@@ -411,7 +411,7 @@ static void am_load(struct am_state *state)BUG ("state file 'last' does not exist" ); state->last =strtol (sb.buf,NULL ,10 ); -if (read_author_script (state) <0 ) +if (read_am_author_script (state) <0 )die (_ ("could not parse author script" ));read_commit_msg (state);
Signature · 1 line
2.18.0
PATCH 2/3 add read_author_script() to libgit
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
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.
--- builtin/am.c | 57 ++++------------------------------------------- sequencer.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ sequencer.h | 3 +++ 3 files changed, 69 insertions(+), 53 deletions(-)
builtin/am.c +4 -53
@@ -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 ; }/**
sequencer.c +62 -0
68 lines not on this page · raw message
sequencer.h +3 -0
@@ -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
Signature · 1 line
2.18.0
Re: [PATCH 2/3] add read_author_script() to libgit
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 280
Quoted 11 lines · Phillip Wood
Add
read_author_script()tosequencer.cbased on the implementation inbuiltin/am.cand updateread_am_author_script()to useread_author_script(). The sequencer code that reads the author script will be updated in the next commit.Signed-off-by Phillip Wood <[email protected]>---
builtin/am.c +0
-0@@ -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'...
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 289
Quoted 4 lines
builtin/am.c +3
-0const 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...
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 285
Quoted 1 line
builtin/am.c +1
-0+return 0 ;
... or returns 0.
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 286
Quoted 1 line
builtin/am.c +0
-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.
Re: [PATCH 2/3] add read_author_script() to libgit
Hi Eric
Thanks for looking at this series, sorry it has taken so long for me to reply.
Quoted 37 lines · Eric Sunshine
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 280
Quoted 11 lines · Phillip Wood
Add
read_author_script()tosequencer.cbased on the implementation inbuiltin/am.cand updateread_am_author_script()to useread_author_script(). The sequencer code that reads the author script will be updated in the next commit.Signed-off-by Phillip Wood <[email protected]>---
builtin/am.c +0
-0@@ -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'...
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 289
Quoted 4 lines
builtin/am.c +3
-0const 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...
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 285
Quoted 1 line
builtin/am.c +1
-0+return 0 ;... or returns 0.
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 286
Quoted 1 line
builtin/am.c +0
-0}It's a little disturbing that it now invokes
exit()directly rather than callingdie()sincedie()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(...);
Quoted 7 lines
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
Re: [PATCH 2/3] add read_author_script() to libgit
Quoted 10 lines · Phillip Wood
Quoted 9 lines · Eric Sunshine
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.)
Quoted 7 lines
Quoted 3 lines
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 3/3 sequencer: use read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Use the new function to read the author script, updating read_env_script() and read_author_ident(). This means we now have a single code path that reads the author script and uses sq_dequote() to dequote it. This fixes potential problems with user edited scripts as read_env_script() which did not track quotes properly.
This commit also removes the fallback code for checking for a broken author script after git is upgraded when a rebase is stopped. Now that the parsing uses sq_dequote() it will reliably return an error if the quoting is broken and the user will have to abort the rebase and restart. This isn't ideal but it's a corner case and the detection of the broken quoting could be confused by user edited author scripts.
--- sequencer.c | 108 +++++++++++++++------------------------------------- 1 file changed, 30 insertions(+), 78 deletions(-)
sequencer.c +30 -78
@@ -723,54 +723,35 @@ int read_author_script(const char *path, char **name, char **email, char **date,return retval; } -/* -* write_author_script() used to fail to terminate the last line with a "'" and -* also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". We check for -* the terminating "'" on the last line to see how "'" has been escaped in case -* git was upgraded while rebase was stopped. -*/ -static int quoting_is_broken (const char *s,size_t n) -{ -/* Skip any empty lines in case the file was hand edited */ -while (n >0 && s[--n] =='\n' ) - ;/* empty */ -if (n >0 && s[n] !='\'' ) -return 1 ; - -return 0 ; -} -/* * Read a list of environment variable assignments (such as the author-script * file) into an environment block. Returns -1 on error, 0 otherwise. */ static int read_env_script (struct argv_array *env) {struct strbuf script =STRBUF_INIT ; -int i, count =0 , sq_bug; -const char *p2; -char *p; +char *name, *email, *date; -if (strbuf_read_file (&script,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return -1 ; -/* write_author_script() used to quote incorrectly */ - sq_bug =quoting_is_broken (script.buf, script.len); -for (p = script.buf; *p; p++) -if (sq_bug &&skip_prefix (p,"'\\\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (skip_prefix (p,"'\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (*p =='\'' ) -strbuf_splice (&script, p-- - script.buf,1 ,"" ,0 ); -else if (*p =='\n' ) { - *p ='\0' ; - count++; - } -for (i =0 , p = script.buf; i < count; i++) { -argv_array_push (env, p); - p +=strlen (p) +1 ; - } +strbuf_addstr (&script,"GIT_AUTHOR_NAME=" ); +strbuf_addstr (&script, name); +argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_EMAIL=" ); +strbuf_addstr (&script, email); +argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_DATE=" ); +strbuf_addstr (&script, date); +argv_array_push (env, script.buf); +strbuf_release (&script); + +free (name); +free (email); +free (date);return 0 ; }
@@ -790,54 +771,25 @@ static char *get_author(const char *message)/* Read author-script and return an ident line (author <email> timestamp) */ static const char *read_author_ident (struct strbuf *buf) { -const char *keys[] = { -"GIT_AUTHOR_NAME=" ,"GIT_AUTHOR_EMAIL=" ,"GIT_AUTHOR_DATE=" - }; -struct strbuf out =STRBUF_INIT ; -char *in, *eol; -const char *val[3 ]; -int i =0 ; +char *name, *email, *date; -if (strbuf_read_file (buf,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return NULL ; -/* dequote values and construct ident line in-place */ -for (in = buf->buf; i <3 && in - buf->buf < buf->len; i++) { -if (!skip_prefix (in, keys[i], (const char **)&in)) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } - - eol =strchrnul (in,'\n' ); - *eol ='\0' ; -if (!sq_dequote (in)) { -warning (_ ("bad quoting on %s value in '%s'" ), - keys[i],rebase_path_author_script ()); -return NULL ; - } - val[i] = in; - in = eol +1 ; - } - -if (i <3 ) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } -/* validate date since fmt_ident() will die() on bad value */ -if (parse_date (val[2 ], &out)){ +if (parse_date (date, buf)){warning (_ ("invalid date format '%s' in '%s'" ), - val[2 ],rebase_path_author_script ()); -strbuf_release (&out); + date,rebase_path_author_script ()); +strbuf_release (buf);return NULL ; } -strbuf_reset (&out); -strbuf_addstr (&out,fmt_ident (val[0 ], val[1 ], val[2 ],0 )); -strbuf_swap (buf, &out); -strbuf_release (&out); +strbuf_reset (buf); +strbuf_addstr (buf,fmt_ident (name, email, date,0 )); +free (name); +free (email); +free (date);return buf->buf; }
Signature · 1 line
2.18.0
Re: [PATCH 3/3] sequencer: use read_author_script()
Quoted 14 lines · Phillip Wood
Use the new function to read the author script, updating
read_env_script()andread_author_ident(). This means we now have a single code path that reads the author script and usessq_dequote()to dequote it. This fixes potential problems with user edited scripts asread_env_script()which did not track quotes properly.[...]Signed-off-by Phillip Wood <[email protected]>---
sequencer.c +2
-0@@ -723,54 +723,35 @@ int read_author_script(const char *path, char **name, char **email, char **date,static int read_env_script (struct argv_array *env) { +strbuf_addstr (&script,"GIT_AUTHOR_NAME=" ); +strbuf_addstr (&script, name);
sequencer.c int read_author_script(const char *path, char **name, char **email, char **date, · line 736
Quoted 10 lines
sequencer.c +10
-0+argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_EMAIL=" ); +strbuf_addstr (&script, email); +argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_DATE=" ); +strbuf_addstr (&script, date); +argv_array_push (env, script.buf); +strbuf_release (&script);
I haven't read this series closely yet, but this caught my eye while scanning it quickly. The above noisy code can all be collapsed to the simpler:
argv_array_pushf(env, "GIT_AUTHOR_NAME=%s", name);
argv_array_pushf(env, "GIT_AUTHOR_EMAIL =%s", email);
argv_array_pushf(env, "GIT_AUTHOR_DATE =%s", date);Re: [PATCH 3/3] sequencer: use read_author_script()
In reply to Phillip WoodQuoted 5 lines · Phillip Wood
Use the new function to read the author script, updating
read_env_script()andread_author_ident(). This means we now have a single code path that reads the author script and usessq_dequote()to dequote it. This fixes potential problems with user edited scripts asread_env_script()which did not track quotes properly.
Quoted 7 lines
[...]Signed-off-by Phillip Wood <[email protected]>---
/*
* Read a list of environment variable assignments (such as the author-script
* file) into an environment block. Returns -1 on error, 0 otherwise. */
According to this comment, this function is capable of parsing a file of arbitrary "NAME=Value" lines, and indeed the original code does just that, but...
sequencer.c int read_author_script(const char *path, char **name, char **email, char **date, · line 736
Quoted 7 lines
static int
read_env_script(struct argv_array *env){sequencer.c +3
-1+char *name, *email, *date; -if (strbuf_read_file (&script,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))
...the new implementation is able to handle only GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and GIT_AUTHOR_DATE, in exactly that order.
This seems like a pretty serious (and possibly buggy) change of behavior, and makes the function much less useful (in general). Is it true that it will only ever be used for files containing that limited set of names? If so, the behavior change deserves mention in the commit message, the function comment needs updating, and the function itself probably ought to be renamed.
sequencer.c int read_author_script(const char *path, char **name, char **email, char **date, · line 750
Quoted 12 lines
sequencer.c +12
-0+strbuf_addstr (&script,"GIT_AUTHOR_NAME=" ); +strbuf_addstr (&script, name); +argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_EMAIL=" ); +strbuf_addstr (&script, email); +argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_DATE=" ); +strbuf_addstr (&script, date); +argv_array_push (env, script.buf); +strbuf_release (&script);
Mentioned earlier[1], this can all collapse down to:
argv_array_pushf(env, "GIT_AUTHOR_NAME=%s", name); argv_array_pushf(env, "GIT_AUTHOR_EMAIL=%s", email); argv_array_pushf(env, "GIT_AUTHOR_DATE=%s", date);
However, it's unfortunate that this manual and hard-coded reconstruction is needed at all. If you restructure the factoring of this patch series, such ugliness can be avoided altogether. For instance, the series could be structured like this:
1. Introduce a general-purpose function for reading a file containing arbitrary "NAME=Value" lines (not carrying about specific key names or their order) and returning them in some data structure (perhaps via 'string_list' as parse_key_value_squoted() in patch 2/3 does).
2. Build read_author_script() atop #1, making it expect and extract GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and GIT_AUTHOR_DATE (possibly in that order, or possibly not if we don't care).
3. Retrofit existing parsers to call one of those two functions (this step may happen over several patches). So, for instance, read_env_script() would call the generic parser from #1, whereas sequencer.c:read_author_ident() would call the more specific parser from #2.
More below...
Quoted 18 lines
sequencer.c +2
-11@@ -790,54 +771,25 @@ static char *get_author(const char *message)/* Read author-script and return an ident line (author <email> timestamp) */ static const char *read_author_ident (struct strbuf *buf) { -const char *keys[] = { -"GIT_AUTHOR_NAME=" ,"GIT_AUTHOR_EMAIL=" ,"GIT_AUTHOR_DATE=" - }; -if (strbuf_read_file (buf,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return NULL ; -/* dequote values and construct ident line in-place */ -for (in = buf->buf; i <3 && in - buf->buf < buf->len; i++) { -if (!skip_prefix (in, keys[i], (const char **)&in)) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - }
sequencer.c static char *get_author(const char *message) · line 814
Quoted 10 lines
sequencer.c +0
-10-if (!sq_dequote (in)) { -warning (_ ("bad quoting on %s value in '%s'" ), - keys[i],rebase_path_author_script ()); -return NULL ; - } -if (i <3 ) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - }
The parsing code being thrown away here does a better job of diagnosing problems (thus helping the user figure out what went wrong) than the new shared parser introduced by patch 2/3. The shared function only ever reports a generic "unable to parse", whereas the above code gets specific, saying that it was looking for a particular key or that quoting was broken. I'd have expected the new shared parser to encompass the best features of the existing parsers (such as presenting better error messages).
sequencer.c static char *get_author(const char *message) · line 781
Quoted 3 lines
sequencer.c +1
-1/* validate date since fmt_ident() will die() on bad value */ -if (parse_date (val[2 ], &out)){ +if (parse_date (date, buf)){
Re-purposing the strbuf 'buf', which is passed into this function, binds this function too tightly with its caller by assuming that the caller will never need the original content of 'buf' anymore. Thus, it would be better for this code continue using its own local strbuf 'out' rather than re-purposing the incoming 'buf'.
sequencer.c static char *get_author(const char *message) · line 784
Quoted 5 lines
sequencer.c +2
-2warning (_ ("invalid date format '%s' in '%s'" ), - val[2 ],rebase_path_author_script ()); -strbuf_release (&out); + date,rebase_path_author_script ()); +strbuf_release (buf);
Likewise, it's doubly odd to see this function releasing 'buf' which it does not own.
Review static char *get_author(const char *message) · line 783
Quoted 2 lines
(quoted diff) +0
-0return NULL; }
[1]: https://public-inbox.org/git/CAPig+cRvUr26GZyW6ecYhpwABueBqaEfZH1+JjLaqZo8+RTD6Q@mail.gmail.com/
Re: [PATCH 3/3] sequencer: use read_author_script()
Quoted 29 lines · Eric Sunshine
Quoted 5 lines · Phillip Wood
Use the new function to read the author script, updating
read_env_script()andread_author_ident(). This means we now have a single code path that reads the author script and usessq_dequote()to dequote it. This fixes potential problems with user edited scripts asread_env_script()which did not track quotes properly.Quoted 7 lines
[...]Signed-off-by Phillip Wood <[email protected]>---
/*
* Read a list of environment variable assignments (such as the author-script
* file) into an environment block. Returns -1 on error, 0 otherwise. */According to this comment, this function is capable of parsing a file of arbitrary "NAME=Value" lines, and indeed the original code does just that, but...
sequencer.c int read_author_script(const char *path, char **name, char **email, char **date, · line 736
Quoted 7 lines
static int
read_env_script(struct argv_array *env){sequencer.c +3
-1+char *name, *email, *date; -if (strbuf_read_file (&script,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))...the new implementation is able to handle only
GIT_AUTHOR_NAME,GIT_AUTHOR_EMAIL, andGIT_AUTHOR_DATE, in exactly that order.
Quoted 6 lines
This seems like a pretty serious (and possibly buggy) change of behavior, and makes the function much less useful (in general). Is it true that it will only ever be used for files containing that limited set of names? If so, the behavior change deserves mention in the commit message, the function comment needs updating, and the function itself probably ought to be renamed.
You're right the change in behavior should be mentioned explicitly, I'd not thought about it in those terms. I'm not sure if the change is buggy, this code is what am uses for its author script handling. To me the point of the author-script file is to set the author details, not to set arbitrary environment variables. We have already significantly reduced what someone can do with this file in the transition from shell to C as we no longer support arbitrary shell code in the file. I'd rather try and reuse the existing code from am unless someone can demonstrate an active use for something more general. (I'm still not sure what use editing the author-script is - it is only of use if the rebase stops for conflicts, it cannot be used to change the author of an arbitrary set of commits)
Quoted 33 lines
sequencer.c int read_author_script(const char *path, char **name, char **email, char **date, · line 750
Quoted 12 lines
sequencer.c +12
-0+strbuf_addstr (&script,"GIT_AUTHOR_NAME=" ); +strbuf_addstr (&script, name); +argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_EMAIL=" ); +strbuf_addstr (&script, email); +argv_array_push (env, script.buf); +strbuf_reset (&script); +strbuf_addstr (&script,"GIT_AUTHOR_DATE=" ); +strbuf_addstr (&script, date); +argv_array_push (env, script.buf); +strbuf_release (&script);Mentioned earlier[1], this can all collapse down to:
argv_array_pushf(env, "GIT_AUTHOR_NAME=%s", name);argv_array_pushf(env, "GIT_AUTHOR_EMAIL=%s", email);argv_array_pushf(env, "GIT_AUTHOR_DATE=%s", date);However, it's unfortunate that this manual and hard-coded reconstruction is needed at all. If you restructure the factoring of this patch series, such ugliness can be avoided altogether. For instance, the series could be structured like this:
1. Introduce a general-purpose function for reading a file containing arbitrary "NAME=Value" lines (not carrying about specific key names or their order) and returning them in some data structure (perhaps via '
string_list' asparse_key_value_squoted()in patch 2/3 does).2. Build
read_author_script()atop #1, making it expect and extractGIT_AUTHOR_NAME,GIT_AUTHOR_EMAIL, andGIT_AUTHOR_DATE(possibly in that order, or possibly not if we don't care).
Quoted 5 lines
3. Retrofit existing parsers to call one of those two functions (this step may happen over several patches). So, for instance,
read_env_script()would call the generic parser from #1, whereas sequencer.c:read_author_ident() would call the more specific parser from #2.
That plan requires all new code rather than reusing tried and tested code from am. Furthermore I'm not sure it has any advantage as far as users are concerned. My aim with this series was to try and do something fairly simple that reused the parsing from am, rather than build a whole new system with its own bugs.
Quoted 31 lines
More below...
Quoted 18 lines
sequencer.c +2
-11@@ -790,54 +771,25 @@ static char *get_author(const char *message)/* Read author-script and return an ident line (author <email> timestamp) */ static const char *read_author_ident (struct strbuf *buf) { -const char *keys[] = { -"GIT_AUTHOR_NAME=" ,"GIT_AUTHOR_EMAIL=" ,"GIT_AUTHOR_DATE=" - }; -if (strbuf_read_file (buf,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return NULL ; -/* dequote values and construct ident line in-place */ -for (in = buf->buf; i <3 && in - buf->buf < buf->len; i++) { -if (!skip_prefix (in, keys[i], (const char **)&in)) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - }sequencer.c static char *get_author(const char *message) · line 814
Quoted 10 lines
sequencer.c +0
-10-if (!sq_dequote (in)) { -warning (_ ("bad quoting on %s value in '%s'" ), - keys[i],rebase_path_author_script ()); -return NULL ; - } -if (i <3 ) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - }
Quoted 8 lines
The parsing code being thrown away here does a better job of diagnosing problems (thus helping the user figure out what went wrong) than the new shared parser introduced by patch 2/3. The shared function only ever reports a generic "unable to parse", whereas the above code gets specific, saying that it was looking for a particular key or that quoting was broken. I'd have expected the new shared parser to encompass the best features of the existing parsers (such as presenting better error messages).
You're right but the context is that this function is only used when the root commit is rebased (and then only for the root commit). Everything else goes through read_env_script() which doesn't even bother to check if all the variables have been set or report any parsing errors.
Quoted 10 lines
sequencer.c static char *get_author(const char *message) · line 781
Quoted 4 lines
sequencer.c +1
-1/* validate date since fmt_ident() will die() on bad value */ -if (parse_date (val[2 ], &out)){ +if (parse_date (date, buf)){Re-purposing the strbuf 'buf', which is passed into this function, binds this function too tightly with its caller by assuming that the caller will never need the original content of 'buf' anymore. Thus, it would be better for this code continue using its own local strbuf 'out' rather than re-purposing the incoming 'buf'.
That's a good point, I'll fix it.
Quoted 6 lines
sequencer.c static char *get_author(const char *message) · line 784
Quoted 5 lines
sequencer.c +2
-2warning (_ ("invalid date format '%s' in '%s'" ), - val[2 ],rebase_path_author_script ()); -strbuf_release (&out); + date,rebase_path_author_script ()); +strbuf_release (buf);
Quoted 8 lines
Likewise, it's doubly odd to see this function releasing 'buf' which it does not own.
Review static char *get_author(const char *message) · line 783
Quoted 2 lines
(quoted diff) +0
-0return NULL; }[1]: https://public-inbox.org/git/CAPig+cRvUr26GZyW6ecYhpwABueBqaEfZH1+JjLaqZo8+RTD6Q@mail.gmail.com/
Thanks for looking at this, I'm keen to keep things simple and reuse the am author-script parsing if possible. It is more restrictive but I'm not sure that anyone is actually taking advantage of the flexibility offered by the current setup and it fixes the de-quoting bugs in read_env_script().
Best Wishes
Phillip
v2 cover am/rebase: share read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Thanks to Eric for his feedback on v1. I've rerolled based on that. Patches 1 & 2 are new and try to address some of the concerns Eric raised, particularly the error handling for a badly edited author script. See the notes on patches 4 & 5 for the changes to those (they were previously 2 & 3).
v1 cover letter:
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (5):
am: don't die in read_author_script()
am: improve author-script error reporting
am: rename read_author_script()
add read_author_script() to libgit
sequencer: use read_author_script()
builtin/am.c | 60 ++-------------- sequencer.c | 192 ++++++++++++++++++++++++++++++++------------------- sequencer.h | 3 + 3 files changed, 128 insertions(+), 127 deletions(-)
Signature · 1 line
2.19.0
PATCH v2 1/5 am: don't die in read_author_script()
From: Phillip Wood <[email protected]>
The caller is already prepared to handle errors returned from this function so there is no need for it to die if it cannot read the file.
--- builtin/am.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/am.c +2 -1
@@ -318,7 +318,8 @@ static int read_author_script(struct am_state *state)if (fd <0 ) {if (errno ==ENOENT )return 0 ; -die_errno (_ ("could not open '%s' for reading" ), filename); +return error_errno (_ ("could not open '%s' for reading" ), + filename); }strbuf_read (&buf, fd,0 );close (fd);
Signature · 1 line
2.19.0
Re: [PATCH v2 1/5] am: don't die in read_author_script()
Quoted 10 lines · Phillip Wood
From: Phillip Wood <[email protected]>
The caller is already prepared to handle errors returned from this function so there is no need for it to die if it cannot read the file.
Suggested-by Eric Sunshine <[email protected]>Signed-off-by Phillip Wood <[email protected]>--- builtin/am.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
OK. Having fewer die's in reusable codepath can only be a good thing.
Quoted 15 lines
builtin/am.c +2
-1@@ -318,7 +318,8 @@ static int read_author_script(struct am_state *state)if (fd <0 ) {if (errno ==ENOENT )return 0 ; -die_errno (_ ("could not open '%s' for reading" ), filename); +return error_errno (_ ("could not open '%s' for reading" ), + filename); }strbuf_read (&buf, fd,0 );close (fd);
PATCH v2 2/5 am: improve author-script error reporting
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
If there are errors in a user edited author-script there was no indication of what was wrong. This commit adds some specific error messages depending on the problem. It also relaxes the requirement that the variables appear in a specific order in the file to match the behavior of 'rebase --interactive'.
--- builtin/am.c | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-)
builtin/am.c +39 -10
@@ -270,8 +270,11 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)struct string_list_item *item;char *np;char *cp =strchr (buf,'=' ); -if (!cp) -return -1 ; +if (!cp) { + np =strchrnul (buf,'\n' ); +return error (_ ("unable to parse '%.*s'" ), + (int ) (np - buf), buf); + } np =strchrnul (cp,'\n' ); *cp++ ='\0' ; item =string_list_append (list, buf);
@@ -280,7 +283,8 @@ static int parse_key_value_squoted(char *buf, struct string_list *list) *np ='\0' ; cp =sq_dequote (cp);if (!cp) -return -1 ; +return error (_ ("unable to dequote value of '%s'" ), + item->string); item->util =xstrdup (cp); }return 0 ;
@@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)struct strbuf buf =STRBUF_INIT ;struct string_list kv =STRING_LIST_INIT_DUP ;int retval =-1 ;/* assume failure */ +int i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ;int fd;assert (!state->author_name);
@@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)if (parse_key_value_squoted (buf.buf, &kv)) 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" )) +for (i =0 ; i < kv.nr; i++) { +if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { +if (name_i >=0 ) + name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); +else + name_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { +if (email_i >=0 ) + email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); +else + email_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { +if (date_i >=0 ) + date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); +else + date_i = i; + }else { + err =error (_ ("unknown variable '%s'" ), + kv.items[i].string); + } + } +if (name_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_NAME'" )); +if (email_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); +if (date_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_DATE'" )); +if (date_i <0 || email_i <0 || date_i <0 || err) goto finish; - state->author_name = kv.items[0 ].util; - state->author_email = kv.items[1 ].util; - state->author_date = kv.items[2 ].util; + state->author_name = kv.items[name_i].util; + state->author_email = kv.items[email_i].util; + state->author_date = kv.items[date_i].util; retval =0 ; finish:string_list_clear (&kv, !!retval);
Signature · 1 line
2.19.0
Re: [PATCH v2 2/5] am: improve author-script error reporting
Quoted 7 lines · Phillip Wood
From: Phillip Wood <[email protected]>
If there are errors in a user edited author-script there was no indication of what was wrong. This commit adds some specific error messages depending on the problem. It also relaxes the requirement that the variables appear in a specific order in the file to match the behavior of 'rebase --interactive'.
That relaxing is sensible; there is no reason to insist that the file we are reading was written by exactly the same writer as we have.
Quoted 5 lines
builtin/am.c +0
-0
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 277
Quoted 10 lines
builtin/am.c +5
-2@@ -270,8 +270,11 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)struct string_list_item *item;char *np;char *cp =strchr (buf,'=' ); -if (!cp) -return -1 ; +if (!cp) { + np =strchrnul (buf,'\n' ); +return error (_ ("unable to parse '%.*s'" ), + (int ) (np - buf), buf); + }
We are unable to parse because it is not of KEY='VALUE' form. Is that something worth reporting, e.g. "no key present in '%.*s'"?
builtin/am.c static int parse_key_value_squoted(char *buf, struct string_list *list) · line 287
Quoted 7 lines
builtin/am.c +2
-1@@ -280,7 +283,8 @@ static int parse_key_value_squoted(char *buf, struct string_list *list) *np ='\0' ; cp =sq_dequote (cp);if (!cp) -return -1 ; +return error (_ ("unable to dequote value of '%s'" ), + item->string);
At this point, item->string is what we earlier found on the left hand side of the '=', i.e. the key. The message makes sense.
builtin/am.c static int read_author_script(struct am_state *state) · line 315
Quoted 5 lines
builtin/am.c +1
-0@@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)struct strbuf buf =STRBUF_INIT ;struct string_list kv =STRING_LIST_INIT_DUP ;int retval =-1 ;/* assume failure */ +int i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ;
That -2 is somewhat cute. If we find a dup, then it will become -1 so later check for missing field would not trigger.
Quoted 4 lines
builtin/am.c +0
-0@@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)if (parse_key_value_squoted (buf.buf, &kv)) goto finish;
builtin/am.c static int read_author_script(struct am_state *state) · line 339
Quoted 10 lines
builtin/am.c +6
-4-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" )) +for (i =0 ; i < kv.nr; i++) { +if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { +if (name_i >=0 ) + name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); +else + name_i = i;
However, if you have three instances of GIT_AUTHOR_NAME, then
- the first one makes name_i point at it
- the second one triggers an error and name_i becomes -1
- the third one makes name_i point at it again
And name_i is not -2 so we won't give "missing" error, which is OK, but we end up having a usable name even though we said we detected duplicate!
You can probably compare name_i with -2 when detecting the dup to fix it, i.e.
if (name_i != -2)
name_i = error("found dup");
else
name_i = i;Quoted 32 lines
builtin/am.c +25
-3+ }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { +if (email_i >=0 ) + email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); +else + email_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { +if (date_i >=0 ) + date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); +else + date_i = i; + }else { + err =error (_ ("unknown variable '%s'" ), + kv.items[i].string); + } + } +if (name_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_NAME'" )); +if (email_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); +if (date_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_DATE'" )); +if (date_i <0 || email_i <0 || date_i <0 || err) goto finish; - state->author_name = kv.items[0 ].util; - state->author_email = kv.items[1 ].util; - state->author_date = kv.items[2 ].util; + state->author_name = kv.items[name_i].util; + state->author_email = kv.items[email_i].util; + state->author_date = kv.items[date_i].util; retval =0 ; finish:string_list_clear (&kv, !!retval);
PATCH v2 3/5 am: rename read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Rename read_author_script() in preparation for adding a shared read_author_script() function to libgit.
--- builtin/am.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/am.c +2 -2
@@ -306,7 +306,7 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)* script, and thus if the file differs from what this function expects, it is * better to bail out than to do something that the user does not expect. */ -static int read_author_script (struct am_state *state) +static int read_am_author_script (struct am_state *state) {const char *filename =am_path (state,"author-script" );struct strbuf buf =STRBUF_INIT ;
@@ -441,7 +441,7 @@ static void am_load(struct am_state *state)BUG ("state file 'last' does not exist" ); state->last =strtol (sb.buf,NULL ,10 ); -if (read_author_script (state) <0 ) +if (read_am_author_script (state) <0 )die (_ ("could not parse author script" ));read_commit_msg (state);
Signature · 1 line
2.19.0
PATCH v2 4/5 add read_author_script() to libgit
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
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.
---
Notes:
changes since v1:
- added comment above read_author_script()
- rebased to reflect changes added in patch 2
builtin/am.c | 86 +---------------------------------------- sequencer.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ sequencer.h | 3 ++ 3 files changed, 110 insertions(+), 84 deletions(-)
builtin/am.c +2 -84
@@ -260,36 +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) { - np =strchrnul (buf,'\n' ); -return error (_ ("unable to parse '%.*s'" ), - (int ) (np - buf), buf); - } - 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 error (_ ("unable to dequote value of '%s'" ), - item->string); - 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.
@@ -309,65 +279,13 @@ 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 i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ; -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 ; -return error_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; - -for (i =0 ; i < kv.nr; i++) { -if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { -if (name_i >=0 ) - name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); -else - name_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { -if (email_i >=0 ) - email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); -else - email_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { -if (date_i >=0 ) - date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); -else - date_i = i; - }else { - err =error (_ ("unknown variable '%s'" ), - kv.items[i].string); - } - } -if (name_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_NAME'" )); -if (email_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); -if (date_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_DATE'" )); -if (date_i <0 || email_i <0 || date_i <0 || err) - goto finish; - state->author_name = kv.items[name_i].util; - state->author_email = kv.items[email_i].util; - state->author_date = kv.items[date_i].util; - retval =0 ; -finish: -string_list_clear (&kv, !!retval); -strbuf_release (&buf); -return retval; +return read_author_script (filename, &state->author_name, + &state->author_email, &state->author_date,1 ); }/**
sequencer.c +105 -0
111 lines not on this page · raw message
sequencer.h +3 -0
@@ -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
Signature · 1 line
2.19.0
PATCH v2 5/5 sequencer: use read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Use the new function added in the last commit to read the author script, updating read_env_script() and read_author_ident(). We now have a single code path that reads the author script for am and all flavors of rebase. This changes the behavior of read_env_script() as previously it would set any environment variables that were in the author-script file. Now it is an error if the file contains other variables or any of GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE are missing. This is what am and the non interactive version of rebase have been doing for several years so hopefully it will not cause a problem for interactive rebase users. The advantage is that we are reusing existing code from am which uses sq_dequote() to properly dequote variables. This fixes potential problems with user edited scripts as read_env_script() which did not track quotes properly.
This commit also removes the fallback code for checking for a broken author script after git is upgraded when a rebase is stopped. Now that the parsing uses sq_dequote() it will reliably return an error if the quoting is broken and the user will have to abort the rebase and restart. This isn't ideal but it's a corner case and the detection of the broken quoting could be confused by user edited author scripts.
---
Notes:
changes since v1
- use argv_array_pushf() as suggested by Eric
- fixed strbuf handling as suggested by Eric
- fix comments and commit message to reflect changed behavior of read_env_script()
sequencer.c | 97 ++++++++++++----------------------------------------- 1 file changed, 21 insertions(+), 76 deletions(-)
sequencer.c +21 -76
@@ -767,53 +767,24 @@ int read_author_script(const char *path, char **name, char **email, char **date, }/* -* write_author_script() used to fail to terminate the last line with a "'" and -* also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". We check for -* the terminating "'" on the last line to see how "'" has been escaped in case -* git was upgraded while rebase was stopped. -*/ -static int quoting_is_broken (const char *s,size_t n) -{ -/* Skip any empty lines in case the file was hand edited */ -while (n >0 && s[--n] =='\n' ) - ;/* empty */ -if (n >0 && s[n] !='\'' ) -return 1 ; - -return 0 ; -} - -/* -* Read a list of environment variable assignments (such as the author-script -* file) into an environment block. Returns -1 on error, 0 otherwise. +* Read a GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL AND GIT_AUTHOR_DATE from a +* file with shell quoting into struct argv_array. Returns -1 on +* error, 0 otherwise. */ static int read_env_script (struct argv_array *env) { -struct strbuf script =STRBUF_INIT ; -int i, count =0 , sq_bug; -const char *p2; -char *p; +char *name, *email, *date; -if (strbuf_read_file (&script,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return -1 ; -/* write_author_script() used to quote incorrectly */ - sq_bug =quoting_is_broken (script.buf, script.len); -for (p = script.buf; *p; p++) -if (sq_bug &&skip_prefix (p,"'\\\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (skip_prefix (p,"'\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (*p =='\'' ) -strbuf_splice (&script, p-- - script.buf,1 ,"" ,0 ); -else if (*p =='\n' ) { - *p ='\0' ; - count++; - } -for (i =0 , p = script.buf; i < count; i++) { -argv_array_push (env, p); - p +=strlen (p) +1 ; - } +argv_array_pushf (env,"GIT_AUTHOR_NAME=%s" , name); +argv_array_pushf (env,"GIT_AUTHOR_EMAIL=%s" , email); +argv_array_pushf (env,"GIT_AUTHOR_DATE=%s" , date); +free (name); +free (email); +free (date);return 0 ; }
@@ -833,54 +804,28 @@ static char *get_author(const char *message)/* Read author-script and return an ident line (author <email> timestamp) */ static const char *read_author_ident (struct strbuf *buf) { -const char *keys[] = { -"GIT_AUTHOR_NAME=" ,"GIT_AUTHOR_EMAIL=" ,"GIT_AUTHOR_DATE=" - };struct strbuf out =STRBUF_INIT ; -char *in, *eol; -const char *val[3 ]; -int i =0 ; +char *name, *email, *date; -if (strbuf_read_file (buf,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return NULL ; -/* dequote values and construct ident line in-place */ -for (in = buf->buf; i <3 && in - buf->buf < buf->len; i++) { -if (!skip_prefix (in, keys[i], (const char **)&in)) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } - - eol =strchrnul (in,'\n' ); - *eol ='\0' ; -if (!sq_dequote (in)) { -warning (_ ("bad quoting on %s value in '%s'" ), - keys[i],rebase_path_author_script ()); -return NULL ; - } - val[i] = in; - in = eol +1 ; - } - -if (i <3 ) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } -/* validate date since fmt_ident() will die() on bad value */ -if (parse_date (val[2 ], &out)){ +if (parse_date (date, &out)){warning (_ ("invalid date format '%s' in '%s'" ), - val[2 ],rebase_path_author_script ()); + date,rebase_path_author_script ());strbuf_release (&out);return NULL ; }strbuf_reset (&out); -strbuf_addstr (&out,fmt_ident (val[0 ], val[1 ], val[2 ],0 )); +strbuf_addstr (&out,fmt_ident (name, email, date,0 ));strbuf_swap (buf, &out);strbuf_release (&out); +free (name); +free (email); +free (date);return buf->buf; }
Signature · 1 line
2.19.0
Quoted 7 lines · Phillip Wood
From: Phillip Wood <[email protected]>
Thanks to Eric for his feedback on v1. I've rerolled based on that. Patches 1 & 2 are new and try to address some of the concerns Eric raised, particularly the error handling for a badly edited author script. See the notes on patches 4 & 5 for the changes to those (they were previously 2 & 3).
I spotted a weird corner case buglet, but it seems that this one is ready for 'next' even without fixing that "give it three times and we will happily continue" thing.
Do we know of any other issues? Can we now move it forward?
Thanks.
Hi Junio
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Phillip Wood
From: Phillip Wood <[email protected]>
Thanks to Eric for his feedback on v1. I've rerolled based on that. Patches 1 & 2 are new and try to address some of the concerns Eric raised, particularly the error handling for a badly edited author script. See the notes on patches 4 & 5 for the changes to those (they were previously 2 & 3).
Quoted 3 lines
I spotted a weird corner case buglet, but it seems that this one is ready for 'next' even without fixing that "give it three times and we will happily continue" thing.
Well spotted on the corner case. If you're happy to hold off on moving it to next I can send a re-roll with fixes for that next week or I can do it as a fixup if you want to move this forward now.
Quoted 1 line
Do we know of any other issues? Can we now move it forward?
I don't know of any other issues but I don't think anyone else has looked at this iteration. Thanks for taking a look at these.
Best Wishes
Phillip
Quoted 2 lines
Thanks.
Quoted 7 lines · Phillip Wood
Quoted 3 lines
I spotted a weird corner case buglet, but it seems that this one is ready for 'next' even without fixing that "give it three times and we will happily continue" thing.
Well spotted on the corner case. If you're happy to hold off on moving it to next I can send a re-roll with fixes for that next week or I can do it as a fixup if you want to move this forward now.
OK, I'll mark it as "expecting a reroll".
v3 cover am/rebase: share read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Thanks to Junio for the feedback on v2. I've updated patch 4 based on those comments, the rest are unchanged.
v1 cover letter:
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (5):
am: don't die in read_author_script()
am: improve author-script error reporting
am: rename read_author_script()
add read_author_script() to libgit
sequencer: use read_author_script()
builtin/am.c | 60 ++-------------- sequencer.c | 192 ++++++++++++++++++++++++++++++++------------------- sequencer.h | 3 + 3 files changed, 128 insertions(+), 127 deletions(-)
Signature · 1 line
2.19.1
PATCH v3 1/5 am: don't die in read_author_script()
From: Phillip Wood <[email protected]>
The caller is already prepared to handle errors returned from this function so there is no need for it to die if it cannot read the file.
--- builtin/am.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/am.c +2 -1
@@ -318,7 +318,8 @@ static int read_author_script(struct am_state *state)if (fd <0 ) {if (errno ==ENOENT )return 0 ; -die_errno (_ ("could not open '%s' for reading" ), filename); +return error_errno (_ ("could not open '%s' for reading" ), + filename); }strbuf_read (&buf, fd,0 );close (fd);
Signature · 1 line
2.19.1
PATCH v3 2/5 am: improve author-script error reporting
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
If there are errors in a user edited author-script there was no indication of what was wrong. This commit adds some specific error messages depending on the problem. It also relaxes the requirement that the variables appear in a specific order in the file to match the behavior of 'rebase --interactive'.
--- builtin/am.c | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-)
builtin/am.c +39 -10
@@ -270,8 +270,11 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)struct string_list_item *item;char *np;char *cp =strchr (buf,'=' ); -if (!cp) -return -1 ; +if (!cp) { + np =strchrnul (buf,'\n' ); +return error (_ ("unable to parse '%.*s'" ), + (int ) (np - buf), buf); + } np =strchrnul (cp,'\n' ); *cp++ ='\0' ; item =string_list_append (list, buf);
@@ -280,7 +283,8 @@ static int parse_key_value_squoted(char *buf, struct string_list *list) *np ='\0' ; cp =sq_dequote (cp);if (!cp) -return -1 ; +return error (_ ("unable to dequote value of '%s'" ), + item->string); item->util =xstrdup (cp); }return 0 ;
@@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)struct strbuf buf =STRBUF_INIT ;struct string_list kv =STRING_LIST_INIT_DUP ;int retval =-1 ;/* assume failure */ +int i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ;int fd;assert (!state->author_name);
@@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)if (parse_key_value_squoted (buf.buf, &kv)) 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" )) +for (i =0 ; i < kv.nr; i++) { +if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { +if (name_i >=0 ) + name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); +else + name_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { +if (email_i >=0 ) + email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); +else + email_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { +if (date_i >=0 ) + date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); +else + date_i = i; + }else { + err =error (_ ("unknown variable '%s'" ), + kv.items[i].string); + } + } +if (name_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_NAME'" )); +if (email_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); +if (date_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_DATE'" )); +if (date_i <0 || email_i <0 || date_i <0 || err) goto finish; - state->author_name = kv.items[0 ].util; - state->author_email = kv.items[1 ].util; - state->author_date = kv.items[2 ].util; + state->author_name = kv.items[name_i].util; + state->author_email = kv.items[email_i].util; + state->author_date = kv.items[date_i].util; retval =0 ; finish:string_list_clear (&kv, !!retval);
Signature · 1 line
2.19.1
PATCH v3 3/5 am: rename read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Rename read_author_script() in preparation for adding a shared read_author_script() function to libgit.
--- builtin/am.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/am.c +2 -2
@@ -306,7 +306,7 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)* script, and thus if the file differs from what this function expects, it is * better to bail out than to do something that the user does not expect. */ -static int read_author_script (struct am_state *state) +static int read_am_author_script (struct am_state *state) {const char *filename =am_path (state,"author-script" );struct strbuf buf =STRBUF_INIT ;
@@ -441,7 +441,7 @@ static void am_load(struct am_state *state)BUG ("state file 'last' does not exist" ); state->last =strtol (sb.buf,NULL ,10 ); -if (read_author_script (state) <0 ) +if (read_am_author_script (state) <0 )die (_ ("could not parse author script" ));read_commit_msg (state);
Signature · 1 line
2.19.1
PATCH v3 4/5 add read_author_script() to libgit
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
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.
---
Notes:
changes since v1:
- added comment above read_author_script()
- rebased to reflect changes added in patch 2
builtin/am.c | 86 +---------------------------------------- sequencer.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ sequencer.h | 3 ++ 3 files changed, 110 insertions(+), 84 deletions(-)
builtin/am.c +2 -84
@@ -260,36 +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) { - np =strchrnul (buf,'\n' ); -return error (_ ("unable to parse '%.*s'" ), - (int ) (np - buf), buf); - } - 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 error (_ ("unable to dequote value of '%s'" ), - item->string); - 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.
@@ -309,65 +279,13 @@ 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 i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ; -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 ; -return error_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; - -for (i =0 ; i < kv.nr; i++) { -if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { -if (name_i >=0 ) - name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); -else - name_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { -if (email_i >=0 ) - email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); -else - email_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { -if (date_i >=0 ) - date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); -else - date_i = i; - }else { - err =error (_ ("unknown variable '%s'" ), - kv.items[i].string); - } - } -if (name_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_NAME'" )); -if (email_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); -if (date_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_DATE'" )); -if (date_i <0 || email_i <0 || date_i <0 || err) - goto finish; - state->author_name = kv.items[name_i].util; - state->author_email = kv.items[email_i].util; - state->author_date = kv.items[date_i].util; - retval =0 ; -finish: -string_list_clear (&kv, !!retval); -strbuf_release (&buf); -return retval; +return read_author_script (filename, &state->author_name, + &state->author_email, &state->author_date,1 ); }/**
sequencer.c +105 -0
111 lines not on this page · raw message
sequencer.h +3 -0
@@ -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
Signature · 1 line
2.19.1
PATCH v3 5/5 sequencer: use read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Use the new function added in the last commit to read the author script, updating read_env_script() and read_author_ident(). We now have a single code path that reads the author script for am and all flavors of rebase. This changes the behavior of read_env_script() as previously it would set any environment variables that were in the author-script file. Now it is an error if the file contains other variables or any of GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE are missing. This is what am and the non interactive version of rebase have been doing for several years so hopefully it will not cause a problem for interactive rebase users. The advantage is that we are reusing existing code from am which uses sq_dequote() to properly dequote variables. This fixes potential problems with user edited scripts as read_env_script() which did not track quotes properly.
This commit also removes the fallback code for checking for a broken author script after git is upgraded when a rebase is stopped. Now that the parsing uses sq_dequote() it will reliably return an error if the quoting is broken and the user will have to abort the rebase and restart. This isn't ideal but it's a corner case and the detection of the broken quoting could be confused by user edited author scripts.
---
Notes:
changes since v1
- use argv_array_pushf() as suggested by Eric
- fixed strbuf handling as suggested by Eric
- fix comments and commit message to reflect changed behavior of read_env_script()
sequencer.c | 97 ++++++++++++----------------------------------------- 1 file changed, 21 insertions(+), 76 deletions(-)
sequencer.c +21 -76
@@ -767,53 +767,24 @@ int read_author_script(const char *path, char **name, char **email, char **date, }/* -* write_author_script() used to fail to terminate the last line with a "'" and -* also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". We check for -* the terminating "'" on the last line to see how "'" has been escaped in case -* git was upgraded while rebase was stopped. -*/ -static int quoting_is_broken (const char *s,size_t n) -{ -/* Skip any empty lines in case the file was hand edited */ -while (n >0 && s[--n] =='\n' ) - ;/* empty */ -if (n >0 && s[n] !='\'' ) -return 1 ; - -return 0 ; -} - -/* -* Read a list of environment variable assignments (such as the author-script -* file) into an environment block. Returns -1 on error, 0 otherwise. +* Read a GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL AND GIT_AUTHOR_DATE from a +* file with shell quoting into struct argv_array. Returns -1 on +* error, 0 otherwise. */ static int read_env_script (struct argv_array *env) { -struct strbuf script =STRBUF_INIT ; -int i, count =0 , sq_bug; -const char *p2; -char *p; +char *name, *email, *date; -if (strbuf_read_file (&script,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return -1 ; -/* write_author_script() used to quote incorrectly */ - sq_bug =quoting_is_broken (script.buf, script.len); -for (p = script.buf; *p; p++) -if (sq_bug &&skip_prefix (p,"'\\\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (skip_prefix (p,"'\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (*p =='\'' ) -strbuf_splice (&script, p-- - script.buf,1 ,"" ,0 ); -else if (*p =='\n' ) { - *p ='\0' ; - count++; - } -for (i =0 , p = script.buf; i < count; i++) { -argv_array_push (env, p); - p +=strlen (p) +1 ; - } +argv_array_pushf (env,"GIT_AUTHOR_NAME=%s" , name); +argv_array_pushf (env,"GIT_AUTHOR_EMAIL=%s" , email); +argv_array_pushf (env,"GIT_AUTHOR_DATE=%s" , date); +free (name); +free (email); +free (date);return 0 ; }
@@ -833,54 +804,28 @@ static char *get_author(const char *message)/* Read author-script and return an ident line (author <email> timestamp) */ static const char *read_author_ident (struct strbuf *buf) { -const char *keys[] = { -"GIT_AUTHOR_NAME=" ,"GIT_AUTHOR_EMAIL=" ,"GIT_AUTHOR_DATE=" - };struct strbuf out =STRBUF_INIT ; -char *in, *eol; -const char *val[3 ]; -int i =0 ; +char *name, *email, *date; -if (strbuf_read_file (buf,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return NULL ; -/* dequote values and construct ident line in-place */ -for (in = buf->buf; i <3 && in - buf->buf < buf->len; i++) { -if (!skip_prefix (in, keys[i], (const char **)&in)) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } - - eol =strchrnul (in,'\n' ); - *eol ='\0' ; -if (!sq_dequote (in)) { -warning (_ ("bad quoting on %s value in '%s'" ), - keys[i],rebase_path_author_script ()); -return NULL ; - } - val[i] = in; - in = eol +1 ; - } - -if (i <3 ) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } -/* validate date since fmt_ident() will die() on bad value */ -if (parse_date (val[2 ], &out)){ +if (parse_date (date, &out)){warning (_ ("invalid date format '%s' in '%s'" ), - val[2 ],rebase_path_author_script ()); + date,rebase_path_author_script ());strbuf_release (&out);return NULL ; }strbuf_reset (&out); -strbuf_addstr (&out,fmt_ident (val[0 ], val[1 ], val[2 ],0 )); +strbuf_addstr (&out,fmt_ident (name, email, date,0 ));strbuf_swap (buf, &out);strbuf_release (&out); +free (name); +free (email); +free (date);return buf->buf; }
Signature · 1 line
2.19.1
Quoted 4 lines · Phillip Wood
From: Phillip Wood <[email protected]>
Thanks to Junio for the feedback on v2. I've updated patch 4 based on those comments, the rest are unchanged.
Hmph, all these five patches seem to be identical to what I have in 'pu'. Did you send the right version?
Quoted 18 lines
v1 cover letter:
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (5):
am: don't die inread_author_script()
am: improve author-script error reporting
am: renameread_author_script()
addread_author_script()to libgit
sequencer: useread_author_script()builtin/am.c | 60 ++-------------- sequencer.c | 192 ++++++++++++++++++++++++++++++++------------------- sequencer.h | 3 + 3 files changed, 128 insertions(+), 127 deletions(-)
Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Phillip Wood
From: Phillip Wood <[email protected]>
Thanks to Junio for the feedback on v2. I've updated patch 4 based on those comments, the rest are unchanged.
Hmph, all these five patches seem to be identical to what I have in 'pu'. Did you send the right version?
Oh dear that's embarrassing. I updated the patches on my laptop and forget to sync before sending them from my desktop. I'll send v4 now.
Sorry for the confusion
Phillip
Quoted 19 lines
Quoted 19 lines
v1 cover letter:
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (5):
am: don't die inread_author_script()
am: improve author-script error reporting
am: renameread_author_script()
addread_author_script()to libgit
sequencer: useread_author_script()builtin/am.c | 60 ++-------------- sequencer.c | 192 ++++++++++++++++++++++++++++++++------------------- sequencer.h | 3 + 3 files changed, 128 insertions(+), 127 deletions(-)
v4 cover am/rebase: share read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Sorry for the confusion with v3, here are the updated patches.
Thanks to Junio for the feedback on v2. I've updated patch 4 based on those comments, the rest are unchanged.
v1 cover letter:
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (5):
am: don't die in read_author_script()
am: improve author-script error reporting
am: rename read_author_script()
add read_author_script() to libgit
sequencer: use read_author_script()
builtin/am.c | 60 ++-------------- sequencer.c | 192 ++++++++++++++++++++++++++++++++------------------- sequencer.h | 3 + 3 files changed, 128 insertions(+), 127 deletions(-)
Signature · 1 line
2.19.1
PATCH v4 1/5 am: don't die in read_author_script()
From: Phillip Wood <[email protected]>
The caller is already prepared to handle errors returned from this function so there is no need for it to die if it cannot read the file.
--- builtin/am.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/am.c +2 -1
@@ -318,7 +318,8 @@ static int read_author_script(struct am_state *state)if (fd <0 ) {if (errno ==ENOENT )return 0 ; -die_errno (_ ("could not open '%s' for reading" ), filename); +return error_errno (_ ("could not open '%s' for reading" ), + filename); }strbuf_read (&buf, fd,0 );close (fd);
Signature · 1 line
2.19.1
PATCH v4 2/5 am: improve author-script error reporting
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
If there are errors in a user edited author-script there was no indication of what was wrong. This commit adds some specific error messages depending on the problem. It also relaxes the requirement that the variables appear in a specific order in the file to match the behavior of 'rebase --interactive'.
--- builtin/am.c | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-)
builtin/am.c +39 -10
@@ -270,8 +270,11 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)struct string_list_item *item;char *np;char *cp =strchr (buf,'=' ); -if (!cp) -return -1 ; +if (!cp) { + np =strchrnul (buf,'\n' ); +return error (_ ("unable to parse '%.*s'" ), + (int ) (np - buf), buf); + } np =strchrnul (cp,'\n' ); *cp++ ='\0' ; item =string_list_append (list, buf);
@@ -280,7 +283,8 @@ static int parse_key_value_squoted(char *buf, struct string_list *list) *np ='\0' ; cp =sq_dequote (cp);if (!cp) -return -1 ; +return error (_ ("unable to dequote value of '%s'" ), + item->string); item->util =xstrdup (cp); }return 0 ;
@@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)struct strbuf buf =STRBUF_INIT ;struct string_list kv =STRING_LIST_INIT_DUP ;int retval =-1 ;/* assume failure */ +int i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ;int fd;assert (!state->author_name);
@@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)if (parse_key_value_squoted (buf.buf, &kv)) 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" )) +for (i =0 ; i < kv.nr; i++) { +if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { +if (name_i >=0 ) + name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); +else + name_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { +if (email_i >=0 ) + email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); +else + email_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { +if (date_i >=0 ) + date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); +else + date_i = i; + }else { + err =error (_ ("unknown variable '%s'" ), + kv.items[i].string); + } + } +if (name_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_NAME'" )); +if (email_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); +if (date_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_DATE'" )); +if (date_i <0 || email_i <0 || date_i <0 || err) goto finish; - state->author_name = kv.items[0 ].util; - state->author_email = kv.items[1 ].util; - state->author_date = kv.items[2 ].util; + state->author_name = kv.items[name_i].util; + state->author_email = kv.items[email_i].util; + state->author_date = kv.items[date_i].util; retval =0 ; finish:string_list_clear (&kv, !!retval);
Signature · 1 line
2.19.1
Re: [PATCH v4 2/5] am: improve author-script error reporting
Quoted 13 lines · Phillip Wood
builtin/am.c +10
-0@@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state) +int i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ;@@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state) +for (i =0 ; i < kv.nr; i++) { +if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { +if (name_i >=0 ) + name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); +else + name_i = i; + } + ... + }
builtin/am.c static int read_author_script(struct am_state *state) · line 349
Quoted 10 lines
builtin/am.c +6
-0+if (name_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_NAME'" )); + ... +if (date_i <0 || email_i <0 || date_i <0 || err) goto finish; + state->author_name = kv.items[name_i].util; + ... retval =0 ; finish:string_list_clear (&kv, !!retval);
Junio labeled the "-2" trick "cute", and while it is optimal in that it only traverses the key/value list once, it also increases cognitive load since the reader has to spend a good deal more brain cycles understanding what is going on than would be the case with simpler (and less noisily repetitive) code.
An alternative would be to make the code trivially easy to understand, though a bit more costly, but that expense should be negligible since this file should always be tiny, containing very few key/value pairs, and it's not timing critical code anyhow. For instance:
static char *find(struct string_list *kv, const char *key)
{
const char *val = NULL;
int i;
for (i = 0; i < kv.nr; i++) {
if (!strcmp(kv.items[i].string, key)) {
if (val) {
error(_("duplicate %s"), key);
return NULL;
}
val = kv.items[i].util;
}
}
if (!val)
error(_("missing %s"), key);
return val;
}static int read_author_script(struct am_state *state)
{
...
char *name, *email, *date;
...
name = find(&kv, "GIT_AUTHOR_NAME");
email = find(&kv, "GIT_AUTHOR_EMAIL");
date = find(&kv, "GIT_AUTHOR_DATE");
if (name && email && date) {
state->author_name = name;
state->author_email = email;
state->author_date = date;
retval = 0;
}
string_list_clear&kv, !!retval);
...Re: [PATCH v4 2/5] am: improve author-script error reporting
Quoted 5 lines · Eric Sunshine
Junio labeled the "-2" trick "cute", and while it is optimal in that it only traverses the key/value list once, it also increases cognitive load since the reader has to spend a good deal more brain cycles understanding what is going on than would be the case with simpler (and less noisily repetitive) code.
... and update three copies of very similar looking code that have to stay similar. If this were parsing unbounded and customizable set of variables, I think the approach you suggest to use a helper that iterates over the whole array for each key makes sense, but for now I think what was queued would be OK.
Quoted 39 lines
An alternative would be to make the code trivially easy to understand, though a bit more costly, but that expense should be negligible since this file should always be tiny, containing very few key/value pairs, and it's not timing critical code anyhow. For instance:
static char *find(struct
string_list*kv, const char *key){ const char *val = NULL; int i; for (i = 0; i < kv.nr; i++) { if (!strcmp(kv.items[i].string, key)) { if (val) { error(_("duplicate %s"), key); return NULL; } val = kv.items[i].util; } } if (!val) error(_("missing %s"), key); return val; }static int
read_author_script(struct am_state *state){ ... char *name, *email, *date; ... name = find(&kv, "GIT_AUTHOR_NAME"); email = find(&kv, "GIT_AUTHOR_EMAIL"); date = find(&kv, "GIT_AUTHOR_DATE"); if (name && email && date) { state->author_name = name; state->author_email = email; state->author_date = date; retval = 0; } string_list_clear&kv, !!retval); ...
PATCH v4 3/5 am: rename read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Rename read_author_script() in preparation for adding a shared read_author_script() function to libgit.
--- builtin/am.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/am.c +2 -2
@@ -306,7 +306,7 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)* script, and thus if the file differs from what this function expects, it is * better to bail out than to do something that the user does not expect. */ -static int read_author_script (struct am_state *state) +static int read_am_author_script (struct am_state *state) {const char *filename =am_path (state,"author-script" );struct strbuf buf =STRBUF_INIT ;
@@ -441,7 +441,7 @@ static void am_load(struct am_state *state)BUG ("state file 'last' does not exist" ); state->last =strtol (sb.buf,NULL ,10 ); -if (read_author_script (state) <0 ) +if (read_am_author_script (state) <0 )die (_ ("could not parse author script" ));read_commit_msg (state);
Signature · 1 line
2.19.1
PATCH v4 4/5 add read_author_script() to libgit
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
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.
---
Notes:
changes since v2:
- tweaked an error message as suggested by Junio
- fixed corner case where a key is given three times (Thanks to Junio for pointing this out)
changes since v1:
- added comment above read_author_script()
- rebased to reflect changes added in patch 2
builtin/am.c | 86 +---------------------------------------- sequencer.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ sequencer.h | 3 ++ 3 files changed, 110 insertions(+), 84 deletions(-)
builtin/am.c +2 -84
@@ -260,36 +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) { - np =strchrnul (buf,'\n' ); -return error (_ ("unable to parse '%.*s'" ), - (int ) (np - buf), buf); - } - 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 error (_ ("unable to dequote value of '%s'" ), - item->string); - 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.
@@ -309,65 +279,13 @@ 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 i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ; -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 ; -return error_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; - -for (i =0 ; i < kv.nr; i++) { -if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { -if (name_i >=0 ) - name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); -else - name_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { -if (email_i >=0 ) - email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); -else - email_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { -if (date_i >=0 ) - date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); -else - date_i = i; - }else { - err =error (_ ("unknown variable '%s'" ), - kv.items[i].string); - } - } -if (name_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_NAME'" )); -if (email_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); -if (date_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_DATE'" )); -if (date_i <0 || email_i <0 || date_i <0 || err) - goto finish; - state->author_name = kv.items[name_i].util; - state->author_email = kv.items[email_i].util; - state->author_date = kv.items[date_i].util; - retval =0 ; -finish: -string_list_clear (&kv, !!retval); -strbuf_release (&buf); -return retval; +return read_author_script (filename, &state->author_name, + &state->author_email, &state->author_date,1 ); }/**
sequencer.c +105 -0
111 lines not on this page · raw message
sequencer.h +3 -0
@@ -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
Signature · 1 line
2.19.1
PATCH v4 5/5 sequencer: use read_author_script()
In reply to Phillip WoodFrom: Phillip Wood <[email protected]>
Use the new function added in the last commit to read the author script, updating read_env_script() and read_author_ident(). We now have a single code path that reads the author script for am and all flavors of rebase. This changes the behavior of read_env_script() as previously it would set any environment variables that were in the author-script file. Now it is an error if the file contains other variables or any of GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE are missing. This is what am and the non interactive version of rebase have been doing for several years so hopefully it will not cause a problem for interactive rebase users. The advantage is that we are reusing existing code from am which uses sq_dequote() to properly dequote variables. This fixes potential problems with user edited scripts as read_env_script() which did not track quotes properly.
This commit also removes the fallback code for checking for a broken author script after git is upgraded when a rebase is stopped. Now that the parsing uses sq_dequote() it will reliably return an error if the quoting is broken and the user will have to abort the rebase and restart. This isn't ideal but it's a corner case and the detection of the broken quoting could be confused by user edited author scripts.
---
Notes:
changes since v1
- use argv_array_pushf() as suggested by Eric
- fixed strbuf handling as suggested by Eric
- fix comments and commit message to reflect changed behavior of read_env_script()
sequencer.c | 97 ++++++++++++----------------------------------------- 1 file changed, 21 insertions(+), 76 deletions(-)
sequencer.c +21 -76
@@ -767,53 +767,24 @@ int read_author_script(const char *path, char **name, char **email, char **date, }/* -* write_author_script() used to fail to terminate the last line with a "'" and -* also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". We check for -* the terminating "'" on the last line to see how "'" has been escaped in case -* git was upgraded while rebase was stopped. -*/ -static int quoting_is_broken (const char *s,size_t n) -{ -/* Skip any empty lines in case the file was hand edited */ -while (n >0 && s[--n] =='\n' ) - ;/* empty */ -if (n >0 && s[n] !='\'' ) -return 1 ; - -return 0 ; -} - -/* -* Read a list of environment variable assignments (such as the author-script -* file) into an environment block. Returns -1 on error, 0 otherwise. +* Read a GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL AND GIT_AUTHOR_DATE from a +* file with shell quoting into struct argv_array. Returns -1 on +* error, 0 otherwise. */ static int read_env_script (struct argv_array *env) { -struct strbuf script =STRBUF_INIT ; -int i, count =0 , sq_bug; -const char *p2; -char *p; +char *name, *email, *date; -if (strbuf_read_file (&script,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return -1 ; -/* write_author_script() used to quote incorrectly */ - sq_bug =quoting_is_broken (script.buf, script.len); -for (p = script.buf; *p; p++) -if (sq_bug &&skip_prefix (p,"'\\\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (skip_prefix (p,"'\\''" , &p2)) -strbuf_splice (&script, p - script.buf, p2 - p,"'" ,1 ); -else if (*p =='\'' ) -strbuf_splice (&script, p-- - script.buf,1 ,"" ,0 ); -else if (*p =='\n' ) { - *p ='\0' ; - count++; - } -for (i =0 , p = script.buf; i < count; i++) { -argv_array_push (env, p); - p +=strlen (p) +1 ; - } +argv_array_pushf (env,"GIT_AUTHOR_NAME=%s" , name); +argv_array_pushf (env,"GIT_AUTHOR_EMAIL=%s" , email); +argv_array_pushf (env,"GIT_AUTHOR_DATE=%s" , date); +free (name); +free (email); +free (date);return 0 ; }
@@ -833,54 +804,28 @@ static char *get_author(const char *message)/* Read author-script and return an ident line (author <email> timestamp) */ static const char *read_author_ident (struct strbuf *buf) { -const char *keys[] = { -"GIT_AUTHOR_NAME=" ,"GIT_AUTHOR_EMAIL=" ,"GIT_AUTHOR_DATE=" - };struct strbuf out =STRBUF_INIT ; -char *in, *eol; -const char *val[3 ]; -int i =0 ; +char *name, *email, *date; -if (strbuf_read_file (buf,rebase_path_author_script (),256 ) <=0 ) +if (read_author_script (rebase_path_author_script (), + &name, &email, &date,0 ))return NULL ; -/* dequote values and construct ident line in-place */ -for (in = buf->buf; i <3 && in - buf->buf < buf->len; i++) { -if (!skip_prefix (in, keys[i], (const char **)&in)) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } - - eol =strchrnul (in,'\n' ); - *eol ='\0' ; -if (!sq_dequote (in)) { -warning (_ ("bad quoting on %s value in '%s'" ), - keys[i],rebase_path_author_script ()); -return NULL ; - } - val[i] = in; - in = eol +1 ; - } - -if (i <3 ) { -warning (_ ("could not parse '%s' (looking for '%s')" ), -rebase_path_author_script (), keys[i]); -return NULL ; - } -/* validate date since fmt_ident() will die() on bad value */ -if (parse_date (val[2 ], &out)){ +if (parse_date (date, &out)){warning (_ ("invalid date format '%s' in '%s'" ), - val[2 ],rebase_path_author_script ()); + date,rebase_path_author_script ());strbuf_release (&out);return NULL ; }strbuf_reset (&out); -strbuf_addstr (&out,fmt_ident (val[0 ], val[1 ], val[2 ],0 )); +strbuf_addstr (&out,fmt_ident (name, email, date,0 ));strbuf_swap (buf, &out);strbuf_release (&out); +free (name); +free (email); +free (date);return buf->buf; }
Signature · 1 line
2.19.1
Quoted 6 lines · Phillip Wood
From: Phillip Wood <[email protected]>
Sorry for the confusion with v3, here are the updated patches.
Thanks to Junio for the feedback on v2. I've updated patch 4 based on those comments, the rest are unchanged.
The mistake of overwriting -1 (i.e. earlier we detected dup) with the third instance of the same originates at [2/5], so updating [4/5] without fixing it at its source would mean [4/5] is not a pure code movement to make it available to libgit users---it instead hides a (not so important) bugfix in it.
Quoted 20 lines
v1 cover letter:
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (5):
am: don't die inread_author_script()
am: improve author-script error reporting
am: renameread_author_script()
addread_author_script()to libgit
sequencer: useread_author_script()builtin/am.c | 60 ++-------------- sequencer.c | 192 ++++++++++++++++++++++++++++++++------------------- sequencer.h | 3 + 3 files changed, 128 insertions(+), 127 deletions(-)
PATCH 2/5 am: improve author-script error reporting
From: Phillip Wood <[email protected]>
If there are errors in a user edited author-script there was no indication of what was wrong. This commit adds some specific error messages depending on the problem. It also relaxes the requirement that the variables appear in a specific order in the file to match the behavior of 'rebase --interactive'.
--- builtin/am.c | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-)
builtin/am.c +39 -10
@@ -270,8 +270,11 @@ static int parse_key_value_squoted(char *buf, struct string_list *list)struct string_list_item *item;char *np;char *cp =strchr (buf,'=' ); -if (!cp) -return -1 ; +if (!cp) { + np =strchrnul (buf,'\n' ); +return error (_ ("unable to parse '%.*s'" ), + (int ) (np - buf), buf); + } np =strchrnul (cp,'\n' ); *cp++ ='\0' ; item =string_list_append (list, buf);
@@ -280,7 +283,8 @@ static int parse_key_value_squoted(char *buf, struct string_list *list) *np ='\0' ; cp =sq_dequote (cp);if (!cp) -return -1 ; +return error (_ ("unable to dequote value of '%s'" ), + item->string); item->util =xstrdup (cp); }return 0 ;
@@ -308,6 +312,7 @@ static int read_author_script(struct am_state *state)struct strbuf buf =STRBUF_INIT ;struct string_list kv =STRING_LIST_INIT_DUP ;int retval =-1 ;/* assume failure */ +int i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ;int fd;assert (!state->author_name);
@@ -326,14 +331,38 @@ static int read_author_script(struct am_state *state)if (parse_key_value_squoted (buf.buf, &kv)) 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" )) +for (i =0 ; i < kv.nr; i++) { +if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { +if (name_i !=-2 ) + name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); +else + name_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { +if (email_i !=-2 ) + email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); +else + email_i = i; + }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { +if (date_i !=-2 ) + date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); +else + date_i = i; + }else { + err =error (_ ("unknown variable '%s'" ), + kv.items[i].string); + } + } +if (name_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_NAME'" )); +if (email_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); +if (date_i ==-2 ) +error (_ ("missing 'GIT_AUTHOR_DATE'" )); +if (date_i <0 || email_i <0 || date_i <0 || err) goto finish; - state->author_name = kv.items[0 ].util; - state->author_email = kv.items[1 ].util; - state->author_date = kv.items[2 ].util; + state->author_name = kv.items[name_i].util; + state->author_email = kv.items[email_i].util; + state->author_date = kv.items[date_i].util; retval =0 ; finish:string_list_clear (&kv, !!retval);
Signature · 1 line
2.19.1-708-g4ede3d42df
PATCH 4/5 add read_author_script() to libgit
From: Phillip Wood <[email protected]>
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.
--- builtin/am.c | 86 +---------------------------------------- sequencer.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ sequencer.h | 3 ++ 3 files changed, 110 insertions(+), 84 deletions(-)
builtin/am.c +2 -84
@@ -260,36 +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) { - np =strchrnul (buf,'\n' ); -return error (_ ("unable to parse '%.*s'" ), - (int ) (np - buf), buf); - } - 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 error (_ ("unable to dequote value of '%s'" ), - item->string); - 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.
@@ -309,65 +279,13 @@ 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 i, name_i =-2 , email_i =-2 , date_i =-2 , err =0 ; -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 ; -return error_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; - -for (i =0 ; i < kv.nr; i++) { -if (!strcmp (kv.items[i].string,"GIT_AUTHOR_NAME" )) { -if (name_i !=-2 ) - name_i =error (_ ("'GIT_AUTHOR_NAME' already given" )); -else - name_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_EMAIL" )) { -if (email_i !=-2 ) - email_i =error (_ ("'GIT_AUTHOR_EMAIL' already given" )); -else - email_i = i; - }else if (!strcmp (kv.items[i].string,"GIT_AUTHOR_DATE" )) { -if (date_i !=-2 ) - date_i =error (_ ("'GIT_AUTHOR_DATE' already given" )); -else - date_i = i; - }else { - err =error (_ ("unknown variable '%s'" ), - kv.items[i].string); - } - } -if (name_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_NAME'" )); -if (email_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_EMAIL'" )); -if (date_i ==-2 ) -error (_ ("missing 'GIT_AUTHOR_DATE'" )); -if (date_i <0 || email_i <0 || date_i <0 || err) - goto finish; - state->author_name = kv.items[name_i].util; - state->author_email = kv.items[email_i].util; - state->author_date = kv.items[date_i].util; - retval =0 ; -finish: -string_list_clear (&kv, !!retval); -strbuf_release (&buf); -return retval; +return read_author_script (filename, &state->author_name, + &state->author_email, &state->author_date,1 ); }/**
sequencer.c +105 -0
111 lines not on this page · raw message
sequencer.h +3 -0
@@ -114,4 +114,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
Signature · 1 line
2.19.1-708-g4ede3d42df
Hi Junio
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Phillip Wood
From: Phillip Wood <[email protected]>
Sorry for the confusion with v3, here are the updated patches.
Quoted 8 lines
Quoted 2 lines
Thanks to Junio for the feedback on v2. I've updated patch 4 based on those comments, the rest are unchanged.
The mistake of overwriting -1 (i.e. earlier we detected dup) with the third instance of the same originates at [2/5], so updating [4/5] without fixing it at its source would mean [4/5] is not a pure code movement to make it available to libgit users---it instead hides a (not so important) bugfix in it.
Facepalm. Thanks for clearing up my mess, what you've got in pu looks fine to me.
Thanks
Phillip
Quoted 21 lines
Quoted 21 lines
v1 cover letter:
This is a follow up to pw/rebase-i-author-script-fix, it reduces code duplication and improves rebase's parsing of the author script. After this I'll do another series to share the code to write the author script.
Phillip Wood (5):
am: don't die inread_author_script()
am: improve author-script error reporting
am: renameread_author_script()
addread_author_script()to libgit
sequencer: useread_author_script()builtin/am.c | 60 ++-------------- sequencer.c | 192 ++++++++++++++++++++++++++++++++------------------- sequencer.h | 3 + 3 files changed, 128 insertions(+), 127 deletions(-)