Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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".
-
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
