Re: [PATCH v2] commit.c: Replace starts_with() with skip_prefix()
On Wed, Mar 5, 2014 at 9:06 AM, Karthik Nayak karthik@gmail.com wrote:
Replaces all instances of starts_with() by skip_prefix(),
Use imperative mode: Replace all...
which can not only be used to check presence of a prefix,
but also used further on as it returns the string after the prefix,
if the prefix is present.
This is a lot better than previous versions. It could be improved a
bit by more directly stating that skip_prefix() singly does what the
current code is doing in two steps. (See Tanay's submission [1] for an
example of a well-crafted commit message). However, we're probably at
the point of diminishing returns, so no need to reroll just for this.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243395
Signed-off-by: Karthik Nayak karthik@gmail.com
---
Hey Eric,
Here are the changes i have made in this Patch v2:
1. Edited the variables names to fit their usage
2. set my emacs to indent 8 tabs, so proper indentation
3. fixed my error where i increased the value by 1 in parse_signed_commit().
Thanks again for your patience.
Thanks for the reroll and for explaining the changes. More below.
---
commit.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..f006490 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 *buf_split;
In a previous review, I suggested reading Junio's response [1] to a
similar submission. Of particular interest, Junio says:
A good rule of thumb to remember is to name things after what
they are, not how you obtain them, how they are used or what
they are used for/as.
The name 'buf_split' is clearly a how you obtain them, which does
not convey much. Better would be to name the variable to reflect what
it references once assigned.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/243231/focus=243259
if (!commit-buffer) {
unsigned long size;
@@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab
*author_date,
return;
}
+ buf_split = 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_split) {
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, buf_split,
+line_end - buf_split) ||
!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 *line_split;
Ditto.
if (!buffer || type != OBJ_COMMIT)
goto cleanup;
@@ -,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
char *next = memchr(line, '\n', tail - line);
next = next ? next + 1 : tail;
+ line_split = skip_prefix(line, gpg_sig_header);
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 (line_split line_split[0] == ' ')
+ sig = line_split + 1;
A shortcoming of this change is that skip_prefix() is now being called
unconditionally, even when its result won't be used (as in the first
'if' statement). The original code did the work of checking the prefix
only if the first 'if' statement evaluated to false.
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 +
[PATCH v2] commit.c: Replace starts_with() with skip_prefix()
Replaces all instances of starts_with() by skip_prefix(),
which can not only be used to check presence of a prefix,
but also used further on as it returns the string after the prefix,
if the prefix is present.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
Hey Eric,
Here are the changes i have made in this Patch v2:
1. Edited the variables names to fit their usage
2. set my emacs to indent 8 tabs, so proper indentation
3. fixed my error where i increased the value by 1 in parse_signed_commit().
Thanks again for your patience.
---
commit.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..f006490 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 *buf_split;
if (!commit-buffer) {
unsigned long size;
@@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab
*author_date,
return;
}
+ buf_split = 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_split) {
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, buf_split,
+line_end - buf_split) ||
!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 *line_split;
if (!buffer || type != OBJ_COMMIT)
goto cleanup;
@@ -,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
char *next = memchr(line, '\n', tail - line);
next = next ? next + 1 : tail;
+ line_split = skip_prefix(line, gpg_sig_header);
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 (line_split line_split[0] == ' ')
+ sig = line_split + 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);
+ if(!found) {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
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 v2] commit.c: Replace starts_with() with skip_prefix()
Hey Eric,
Sorry about not cc'ing you again , still figuring out git send-email.
On Wed, Mar 5, 2014 at 7:36 PM, Karthik Nayak karthik@gmail.com wrote:
Replaces all instances of starts_with() by skip_prefix(),
which can not only be used to check presence of a prefix,
but also used further on as it returns the string after the prefix,
if the prefix is present.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
Hey Eric,
Here are the changes i have made in this Patch v2:
1. Edited the variables names to fit their usage
2. set my emacs to indent 8 tabs, so proper indentation
3. fixed my error where i increased the value by 1 in parse_signed_commit().
Thanks again for your patience.
---
commit.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..f006490 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 *buf_split;
if (!commit-buffer) {
unsigned long size;
@@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab
*author_date,
return;
}
+ buf_split = 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_split) {
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, buf_split,
+line_end - buf_split) ||
!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 *line_split;
if (!buffer || type != OBJ_COMMIT)
goto cleanup;
@@ -,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
char *next = memchr(line, '\n', tail - line);
next = next ? next + 1 : tail;
+ line_split = skip_prefix(line, gpg_sig_header);
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 (line_split line_split[0] == ' ')
+ sig = line_split + 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);
+ if(!found) {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
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
