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

[Toon Claes <toon@iotcl.com>]

Junio C Hamano <gitster@pobox.com> writes:

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

I know it's silly, but I wanted to feed these options to
parse_options(). Doing this would make them show up in `git
last-modified -h`.

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

That too, at least for this option. Not the other patch.

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

I actually agree this is better. But I was following the pattern of
`line_termination` other in various other places. I'll adapt it to use a
bool instead.

-- 
Cheers,
Toon