Some PRETTY FORMATS format:<string> options don't work with git-archive export-subst $Format:$

[Derek Moore]

I first noticed this using the system git provided in Fedora 21, so I
cloned the official git repo, built from source, and it would appear
there are even more options that don't work with export-subst in the
latest code.

I tested this under commit 63a45136a329bab550425c3142db6071434d935e
(HEAD, origin/master, origin/HEAD, master).

Take, for example, the following script:

FILE=Foo.pm
DIR=$(mktemp -d /tmp/git.XXXXXX)
cd $DIR
git init
echo '*.pm export-subst ident' >> .gitattributes
echo '# Last Author : $Author: $Format:%an$ $' > $FILE
echo '# Last Date : $Date: $Format:%ai (%ad)% $' >> $FILE
echo '# Last Version : $Revision: $Format:%d$ $' >> $FILE
echo '# Last Version v2: $Revision: $Format:%D$ $' >> $FILE
echo '# Last Commit : $Commit: $Format:%H$ $' >> $FILE
echo '# This File : $Id$' >> $FILE
echo 'our $VERSION = (qw($Format:%N$))[1];' >> $FILE
git add .
git commit -a -m "Initial commit."
git notes add -f -m '$Release: 0048 $'
git tag -f releases/R0048
git archive HEAD $FILE | tar xf - --to-stdout
rm $FILE
git checkout -- $FILE
cat $FILE
git log --format='%N' -1


You can tell I'm attempting to recreate CVS keywords (bwahahaa!) for a
project that is insisting they need them.

I'd be happy to write a bunch of unit tests for export-subst and all
PRETTY FORMATS format:<string> options, if that would be desirable. I
see the t/ directory, and the t/test-lib.sh stuff looks simple enough
(TAP in bash, hmm).

Thanks,

Derek

[Derek Moore]

Post by Derek Moore
I first noticed this using the system git provided in Fedora 21, so I
cloned the official git repo, built from source, and it would appear
there are even more options that don't work with export-subst in the
latest code.

I'm a dumb ass. After installing my newly compiled git to /usr/local,
I neglected to spawn a new shell so the new binary could get picked up
by my $PATH. Despite /usr/local/bin preceding /usr/bin, /usr/bin/git
was cached by my shell, and 'which git' was lying to me, therefore my
testing of new format:<string> options was invalid.

I must retract the statement "and it would appear there are even more
options that don't work with export-subst in the latest code."

As far as I've tested it would seem only %N doesn't resolve inside of
$Format:$, until I maybe do unit tests for this to identify any
others.

[Jeff King]

Post by Derek Moore
As far as I've tested it would seem only %N doesn't resolve inside of
$Format:$, until I maybe do unit tests for this to identify any
others.

Yes, %N is somewhat special in that the calling code needs to initialize
the notes tree itself. We can't just do it lazily when we see the first
%N because _which_ notes we show depends on other options (e.g., for
log, if you've used --notes-ref, --show-notes=..., etc).

So in theory you need something like 5b16360 (pretty: Initialize notes
if %N is used, 2010-04-13), but adapted for git-archive. The trick,
though, is that we do not even see the format string until we are
looking at a particular file with a $Format$ marker. So you'd have to
lazily initialize notes there (and if you want to support picking
specific notes refs, you'd have to teach git-archive new options to do
so[1]).

Here's a quick-and-dirty patch that makes the snippet you posted earlier
do what I think you expected. I haven't tested it beyond that, and am
not planning to push it forward myself, but please feel free to use it
as a basis for building a solution.

---
diff --git a/archive.c b/archive.c
index 952a659..3af781e 100644
--- a/archive.c
+++ b/archive.c
@@ -5,6 +5,7 @@
#include "archive.h"
#include "parse-options.h"
#include "unpack-trees.h"
+#include "notes.h"

static char const * const archive_usage[] = {
N_("git archive [options] <tree-ish> [<path>...]"),
@@ -38,6 +39,8 @@ static void format_subst(const struct commit *commit,
if (src == buf->buf)
to_free = strbuf_detach(buf, NULL);
for (;;) {
+ struct userformat_want ufw = {0};
+ struct strbuf notes = STRBUF_INIT;
const char *b, *c;

b = memmem(src, len, "$Format:", 8);
@@ -50,10 +53,31 @@ static void format_subst(const struct commit *commit,
strbuf_reset(&fmt);
strbuf_add(&fmt, b + 8, c - b - 8);

+ userformat_find_requirements(fmt.buf, &ufw);
+ if (ufw.notes) {
+ static int initialized;
+ if (!initialized) {
+ init_display_notes(NULL);
+ initialized = 1;
+ }
+ format_display_notes(commit->object.sha1, &notes,
+ get_log_output_encoding(), 1);
+ /*
+ * trim trailing newlines from note content, which is
+ * probably more appropriate for $Format$; should
+ * this actually remove internal newlines, too?
+ */
+ strbuf_rtrim(&notes);
+ }
+ ctx.notes_message = notes.buf;
+
strbuf_add(buf, src, b - src);
format_commit_message(commit, fmt.buf, buf, &ctx);
len -= c + 1 - src;
src = c + 1;
+
+ ctx.notes_message = NULL;
+ strbuf_release(&notes);
}
strbuf_add(buf, src, len);
strbuf_release(&fmt);

-Peff

[1] I think you could get pretty far using `git -c core.notesRef=foo` to
give ad-hoc config, as the notes code should use that as the
ultimate default. But I didn't try it.

[Derek Moore]

Thanks, Jeff,

I may look into cleaning up your patch to propose a proper solution to
this list, as time allows.

Incidentally, I did stumble across:

http://git.kernel.org/cgit/git/git.git/commit/?id=5b16360330822527eac1fa84131d185ff784c9fb

which also references you, but I forgot to mention it.

Post by Jeff King

Post by Derek Moore
As far as I've tested it would seem only %N doesn't resolve inside of
$Format:$, until I maybe do unit tests for this to identify any
others.

Yes, %N is somewhat special in that the calling code needs to initialize
the notes tree itself. We can't just do it lazily when we see the first
%N because _which_ notes we show depends on other options (e.g., for
log, if you've used --notes-ref, --show-notes=..., etc).
So in theory you need something like 5b16360 (pretty: Initialize notes
if %N is used, 2010-04-13), but adapted for git-archive. The trick,
though, is that we do not even see the format string until we are
looking at a particular file with a $Format$ marker. So you'd have to
lazily initialize notes there (and if you want to support picking
specific notes refs, you'd have to teach git-archive new options to do
so[1]).
Here's a quick-and-dirty patch that makes the snippet you posted earlier
do what I think you expected. I haven't tested it beyond that, and am
not planning to push it forward myself, but please feel free to use it
as a basis for building a solution.
---
diff --git a/archive.c b/archive.c
index 952a659..3af781e 100644
--- a/archive.c
+++ b/archive.c
@@ -5,6 +5,7 @@
#include "archive.h"
#include "parse-options.h"
#include "unpack-trees.h"
+#include "notes.h"
static char const * const archive_usage[] = {
N_("git archive [options] <tree-ish> [<path>...]"),
@@ -38,6 +39,8 @@ static void format_subst(const struct commit *commit,
if (src == buf->buf)
to_free = strbuf_detach(buf, NULL);
for (;;) {
+ struct userformat_want ufw = {0};
+ struct strbuf notes = STRBUF_INIT;
const char *b, *c;
b = memmem(src, len, "$Format:", 8);
@@ -50,10 +53,31 @@ static void format_subst(const struct commit *commit,
strbuf_reset(&fmt);
strbuf_add(&fmt, b + 8, c - b - 8);
+ userformat_find_requirements(fmt.buf, &ufw);
+ if (ufw.notes) {
+ static int initialized;
+ if (!initialized) {
+ init_display_notes(NULL);
+ initialized = 1;
+ }
+ format_display_notes(commit->object.sha1, &notes,
+ get_log_output_encoding(), 1);
+ /*
+ * trim trailing newlines from note content, which is
+ * probably more appropriate for $Format$; should
+ * this actually remove internal newlines, too?
+ */
+ strbuf_rtrim(&notes);
+ }
+ ctx.notes_message = notes.buf;
+
strbuf_add(buf, src, b - src);
format_commit_message(commit, fmt.buf, buf, &ctx);
len -= c + 1 - src;
src = c + 1;
+
+ ctx.notes_message = NULL;
+ strbuf_release(&notes);
}
strbuf_add(buf, src, len);
strbuf_release(&fmt);
-Peff
[1] I think you could get pretty far using `git -c core.notesRef=foo` to
give ad-hoc config, as the notes code should use that as the
ultimate default. But I didn't try it.