Re: [RFC/PATCH] mergetool: use resolved conflicts in all the views
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 16, 2020, 22:24 UTC
- Message-ID
- <xmqqa6ud2xuw.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20201216174345.28146-1-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 19 quoted lines
> It doesn't make sense to display already-resolved conflicts in the > different views of all mergetools. > > We already have the best version in MERGED, with annotations that can > be used to extract a pruned version of LOCAL and REMOTE. If we are using > the diff3 conflict-style, we can even extract BASE. > > Let's use these annotations instead of using the original files before > the conflict resolution. > > TODO: There may be a better way to extract these files that doesn't rely > on the user's conflict-style configuration. > > See Seth House's blog post [1] for the idea and the rationale. > > [1] https://www.eseth.org/2020/mergetools.html > > Cc: Seth House <seth@eseth.com> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Hmph, I got what Seth showed, but I do not quite see how the ideas in the post relate to what this patch does. The patch just avoids grabbing the contents of each stage out to a file for three stages using "git checkout-index" and instead does the same by munging the diff3 output, which ought to have the same information at least for text files, using "sed", or is there something I am not seeing?
If there is none, then what is the benefit of doing the same thing without running 3 checkout-index? Performance?
Also using checkout-index to grab the raw contents without relying on the textual context marker would mean that a custom mergetool backend could be used to merge non-text files. So if this optimization (I am still assuming this is "doing the same without running three checkout-index to gain performance" I read out of the patch text) is worth doing, there needs a knob to allow users to opt out of it somehow to avoid regressing on the feature front.
If the mergetool were in control to (re)start the merge process that would result in the conflicted state, we could override the end-user preference on the conflict style (either 'GNU merge'-style or 'diff3'-style) and conflict-marker-length to be used in showing textual conflicts, but as I understand "mergetool" is handed an already conflicted state and asked to resolve it, it would not be possible without at least looking at the stage #1 to recover the base for folks who do not use diff3 style.
Show 39 quoted lines
> ---
> git-mergetool.sh | 19 ++++---------------
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index e3f6d543fb..4759433d46 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -227,18 +227,6 @@ stage_submodule () {
> git update-index --add --replace --cacheinfo 160000 "$submodule_sha1" "${work_rel_path%/}" || die
> }
>
> -checkout_staged_file () {
> - tmpfile="$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" &&
> - tmpfile=${tmpfile%%' '*}
> -
> - if test $? -eq 0 && test -n "$tmpfile"
> - then
> - mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
> - else
> - >"$3"
> - fi
> -}
> -
> merge_file () {
> MERGED="$1"
>
> @@ -318,9 +306,10 @@ merge_file () {
> # where the base's directory no longer exists.
> mkdir -p "$(dirname "$MERGED")"
>
> - checkout_staged_file 1 "$MERGED" "$BASE"
> - checkout_staged_file 2 "$MERGED" "$LOCAL"
> - checkout_staged_file 3 "$MERGED" "$REMOTE"
> + # TODO: How do we get $MERGED always with diff3?
> + sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======$/,/^>>>>>>> /d' "$MERGED" > "$BASE"
> + sed -e '/^<<<<<<< /,/^=======$/d' -e '/^>>>>>>> /d' "$MERGED" > "$LOCAL"
> + sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$MERGED" > "$REMOTE"
> Style (lose SP after '>' redirection; I looked at the existing code and spotted one existing violation but that is not an excuse to make things even less consistent).
Also, conflict-marker-size=<N> attributes can change the length, so hardcoding these patterns would not work for everybody.
> if test -z "$local_mode" || test -z "$remote_mode" > then
Thanks.