Hi Junio,
Junio C Hamano wrote:
> A few comments and thoughts, all minor.
>
> * On "revert: simplify getting commit subject in format_todo()"
>
> The old code used to use get_message() on cur->item, checked its return
> value (if cur->item is not parsed, or has already been used and its buffer
> cleared, cur->item->buffer would be NULL and get_message() returns -1) and
> issued an error. The new code uses find_commit_subject without first
> checking if cur->item->buffer is NULL.
>
> Does this mean the old code was overly defensive, or is the new code too
> lax?
>
> I understand that parse_insn_line() uses lookup_commit_reference() which
> calls parse_object() on the object name (and if it is a tag, deref_tag()
> will parse the tagged object until we see something that is not a tag), so
> we know cur->item is parsed before we see it, so I suspect you merely were
> being overly defensive, but I may be missing something.
Right. Actually, I was being overly defensive because I was being
lazy about having to deal the an empty-commit-subject case in the
parser. With "revert: make commit subjects in insn sheet optional",
that's no more a concern- I'll reorder these two patches, and explain
this detail in the commit message in the re-roll.
> * On "revert: make commit subjects in insn sheet optional"
>
> After finding the verb and advancing the pointer "bol" at the beginning of
> the object name, end_of_object_name variable points at the first SP or LF
> using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
> hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
> we can allow something like "pick rr/revert-cherry-pick~3"?
Yes, it is exactly for that reason :)
> I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
> instead, i.e. allow users with fat fingers to use one or more SP or even HT
> to separate the verb and the operand.
Hm, I'm not too enthusiastic about this change, because we don't
advertise hand-editing the instruction sheet yet: it's just some extra
parsing cruft along with guardian buffer-overflow code that buys us no
immediate benefits.
> The last test you added to 3510 in this patch runs test_line_count
> unconditionally, by the way.
Good catch. Missing the chaining '&&' seems to be quite a common
error: I vaguely remember seeing a patch to detect this sometime ago.
Jonathan?
> * On "revert: allow mixed pick and revert instructions"
>
> Reporting what we did not understand from parse_insn_line() is a good
> idea, but I think the line number should be reported at the beginning
> of the same line.
Makes sense. Do you like this?
diff --git a/builtin/revert.c b/builtin/revert.c
index e447449..cfa770e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -715,7 +715,8 @@ static int format_todo(struct strbuf *buf, struct
replay_insn_list *todo_list)
return 0;
}
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+ struct replay_insn_list *item, int lineno)
{
unsigned char commit_sha1[20];
char *end_of_object_name;
@@ -731,7 +732,8 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
size_t len = strchrnul(bol, '\n') - bol;
if (len > 255)
len = 255;
- return error(_("Unrecognized action: %.*s"), (int)len, bol);
+ return error(_("Line %d: Unrecognized action: %.*s"),
+ lineno, (int)len, bol);
}
end_of_object_name = bol + strcspn(bol, " \n");
@@ -741,11 +743,11 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
*end_of_object_name = saved;
if (status < 0)
- return error(_("Malformed object name: %s"), bol);
+ return error(_("Line %d: Malformed object name: %s"), lineno, bol);
item->operand = lookup_commit_reference(commit_sha1);
if (!item->operand)
- return error(_("Not a valid commit: %s"), bol);
+ return error(_("Line %d: Not a valid commit: %s"), lineno, bol);
item->next = NULL;
return 0;
@@ -760,8 +762,8 @@ static int parse_insn_buffer(char *buf, struct
replay_insn_list **todo_list)
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- if (parse_insn_line(p, eol, &item) < 0)
- return error(_("on line %d."), i);
+ if (parse_insn_line(p, eol, &item, lineno) < 0)
+ return -1;
next = replay_insn_list_append(item.action, item.operand, next);
p = *eol ? eol + 1 : eol;
}
--
> I'd say that I'd prefer queuing re-rolled patches posted on the list; I'll
> discard the rr/revert-cherry-pick (6a156fd) from my tree.
Thanks for letting me know -- will post shortly.
-- Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html