Re: [PATCH v2 4/4] log: --author-date-order
- From
Jeff King <peff@peff.net>
- Date
- Jun 10, 2013, 05:50 UTC
- Message-ID
- <20130610055014.GF3621@sigill.intra.peff.net>
- In-Reply-To
- <1370820277-30158-5-git-send-email-gitster@pobox.com>
On Sun, Jun 09, 2013 at 04:24:37PM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> Sometimes people would want to view the commits in parallel > histories in the order of author dates, not committer dates. > > Teach "topo-order" sort machinery to do so, using a commit-info slab > to record the author dates of each commit, and commit-queue to sort > them.
Nice, this is basically what I was envisioning when I mentioned the slabs. However, I don't think the code works. :(
Show 28 quoted lines
> +static void record_author_date(struct author_date_slab *author_date,
> + struct commit *commit)
> +{
> + const char *buf, *line_end;
> + struct ident_split ident;
> + char *date_end;
> + unsigned long date;
> +
> + for (buf = commit->buffer; buf; buf = line_end + 1) {
> + line_end = strchrnul(buf, '\n');
> + if (prefixcmp(buf, "author ")) {
> + 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 "))) ||
> + !ident.date_begin || !ident.date_end)
> + return; /* malformed "author" line */
> + break;
> + }
> +
> + date = strtoul(ident.date_begin, &date_end, 10);
> + if (date_end != ident.date_end)
> + return; /* malformed date */
> + *(author_date_slab_at(author_date, commit)) = date;
> +}I'm not excited about introducing yet another place that parses commit objects (mostly not for correctness, but because we have had inconsistency in how malformed objects are treated). It is at least using split_ident_line which covers the hard bits. I wonder how much slower it would be to simply call format_commit_message to do the parsing.
Show 7 quoted lines
> /* Mark them and clear the indegree */
> for (next = orig; next; next = next->next) {
> struct commit *commit = next->item;
> *(indegree_slab_at(&indegree, commit)) = 1;
> + /* also record the author dates, if needed */
> + if (sort_order == REV_SORT_BY_AUTHOR_DATE)
> + record_author_date(&author_date, commit);The record_author_date function assumes that commit->buffer is valid (i.e., not NULL). We seem to assume that the commits are parsed already (for looking at parents, and at the committer date). But if "save_commit_buffer" is set to 0 (as it is for rev-list), we would not have a buffer at all.
It's hard to notice the problem because a NULL buffer will cause record_author_date to simply leave the slab entry at 0. That would give the same output as regular "--topo-order" (because everybody has the same timestamp), except that the priority queue heap is not stable. With this patch:
diff --git a/commit.c b/commit.c index f3a2f09..5e62ae8 100644 --- a/commit.c +++ b/commit.c @@ -521,6 +521,9 @@ static void record_author_date(struct author_date_slab *author_date, char *date_end; unsigned long date; + if (!commit->buffer) + die("whooops!"); + for (buf = commit->buffer; buf; buf = line_end + 1) { line_end = strchrnul(buf, '\n'); if (prefixcmp(buf, "author ")) { you can see the problem more clearly with "git rev-list --author-date-order HEAD". -Peff