Re: [PATCH v4 01/10] show, log: provide a --remerge-diff capability

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 1, 2022, 09:09 UTC
Message-ID: <220201.86zgnb9bf9.gmgdl@evledraar.gmail.com>
In-Reply-To: <0b94724311df34dd10debd43c466695ed406d790.1642792341.git.gitgitgadget@gmail.com>

On Fri, Jan 21 2022, Elijah Newren via GitGitGadget wrote:

Show 20 quoted lines

> From: Elijah Newren <newren@gmail.com>
> [...]
>  ifdef::git-log[]
> ---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc)::
> +--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
>  --no-diff-merges::
>  	Specify diff format to be used for merge commits. Default is
>  	{diff-merges-default} unless `--first-parent` is in use, in which case
> @@ -64,6 +64,14 @@ ifdef::git-log[]
>  	each of the parents. Separate log entry and diff is generated
>  	for each parent.
>  +
> +--diff-merges=remerge:::
> +--diff-merges=r:::
> +--remerge-diff:::
> +	With this option, two-parent merge commits are remerged to
> +	create a temporary tree object -- potentially containing files
> +	with conflict markers and such.  A diff is then shown between
> +	that temporary tree and the actual merge commit.
> ++

Re some previous discussion. I really think we should add something like
this paragraph to this:
    
    The output emitted when this option is used is subject to change, and so
    is its interaction with other options (unless explicitly
    documented). I.e. many of the same caveats as the "OUTPUT STABILITY" in
    the linkgit:git-range-diff[1] documentation describes apply here. In
    particular other diff filtering options, pathspec limitations etc. may
    not produce the expected results, as some of those may apply to the
    "real" diff of the merge, and not on the generated "remerge-diff".

I think that would nicely give us permission to develop this further without having to think about all the option interaction etc.

This is really useful right now, but I'd hate for it to get merged with some bug/behavior that's not obvious to us now, and it being hard to fix that because we'd have to consider the implicitly promised backwards compatibility.

Show 7 quoted lines

>  	int saved_dcctc = 0;
> +	struct tmp_objdir *remerge_objdir = NULL;
> +
> +	if (rev->remerge_diff) {
> +		remerge_objdir = tmp_objdir_create("remerge-diff");
> +		if (!remerge_objdir)
> +			die(_("unable to create temporary object directory"));

I guess the s/die_errno/die/ here is better for now as we won't report the wrong errno, but also lose the common case of errno being right. But that can be fixed up with some other series to the tmp-objdir API.

Show 6 quoted lines

> [...]
> +# This test is ort-specific
> +test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort || {
> +	skip_all="GIT_TEST_MERGE_ALGORITHM != ort"
> +	test_done
> +}

FWIW this is still on a more complex pattern that it needs to be, see this v1 discussion (which you seemed to ack):

https://lore.kernel.org/git/CABPp-BE+4rZNP-5mT2MNOWR6y6BgEG6mt1r_qcrZtarom6aGsw@mail.gmail.com/