Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:
> 0. You can merge this into `master` resolving the conflicts: it's a
> fairly straightforward resolution. As soon as you publish the new
> `master`, I can continue working on `rr/sequencer`.
> 1. I can post the `rr/revert-cherry-pick` to the list, hoping that it
> will make it to `master` without more disruptions. I can rebase
> `rr/sequencer` on this new series and continue working. For your
> reference, this [1] is what I'll be posting to the list if we pick
> this option.
Merging
https://github.com/artagnon/git.git rr/revert-cherry-pick ;# 1b7c2632
and merging rr/revert-cherry-pick currently in 'pu' (6a156fd) to 'master'
results in identical trees, so I think these amount to the same thing.
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.
* 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"?
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.
The last test you added to 3510 in this patch runs test_line_count
unconditionally, by the way.
* 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.
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.
--
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