Thanks for the resend. Etiquette on this list is to cc: people who
commented on previous versions of the submission. As Tanay already
mentioned, use [PATCH vN] in the subject where N is the version number
of this attempt. The -v option of "git format-email" can help.
More below.
On Tue, Mar 4, 2014 at 10:54 AM, Karthik Nayak <karthik....@gmail.com> wrote:
> In record_author_date() :
> Replace "buf + strlen(author )" by skip_prefix(), which is
> saved in a new "const char" variable "indent_line".
>
> In parse_signed_commit() :
> Replace "line + gpg_sig_header_len" by skip_prefix(), which
> is saved in a new "const char" variable "indent_line".
>
> In parse_gpg_output() :
> Replace "buf + strlen(sigcheck_gpg_status[i].check + 1)" by
> skip_prefix(), which is saved in already available
> variable "found".
It's not necessary to explain in prose what the patch itself already
states more concisely and precisely. All of this text should be
dropped from the commit message. Instead, explain the purpose of the
patch, the problem it solves, etc. Please read the "(2) Describe your
changes well" section of Documentation/SubmittingPatches to get an
idea of what sort of information to include in the commit message.
A better commit message should say something about how the affected
functions want to know not only if the string has a prefix, but also
the text following the prefix, and that skip_prefix() can provide both
pieces of information.
> Signed-off-by: Karthik Nayak <karthik....@gmail.com>
> ---
> commit.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..71a03e3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -553,6 +553,7 @@ static void record_author_date(struct author_date_slab
> *author_date,
> struct ident_split ident;
> char *date_end;
> unsigned long date;
> + const char *indent_line;
Strange variable name. The code in question splits apart a person's
identification string (name, email, etc.). It has nothing to do with
indentation.
> if (!commit->buffer) {
> unsigned long size;
> @@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab
> *author_date,
> return;
> }
>
> + indent_line = skip_prefix(buf, "author ");
> +
> for (buf = commit->buffer ? commit->buffer : buffer;
> buf;
> buf = line_end + 1) {
> line_end = strchrnul(buf, '\n');
> - if (!starts_with(buf, "author ")) {
> + if (!indent_line) {
> if (!line_end[0] || line_end[1] == '\n')
> return; /* end of header */
> continue;
> }
> - if (split_ident_line(&ident,
> - buf + strlen("author "),
> - line_end - (buf + strlen("author "))) ||
> + if (split_ident_line(&ident, indent_line,
> + line_end -
> indent_line) ||
Indent the second line (using tabs plus possible spaces) so that it
lines up with the &ident in the line above. Be sure to set your editor
so tabs have width 8.
> !ident.date_begin || !ident.date_end)
> goto fail_exit; /* malformed "author" line */
> break;
> @@ -1098,6 +1100,7 @@ int parse_signed_commit(const unsigned char *sha1,
> char *buffer = read_sha1_file(sha1, &type, &size);
> int in_signature, saw_signature = -1;
> char *line, *tail;
> + const char *indent_line;
>
> if (!buffer || type != OBJ_COMMIT)
> goto cleanup;
> @@ -1111,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
> char *next = memchr(line, '\n', tail - line);
>
> next = next ? next + 1 : tail;
> + indent_line = skip_prefix(line, gpg_sig_header);
Even stranger variable name for a GPG signature (which has nothing at
all to do with indentation).
> if (in_signature && line[0] == ' ')
> sig = line + 1;
> - else if (starts_with(line, gpg_sig_header) &&
> - line[gpg_sig_header_len] == ' ')
> - sig = line + gpg_sig_header_len + 1;
> + else if (indent_line && indent_line[1] == ' ')
> + sig = indent_line + 2;
Why is this adding 2 rather than 1?
> if (sig) {
> strbuf_add(signature, sig, next - sig);
> saw_signature = 1;
> @@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check
> *sigc)
> for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> const char *found, *next;
>
> - if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
> - /* At the very beginning of the buffer */
> - found = buf + strlen(sigcheck_gpg_status[i].check +
> 1);
> - } else {
> + found = skip_prefix(buf, sigcheck_gpg_status[i].check +1);
Add a space after the plus sign.
> + if(!found) {
> found = strstr(buf, sigcheck_gpg_status[i].check);
'found' is being computed again here, even though you already computed
it just above via skip_prefix(). This seems pretty wasteful.
> if (!found)
> continue;
> --
> 1.9.0.138.g2de3478
>
> Hey Eric,
> As per your suggestion in the previous mail :
> http://article.gmane.org/gmane.comp.version-control.git/243316
> I have made necessary changes:
> 1. Better Naming of variables as per suggestion
> 2. Proper replacement of skip_prefix() over starts_with() .
Thanks for explaining the changes since the previous version and
providing a link. That's good etiquette.
As Tanay already mentioned in his review, place this commentary just
below the "---" line following your sign off. (You placed it below the
"--" line.)
--
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
