Re: [PATCH] Add the --submodule option to the diff option family

[Jens Lehmann <Jens.Lehmann@web.de>]

Junio C Hamano schrieb:
> I have four patches queued on js/diff-verbose-submodule topic and I think
> this corresponds to the first three, correct?

Yes.


>> +--submodule[=<format>]::
>> +	Chose the output format for submodule differences. <format> can be one of
>> +	'short' and 'left-right-log'. 'short' is the default value for this
>> +	option and and shows pairs of commit names. 'left-right-log' lists the
>> +	commits in that commit range like the 'summary' option of
>> +	linkgit:git-submodule[1] does.
>> +
> 
> Well, while left-right-log may be logically the most correct name for this
> option, I think it is too long to be practical.  Because it is not like we
> would want to have an option to have full log there when we are showing
> "diff", I think it would make sense to making left-right-log the default
> when "--submodule" (without format specification) is given, and possibly
> give "--submodule=log" as the synonym for this format.

O.k., "--submodule=log" it is.


> After all, if you do not give --submodule, we will give the traditional
> short format, no?

Yes. Will make the documentation clearer on this.


>> diff --git a/diff.c b/diff.c
>> index c719ce2..8af1ae2 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2771,6 +2783,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>>  		DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
>>  	else if (!strcmp(arg, "--ignore-submodules"))
>>  		DIFF_OPT_SET(options, IGNORE_SUBMODULES);
>> +	else if (!prefixcmp(arg, "--submodule=")) {
>> +		if (!strcmp(arg + 12, "left-right-log"))
>> +			DIFF_OPT_SET(options, SUBMODULE_LEFT_RIGHT_LOG);
>> +	}
> 
> I do not see --submodule (without "=<format>") supported here as the
> documentation claims, but this is the logical place to do so.

Will change that.


>> diff --git a/submodule.c b/submodule.c
>> new file mode 100644
>> index 0000000..206386f
>> --- /dev/null
>> +++ b/submodule.c
>> @@ -0,0 +1,112 @@
>> +...
>> +void show_submodule_summary(FILE *f, const char *path,
>> +		unsigned char one[20], unsigned char two[20],
>> +		const char *del, const char *add, const char *reset)
>> +{
>> ...
>> +	fwrite(sb.buf, sb.len, 1, f);
>> +
>> +	if (!message) {
>> +		while ((commit = get_revision(&rev))) {
>> + ...
>> +		}
>> +		clear_commit_marks(left, ~0);
>> +		clear_commit_marks(right, ~0);
>> +	}
>> +}
> 
> I thought we had strbuf_release(&sb) here...  Where did it go?

*blush* ... thanks for catching this, will put it back where it belongs.