Re: [PATCH 2/2] shortlog: add '--sort-key' and '--sort-key-regexp' options

["Michał Kiedrowicz" <michal.kiedrowicz@gmail.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> 
> Instead of always sorting by author, allow to sort by either
> a whitespace-delimited field or by a regular expression (first group)
> on the oneline.
> 
> For example, this will give you an overview of the weekday/commit
> distribution:
> 
> 	git shortlog -k 1 --pretty=%cD -s -n
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	Pretty obviously, in git.git this shows that Saturday and
> 	Wednesday, your designated Git days, are on top of the list,
> but what is interesting to me is that the difference between
> Wednesday and Sunday is not all that much.
> 
> 	Sorry, no time for tests, maybe somebody else can help out?
> 
>  Documentation/git-shortlog.txt |   13 +++++++-
>  builtin-shortlog.c             |   64
> +++++++++++++++++++++++++++++++++++++--
> shortlog.h                     |    1 + 3 files changed, 73
> insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-shortlog.txt
> b/Documentation/git-shortlog.txt index 42463a9..3155361 100644
> --- a/Documentation/git-shortlog.txt
> +++ b/Documentation/git-shortlog.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  --------
>  [verse]
>  git log --pretty=short | 'git shortlog' [-h] [-n] [-s] [-e] [-w]
> -git shortlog [-n|--numbered] [-s|--summary] [-e|--email]
> [-w[<width>[,<indent1>[,<indent2>]]]] [<committish>...] +git shortlog
> [-n|--numbered] [-s|--summary] [-e|--email]
> [-w[<width>[,<indent1>[,<indent2>]]]] [(-K|--sort-key-regexp)
> <regexp>] [(-k|--sort-key) <n>] [<committish>...] DESCRIPTION
> ----------- @@ -45,6 +45,17 @@ OPTIONS
>  	and subsequent lines are indented by `indent2` spaces.
> `width`, `indent1`, and `indent2` default to 76, 6 and 9 respectively.
>  
> +-K <regexp>::
> +--sort-key-regexp <regexp>::
> +	Instead of sorting by author, sort by a regular expression
> on the
> +	commit subject (or whatever you specified using --pretty)
> +
> +-k <number>::
> +--sort-key <number>::
> +	Instead of sorting by author, sort by a given
> whitespace-delimited
> +	field of the commit subject (or whatever you specified using
> +	--pretty).  The first field is 1.
> +
>  
>  MAPPING AUTHORS
>  ---------------
> diff --git a/builtin-shortlog.c b/builtin-shortlog.c
> index a684422..2cab5e4 100644
> --- a/builtin-shortlog.c
> +++ b/builtin-shortlog.c
> @@ -145,9 +145,37 @@ static void read_from_stdin(struct shortlog *log)
>  
>  void shortlog_add_commit(struct shortlog *log, struct commit *commit)
>  {
> +	struct strbuf buf = STRBUF_INIT;
> +	char *key = NULL;
>  	const char *author = NULL, *buffer;
>  
>  	buffer = commit->buffer;
> +	if (log->user_format)
> +		pretty_print_commit(CMIT_FMT_USERFORMAT, commit,
> &buf,
> +			DEFAULT_ABBREV, "", "", DATE_NORMAL, 0);
> +
> +	if (log->sort_key) {
> +		const char *p = buf.buf;
> +		regmatch_t match[2];
> +
> +		if (!log->user_format) {
> +			p = strstr(buffer, "\n\n");
> +			if (!p)
> +				return;
> +			p += 2;
> +		}
> +
> +		if (!regexec(log->sort_key, p, 2, match, 0) &&
> +				match[1].rm_so >= 0)
> +			author = key = xstrndup(p + match[1].rm_so,
> +				match[1].rm_eo - match[1].rm_so);
> +		else
> +			author = "<null>";
> +		insert_one_record1(log, key, p);
> +		strbuf_release(&buf);

Missing free()?

> +		return;
> +	}
> +
>  	while (*buffer && *buffer != '\n') {
>  		const char *eol = strchr(buffer, '\n');
>  
> @@ -164,10 +192,6 @@ void shortlog_add_commit(struct shortlog *log,
> struct commit *commit) die("Missing author: %s",
>  		    sha1_to_hex(commit->object.sha1));
>  	if (log->user_format) {
> -		struct strbuf buf = STRBUF_INIT;
> -
> -		pretty_print_commit(CMIT_FMT_USERFORMAT, commit,
> &buf,
> -			DEFAULT_ABBREV, "", "", DATE_NORMAL, 0);
>  		insert_one_record(log, author, buf.buf);
>  		strbuf_release(&buf);
>  		return;
> @@ -175,6 +199,7 @@ void shortlog_add_commit(struct shortlog *log,
> struct commit *commit) if (*buffer)
>  		buffer++;
>  	insert_one_record(log, author, !*buffer ? "<none>" : buffer);
> +	free(key);

Is it the right place for free()?

Here is valgrind output:

==9005== 98,085 bytes in 19,218 blocks are definitely lost in loss record 23 of 25
==9005==    at 0x4A08CFE: malloc (vg_replace_malloc.c:207
==9005==    by 0x4A8D25: xmalloc (wrapper.c:20)
==9005==    by 0x4A8DE1: xmemdupz (wrapper.c:45)
==9005==    by 0x44AFDD: shortlog_add_commit (builtin-shortlog.c:170)
==9005==    by 0x44B31C: cmd_shortlog (builtin-shortlog.c:212)
==9005==    by 0x4049A2: handle_internal_command (git.c:246)
==9005==    by 0x404B63: main (git.c:438)