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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 26, 2025, 16:57 UTC
Message-ID: <xmqq3460pw8y.fsf@gitster.g>
In-Reply-To: <20251126-toon-last-modified-zzzz-v1-1-608350df0caa@iotcl.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 ...

Show 20 quoted lines

>  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

Show 6 quoted lines

> @@ -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

Show 12 quoted lines

>  	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()
>  	};