Re: [PATCH 2/6] Introduce commit notes

[Junio C Hamano <gitster@pobox.com>]

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

> +core.notesRef::
> +	When showing commit messages, also show notes which are stored in
> +	the given ref.  This ref is expected to contain paths of the form
> +	??/*, where the directory name consists of the first two
> +	characters of the commit name, and the base name consists of
> +	the remaining 38 characters.
> ++
> +If such a path exists in the given ref, the referenced blob is read, and
> +appended to the commit message, separated by a "Notes:" line.  If the
> +given ref itself does not exist, it is not an error, but means that no
> +notes should be print.
> ++
> +This setting defaults to "refs/notes/commits", and can be overridden by
> +the `GIT_NOTES_REF` environment variable.
> +

This design forces "one blob and only one blob decorates a
commit".  It certainly makes the implementation and semantics
simpler -- if I have this note and you have that note on the
same commit, comparing notes eventually should result in a merge
of our notes.  But is it sufficient in real life usage scenarios
(what's the use case)?  One example that was raised on the list
is to collect "Acked-by", "Tested-by", etc., and in that case
perhaps one set "refs/notes/acks" may hold the former while
"refs/notes/tests" the latter.  If we wanted to show both at the
same time, is it the only option to put them in the same "note"
blob and not use "refs/notes/{acks,tests}"?

> diff --git a/commit.c b/commit.c
> index 0c350bc..3529b6a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -6,6 +6,7 @@
>  #include "interpolate.h"
>  #include "diff.h"
>  #include "revision.h"
> +#include "notes.h"
>  
>  int save_commit_buffer = 1;
>  
> @@ -1254,6 +1255,10 @@ unsigned long pretty_print_commit(enum cmit_fmt fmt,
>  	 */
>  	if (fmt == CMIT_FMT_EMAIL && offset <= beginning_of_body)
>  		buf[offset++] = '\n';
> +
> +	if (fmt != CMIT_FMT_ONELINE)
> +		get_commit_notes(commit, buf_p, &offset, space_p);
> +
>  	buf[offset] = '\0';
>  	free(reencoded);
>  	return offset;

This makes me wonder if there are cases where "notes" need to be
reencoded to honor log_output_encoding.

Since more and more people live in UTF-8 only world, and this is
a _new_ feature anyway, we could declare that "notes" blobs MUST
be encoded in UTF-8 upfront, but even if we did so we would need
reencoding to log_output_encoding, I suspect.

> diff --git a/notes.c b/notes.c
> new file mode 100644
> index 0000000..5d1bb1a
> --- /dev/null
> +++ b/notes.c
> @@ -0,0 +1,64 @@
> +#include "cache.h"
> +#include "commit.h"
> +#include "notes.h"
> +#include "refs.h"
> +
> +static int initialized;
> +
> +void get_commit_notes(const struct commit *commit,
> +		char **buf_p, unsigned long *offset_p, unsigned long *space_p)
> +{
> +	char name[80];
> +	const char *hex;
> +	unsigned char sha1[20];
> +	char *msg;
> +	unsigned long msgoffset, msglen;
> +	enum object_type type;
> +
> +	if (!initialized) {
> +		const char *env = getenv(GIT_NOTES_REF);
> +		if (env) {
> +			if (notes_ref_name)
> +				free(notes_ref_name);
> +			notes_ref_name = xstrdup(getenv(GIT_NOTES_REF));

	xstrdup(env)?

> +		} else if (!notes_ref_name)
> +			notes_ref_name = xstrdup("refs/notes/commits");

We would probably want to give another preprocessor constant for
this hardcoded string, next to GIT_NOTES_REF definition in cache.h.

> +		if (notes_ref_name && read_ref(notes_ref_name, sha1)) {
> +			free(notes_ref_name);
> +			notes_ref_name = NULL;
> +		}
> +		initialized = 1;
> +	}
> +	if (!notes_ref_name)
> +		return;
> +
> +	hex = sha1_to_hex(commit->object.sha1);
> +	snprintf(name, sizeof(name), "%s:%.*s/%.*s",
> +			notes_ref_name, 2, hex, 38, hex + 2);

Too long a notes_ref_name and it won't overrun the buffer but
the failure is not detected, and ...

> +	if (get_sha1(name, sha1))
> +		return;

... this would fail silently, leaving the user scratching his head.

> +
> +	if (!(msg = read_sha1_file(sha1, &type, &msglen)) || !msglen)
> +		return;

What's in "type" at this point?  Having a tree there is not an
error?

> +	/* we will end the annotation by a newline anyway. */
> +	if (msg[msglen - 1] == '\n')
> +		msglen--;
> +	ALLOC_GROW(*buf_p, *offset_p + 14 + msglen, *space_p);
> +	*offset_p += sprintf(*buf_p + *offset_p, "\nNotes:\n");

Fourteen is because...

> +
> +	for (msgoffset = 0; msgoffset < msglen;) {
> +		int linelen = get_line_length(msg + msgoffset, msglen);
> +
> +		ALLOC_GROW(*buf_p, *offset_p + linelen + 6, *space_p);
> +		*offset_p += sprintf(*buf_p + *offset_p,
> +				"    %.*s", linelen, msg + msgoffset);

Six is because...

> +		msgoffset += linelen;
> +	}
> +	ALLOC_GROW(*buf_p, *offset_p + 1, *space_p);
> +	(*buf_p)[*offset_p] = '\n';
> +	(*offset_p)++;
> +	free(msg);
> +}
> +
> +
> diff --git a/notes.h b/notes.h
> new file mode 100644
> index 0000000..aed80e7
> --- /dev/null
> +++ b/notes.h
> @@ -0,0 +1,9 @@
> +#ifndef NOTES_H
> +#define NOTES_H
> +
> +void get_commit_notes(const struct commit *commit,
> +	char **buf_p, unsigned long *offset_p, unsigned long *space_p);
> +
> +#define GIT_NOTES_REF "GIT_NOTES_REF"

Judging from the existing entries in cache.h, it seems that
GIT_NOTES_REF_ENVIRONMENT would be more appropriate preprocessor
symbol for this.  Also let's have this in cache.h next to
GIT_DIR_ENVIRONMENT and friends, with another definition for
"refs/notes/commits".