Re: [PATCH] Support long format for log-based submodule diff

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2018, 21:41 UTC
Message-ID: <xmqqefkvzhqq.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180307211140.19272-1-rcdailey@gmail.com>

Robert Dailey <rcdailey.lists@gmail.com> writes:

Show 13 quoted lines

> I could have gone through the effort to make this more configurable, but
> before doing that level of work I wanted to get some discussion going to
> understand first if this is a useful change and second how it should be
> configured. For example, we could allow:
>
> $ git diff --submodule=long-log
>
> Or a supplementary option such as:
>
> $ git diff --submodule=log --submodule-log-detail=(long|short)
>
> I'm not sure what makes sense here. I welcome thoughts/discussion and
> will provide follow-up patches.

My quick looking around reveals that prepare_submodule_summary() is called only by show_submodule_summary(), which in turn is called only from builtin_diff() in a codepath like this:

	if (o->submodule_format == DIFF_SUBMODULE_LOG &&
	    (!one->mode || S_ISGITLINK(one->mode)) &&
	    (!two->mode || S_ISGITLINK(two->mode))) {
		show_submodule_summary(o, one->path ? one->path : two->path,
				&one->oid, &two->oid,
				two->dirty_submodule);
		return;
	} else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
		   (!one->mode || S_ISGITLINK(one->mode)) &&
		   (!two->mode || S_ISGITLINK(two->mode))) {
		show_submodule_inline_diff(o, one->path ? one->path : two->path,
				&one->oid, &two->oid,
				two->dirty_submodule);
		return;
	}

It looks like introducing a new value to o->submodule_format (enum diff_submodule_format defined in diff.h) would be one natural way to extend this codepath, at least to me from a quick glance.

It also looks to me that the above may become far easier to read if the common "are we dealing with a filepair <one, two> that involves submodules?" check in the above if/else if cascade is factored out, perhaps like this as a preliminary clean-up step, before adding a new value:

	if ((!one->mode || S_ISGITLINK(one->mode)) &&
	    (!two->mode || S_ISGITLINK(two->mode))) {
		switch (o->submodule_format) {
		case DIFF_SUBMODULE_LOG:
			... do the "log" thing ...
			return;
		case DIFF_SUBMODULE_INLINE_DIFF:
			... do the "inline" thing ...
			return;
		default:
			break;
		}
	}

Then the place to add a new format would be trivially obvious, i.e. just add a new case arm to call a new function to give the summary.