Okay. So, as I’ve written last time, I sent the patches to the mailing list, which I thought were perfect. Turns out they weren’t. There are many things wrong with how we implement the describe atom which Junio pointed out in his reply.

First of which is, when doing something like

git for-each-ref --format="%(describe:tags=yes)" refs/tags/

gives an error because according to the code, we just parse for integer values. config.h has a function git_parse_maybe_bool() which can be used to parse values such as “yes, no, true, false”, which are perfectly valid but are not handled by the current code.

Second, we can’t parse multiple options

git for-each-ref --format="%(describe:tags=yes,abbrev=14)" refs/tags/

will give an error saying that the boolean arg for tags is not proper or something similar for anything which is used in place of tags. We can overcome this by properly parsing the options, probably something similar to pretty.

Third is the clarity of the data structure we use as the basis of the describe atom

struct {
	enum { D_BARE, D_TAGS, D_ABBREV, D_MATCH, D_EXCLUDE } option;
	unsigned int tagbool;
	unsigned int length;
	char *pattern;
} describe;

One can’t really tell where we are using tagbool (a very bad variable name), length and pattern exactly w.r.t. its corresponding option. Also, for obvious reasons, this struct doesn’t really work for multiple options we are discussed above.

Apart from these, Junio also suggested that we document this atom in a different way which would be more clear and I agree.

A possible solution

So, coming to fixing this mess, I have some work locally which is similar to the approach taken in pretty (which I haven’t pushed to my fork yet since for some reason it’s sending SIGSEGV, gdb here I come). We introduce two new functions match_atom_arg_value() and match_atom_bool_arg(), which are similar to the functions match_placeholder_arg_value() and match_placeholder_bool_arg() in pretty.

These functions are used for parsing multiple option arguments given to --pretty=, for example in the case of trailers, where trailers can take multiple sub-options like key and seperator at a time. That is

git log --pretty="%(trailers:key=Helped-By,separator=%x2C )"

where we get all the trailers with “Helped-By”, separated by a comma.

The implementation of describe in pretty also makes use of these functions to parse multiple options so I think making use of them ref-filter would be great in the sense that future atoms may also of make use of these.

I haven’t really discussed this Christian and Hariom yet, regarding the apporach. I’ll do it once I have pushed things.

What’s happening with the other stuff?

I have started working on Hariom’s idea again while waiting for the review on the describe atom and I am mainly focusing on the convert_format() function which plays the central role in the whole thing.

The function basically converts any pretty format into ref-filter format, if there is a map. We can use this directly in pretty and tweak the other functions in pretty to do things accordingly.

I haven’t pushed this to my fork yet as the code doesn’t really work as I intended it to, so I need to make some changes.

Coming to .mailmap options, I haven’t worked on them this week but I should be able to it once v2 of describe atom goes out.

Apart from these, according to the latest “What’s Cooking” email (Jul 10), the patch regarding the describe:abbrev=<number> test has been merged to master and the patches regarding signature atom made it into next and are now on their way to master which is cool.

Hoping that v2 of describe atom is much better.

‘til next time,
Kousik