Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2011, 18:20 UTC
Message-ID: <7vliqph8a6.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0mpPoZJWviBesWgy2dZ4xJrNyhED2znFid8iGbSTirPhQ@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 8 quoted lines

> 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.