Re: [PATCH] commit.c: Replace starts_with() with skip_prefix()
Thanks for the submission. Some commentary below to expose you to the
review process on this project...
On Mon, Mar 3, 2014 at 2:47 AM, Karthik Nayak karthik@gmail.com wrote:
Replace with skip_prefix(), which uses the inbuilt function
strcmp() to compare.
Explaining the purpose of the change is indeed important, however,
this description misses the mark. See below.
Other Places were this can be implemented:
commit.c : line 1117
commit.c : line 1197
Bonus points if you actually follow through with this.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
commit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..1e181cf 100644
--- a/commit.c
+++ b/commit.c
@@ -552,6 +552,7 @@ static void record_author_date(struct author_date_slab
*author_date,
char *buffer = NULL;
struct ident_split ident;
char *date_end;
+ char *flag_sp; /* stores return value of skip_prefix() */
It's not clear why this variable is needed. It's only assigned (below)
but never referenced. Also, the name conveys little or no meaning. If
you need a comment to explain the purpose of the variable, that's
probably a good indication that it's poorly named.
unsigned long date;
if (!commit-buffer) {
@@ -566,7 +567,7 @@ static void record_author_date(struct author_date_slab
*author_date,
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ if ((flag_sp = skip_prefix(buf, author )) == NULL) {
Unfortunately, this change makes the code more difficult to read.
Let's review the GSoC microproject:
Rewrite commit.c:record_author_date() to use skip_prefix().
Are there other places in this file where skip_prefix() would
be **more readable** than starts_with()?
Note the part I **highlighted**. This is a good clue that use of
skip_prefix() can be used to simplify the code. Examine
record_author_date() more carefully and see if you can identify how to
do so.
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
--
1.9.0.138.g2de3478
--
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] commit.c: Replace starts_with() with skip_prefix()
Hello Eric,
Thanks for Pointing out everything, i had a thorough look and fixed a
couple of things.
Here is an Updated Patch.
- Removed unnecessary code and variables.
- Replaced all instances of starts_with() with skip_prefix()
Replace starts_with() with skip_prefix() for better reading purposes.
Also to replace buf + strlen(author ) by skip_prefix(), which is
saved in a new const char variable buf_skipprefix.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
commit.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..e5dc2e2 100644
--- a/commit.c
+++ b/commit.c
@@ -552,6 +552,7 @@ static void record_author_date(struct
author_date_slab *author_date,
char *buffer = NULL;
struct ident_split ident;
char *date_end;
+ const char *buf_skipprefix;
unsigned long date;
if (!commit-buffer) {
@@ -562,18 +563,20 @@ static void record_author_date(struct
author_date_slab *author_date,
return;
}
+ buf_skipprefix = 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 (!buf_skipprefix) {
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 ))) ||
+ buf_skipprefix,
+ line_end - buf_skipprefix) ||
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
@@ -1113,7 +1116,7 @@ int parse_signed_commit(const unsigned char *sha1,
next = next ? next + 1 : tail;
if (in_signature line[0] == ' ')
sig = line + 1;
- else if (starts_with(line, gpg_sig_header)
+ else if (skip_prefix(line, gpg_sig_header)
line[gpg_sig_header_len] == ' ')
sig = line + gpg_sig_header_len + 1;
if (sig) {
@@ -1193,7 +1196,7 @@ 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)) {
+ if (skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) {
/* At the very beginning of the buffer */
found = buf + strlen(sigcheck_gpg_status[i].check + 1);
} else {
--
1.9.0.138.g2de3478
--
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] commit.c: Replace starts_with() with skip_prefix()
On Mon, Mar 3, 2014 at 10:23 AM, karthik nayak karthik@gmail.com wrote:
Hello Eric,
Thanks for Pointing out everything, i had a thorough look and fixed a
couple of things.
Here is an Updated Patch.
- Removed unnecessary code and variables.
- Replaced all instances of starts_with() with skip_prefix()
This commentary is important for the on-going email discussion but
does not belong in the commit message for the permanent project
history, so place it below the --- line just under your sign-off.
Explaining what changed since the last attempt, as you do here, is a
good thing. To further ease the review process, supply a link to the
previous attempt, like this [1].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243194
Replace starts_with() with skip_prefix() for better reading purposes.
Also to replace buf + strlen(author ) by skip_prefix(), which is
saved in a new const char variable buf_skipprefix.
The second sentence is really the meat of this change, and not at all
incidental as Also implies. You should use it (or a refinement of
it) as your commit message. The first sentence doesn't particularly
add much and can easily be dropped.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
commit.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..e5dc2e2 100644
--- a/commit.c
+++ b/commit.c
@@ -552,6 +552,7 @@ static void record_author_date(struct
author_date_slab *author_date,
char *buffer = NULL;
struct ident_split ident;
char *date_end;
+ const char *buf_skipprefix;
Read this response by Junio [2] to another GSoC candidate which
explains why this is a poor variable name and how to craft a better
one.
[2]: http://thread.gmane.org/gmane.comp.version-control.git/243231/focus=243259
unsigned long date;
if (!commit-buffer) {
@@ -562,18 +563,20 @@ static void record_author_date(struct
author_date_slab *author_date,
return;
}
+ buf_skipprefix = skip_prefix(buf, author );
Unfortunately, your patch is badly whitespace-damaged as if it was
pasted into the mailer and mangled. (Your first submission did not
have this problem.) If you can, use git send-email to avoid such
corruption.
+
for (buf = commit-buffer ? commit-buffer : buffer;
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ if (!buf_skipprefix) {
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 ))) ||
+ buf_skipprefix,
+ line_end - buf_skipprefix) ||
Looks reasonable (sans whitespace damage).
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
@@ -1113,7 +1116,7 @@ int parse_signed_commit(const unsigned char *sha1,
next = next ? next + 1 : tail;
if (in_signature line[0] == ' ')
sig = line + 1;
- else if (starts_with(line, gpg_sig_header)
+ else if (skip_prefix(line, gpg_sig_header)
line[gpg_sig_header_len] == ' ')
sig = line + gpg_sig_header_len + 1;
if (sig) {
@@ -1193,7 +1196,7 @@ 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)) {
+ if (skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) {
/* At the very beginning of the buffer */
found = buf + strlen(sigcheck_gpg_status[i].check + 1);
} else {
For these two sets of changes, unless you are actually taking
advantage of the return value of skip_prefix(), the mere mechanical
replacement of starts_with() with skip_prefix() actually hurts
readability. Examine each of these cases more carefully to determine
whether skip_prefix() is in fact useful. If so, take advantage of it.
If not, explain in the patch commentary why skip_prefix() doesn't
help.
--
1.9.0.138.g2de3478
--
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
