Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo
On Thu, May 28, 2015 at 4:44 AM, Junio C Hamano gits...@pobox.com wrote:
Paul Tan pyoka...@gmail.com writes:
@@ -17,6 +34,10 @@ struct am_state {
struct strbuf dir;/* state directory path */
int cur; /* current patch number */
int last; /* last patch number */
+ struct strbuf msg;/* commit message */
+ struct strbuf author_name;/* commit author's name */
+ struct strbuf author_email; /* commit author's email */
+ struct strbuf author_date;/* commit author's date */
int prec; /* number of digits in patch filename */
};
I always get suspicious when structure fields are overly commented,
wondering if it is a sign of naming fields poorly. All of the above
fields look quite self-explanatory and I am not sure if it is worth
having these comments, spending efforts to type SP many times to
align them and all.
Okay, I'll take Jeff's suggestion to organize them into blocks.
+static int read_author_script(struct am_state *state)
+{
+ char *value;
+ struct strbuf sb = STRBUF_INIT;
+ const char *filename = am_path(state, author-script);
+ FILE *fp = fopen(filename, r);
+ if (!fp) {
+ if (errno == ENOENT)
+ return 0;
+ die(_(could not open '%s' for reading), filename);
Hmph, do we want to report with die_errno()?
Yes, we do.
+ }
+
+ if (strbuf_getline(sb, fp, '\n'))
+ return -1;
+ if (!skip_prefix(sb.buf, GIT_AUTHOR_NAME=, (const char**) value))
This cast is unfortunate; can't value be of const char * type?
We can't, because sq_dequote() modifies the string directly. I would
think casting from non-const to const is safer than the other way
around.
Thanks,
Paul
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo
On Thu, May 28, 2015 at 6:13 AM, Junio C Hamano gits...@pobox.com wrote:
Paul Tan pyoka...@gmail.com writes:
+static const char *msgnum(const struct am_state *state)
+{
+ static struct strbuf fmt = STRBUF_INIT;
+ static struct strbuf sb = STRBUF_INIT;
+
+ strbuf_reset(fmt);
+ strbuf_addf(fmt, %%0%dd, state-prec);
+
+ strbuf_reset(sb);
+ strbuf_addf(sb, fmt.buf, state-cur);
Hmph, wouldn't (%*d, state-prec, state-cur) work or am I missing
something?
Yeah, I think it would. I justs didn't know about the existence of '*'.
Thanks,
Paul
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo
Paul Tan pyoka...@gmail.com writes:
@@ -17,6 +34,10 @@ struct am_state {
struct strbuf dir;/* state directory path */
int cur; /* current patch number */
int last; /* last patch number */
+ struct strbuf msg;/* commit message */
+ struct strbuf author_name;/* commit author's name */
+ struct strbuf author_email; /* commit author's email */
+ struct strbuf author_date;/* commit author's date */
int prec; /* number of digits in patch filename */
};
I always get suspicious when structure fields are overly commented,
wondering if it is a sign of naming fields poorly. All of the above
fields look quite self-explanatory and I am not sure if it is worth
having these comments, spending efforts to type SP many times to
align them and all.
By the way, the overall structure of the series look sensible.
+static int read_author_script(struct am_state *state)
+{
+ char *value;
+ struct strbuf sb = STRBUF_INIT;
+ const char *filename = am_path(state, author-script);
+ FILE *fp = fopen(filename, r);
+ if (!fp) {
+ if (errno == ENOENT)
+ return 0;
+ die(_(could not open '%s' for reading), filename);
Hmph, do we want to report with die_errno()?
+ }
+
+ if (strbuf_getline(sb, fp, '\n'))
+ return -1;
+ if (!skip_prefix(sb.buf, GIT_AUTHOR_NAME=, (const char**) value))
This cast is unfortunate; can't value be of const char * type?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo
Paul Tan pyoka...@gmail.com writes:
+static const char *msgnum(const struct am_state *state)
+{
+ static struct strbuf fmt = STRBUF_INIT;
+ static struct strbuf sb = STRBUF_INIT;
+
+ strbuf_reset(fmt);
+ strbuf_addf(fmt, %%0%dd, state-prec);
+
+ strbuf_reset(sb);
+ strbuf_addf(sb, fmt.buf, state-cur);
Hmph, wouldn't (%*d, state-prec, state-cur) work or am I missing
something?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo
On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano wrote:
Paul Tan pyoka...@gmail.com writes:
@@ -17,6 +34,10 @@ struct am_state {
struct strbuf dir;/* state directory path */
int cur; /* current patch number */
int last; /* last patch number */
+ struct strbuf msg;/* commit message */
+ struct strbuf author_name;/* commit author's name */
+ struct strbuf author_email; /* commit author's email */
+ struct strbuf author_date;/* commit author's date */
int prec; /* number of digits in patch filename */
};
I always get suspicious when structure fields are overly commented,
wondering if it is a sign of naming fields poorly. All of the above
fields look quite self-explanatory and I am not sure if it is worth
having these comments, spending efforts to type SP many times to
align them and all.
Just my 2 cents, but I think that grouping and overhead comments can
often make things more obvious. For example:
struct am_state {
/* state directory path */
struct strbuf dir;
/*
* current and last patch numbers; are these 1-indexed
* or 0-indexed?
*/
int cur;
int last;
struct strbuf author_name;
struct strbuf author_email;
struct strbuf author_date;
struct strbuf msg;
/* number of digits in patch filename */
int prec;
}
Note that by grouping cur and last, we can discuss them as a group,
and the overhead comment gives us room to mention their shared
properties (my indexing question is a real question, btw, whose answer I
think would be useful to mention in a comment).
Likewise, by grouping the msg strbuf with the author information, it
becomes much more clear that they are all about the commit metadata
(though I would not be opposed to a comment above the whole block,
either).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
