Re: [PATCH 1/3] last-modified: handle and document NUL termination

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

Toon Claes <toon@iotcl.com> writes:

> When option `-z` is provided to git-last-modified(1), each line is
> separated with a NUL instead of a newline. Document this properly and
> handle parsing of the option in the builtin itself.

I think documenting does make sense, but it is not clear from the
description why it is better to handle the option in the builtin
itself, instead of letting the setup_revisions() take care of it.

Is it because after the command lets setup_revisions() to parse out
the revision range, the command does not really let the revision
machinery drive diffs and let it output anything (hence, even though
rev.diffopt.line_termination is set from the command line, the calling
builtin is the only one that pays attention, and the revision machinery
and the diff machinery called from there does not pay attention to it?

And assuming that this new division of labor between the revision
machinery and the subcommand makes sense (it needs to be explained
better), the updated code does make sense to me.

But it looks suboptimal.  See below.

> +#define LAST_MODIFIED_INIT { \
> +	.line_termination = '\n', \
> +}

You have to introduce such a non-zero initialization, only because
you pretend to accept _any_ byte here, and use it as the line
termination character.  If you were porting Git to ancient Macintosh,
you could set this to '\r' and it would follow their text file
convention there ;-)

But ...

>  struct last_modified_entry {
>  	struct hashmap_entry hashent;
>  	struct object_id oid;
> @@ -55,6 +59,7 @@ struct last_modified {
>  	struct rev_info rev;
>  	bool recursive;
>  	bool show_trees;
> +	int line_termination;
>  
>  	const char **all_paths;
>  	size_t all_paths_nr;
> @@ -165,7 +170,7 @@ static void last_modified_emit(struct last_modified *lm,
>  		putchar('^');
>  	printf("%s\t", oid_to_hex(&commit->object.oid));
>  
> -	if (lm->rev.diffopt.line_termination)
> +	if (lm->line_termination)
>  		write_name_quoted(path, stdout, '\n');
>  	else
>  		printf("%s%c", path, '\0');

... you use hardcoded '\n' here, without allowing the value of
line_termination to affect the termination character.

This is way suboptimal.  Instead, would it work if you add

	bool null_termination;

to the last_modified structure, and do

	if (!lm->null_termination)
		write_name_quoted(path, stdout, '\n');
	else
		printf("%s%c", path, '\0');

here?   Then

> @@ -507,10 +512,10 @@ int cmd_last_modified(int argc, const char **argv, const char *prefix,
>  		      struct repository *repo)
>  {
>  	int ret;
> -	struct last_modified lm = { 0 };
> +	struct last_modified lm = LAST_MODIFIED_INIT;

You do not need this change, and

>  	const char * const last_modified_usage[] = {
> -		N_("git last-modified [--recursive] [--show-trees] "
> +		N_("git last-modified [--recursive] [--show-trees] [-z] "
>  		   "[<revision-range>] [[--] <path>...]"),
>  		NULL
>  	};
> @@ -520,6 +525,8 @@ int cmd_last_modified(int argc, const char **argv, const char *prefix,
>  			 N_("recurse into subtrees")),
>  		OPT_BOOL('t', "show-trees", &lm.show_trees,
>  			 N_("show tree entries when recursing into subtrees")),
> +		OPT_SET_INT('z', NULL, &lm.line_termination,
> +			N_("lines are separated with NUL character"), '\0'),

This will become OPT_BOOL() to set the &lm.null_termination.

>  		OPT_END()
>  	};