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

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

On Thu, Oct 9, 2014 at 10:56 AM, Derek Moore [off-list ref] wrote:

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.

On Thu, Oct 09, 2014 at 12:42:39PM -0500, Derek Moore wrote:

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.

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.


On Thu, Oct 9, 2014 at 2:07 PM, Jeff King [off-list ref] wrote: