Page MenuHomeFreeBSD

diff3: Add support for -A and -m
AbandonedPublic

Authored by thj on Mar 3 2022, 10:30 AM.

Details

Reviewers
pstef
bapt
Group Reviewers
Klara
manpages
Summary

Add support for -A and -m, in the process tidy up the the printing of file
ranges for ed scripts.

Add tests, first using the original provided 1.txt, 2.txt, 3.txt, but also add
lao tzu tao tests from the gnu diff3 how to guide and finally some manually
written tests which excised other cases.

I think this code is quite rough, a lot of it came from brute forcing the diff3
output, which isn't really well documented or explained from what I could find
online. I am happy if this is thought of as an early review, diff3 seems to be
used in important places so I want to get this right.

The gnu diff3 guide is actually quite good, but it doesn't cover every form of
output from diff3.

https://www.gnu.org/software/diffutils/manual/diffutils.html#Comparing-Three-Files

I am not sure about the license of the lao, tzu, tao example, I feel like it is
ancient and should be in the public domain. I'll rewrite it to produce the same
test case with a public domain poem if we need to.

Test Plan

I have included tests, and will add a script here to do some randomly generated
tests of diff3. I would really appreciate more test cases, I am not sure this
exercises all the forms of output diff3 can generate, but I think it is time
for more eyes.

The random tests unearthed a missing case in the ed script generation, that fix
is included in this review.

#!/bin/sh

bsddiff3=/usr/obj/code/freebsd/worktrees/diff3/amd64.amd64/usr.bin/diff3/diff3

randomfile()
{
	dd if=/dev/random bs=100k count=1 | b64encode - > $1
}

checkdiff3()
{
	set -x
	gdiff3 $@ > gdiff3.out
	gr=$?
	$bsddiff3 $@ > bsddiff3.out
	br=$?

	if [ $gr -ne $br ]
	then
		echo test failed return codes differ: diff3 $@
		exit
	fi

	diff gdiff3.out bsddiff3.out

	if [ $? -ne 0 ] 
	then
		sdiff gdiff3.out bsddiff3.out | head
		echo test failed, output differs: diff3 $@
		exit
	fi

	echo test passed diff3 $@
}

randomfiles()
{
	randomfile o
	cp o m
	cp o y
}

checkscripts() 
{
	checkdiff3 -e m o y
	checkdiff3 -A m o y
	checkdiff3 -m m o y
}

test_all_differ()
{
	randomfile o
	randomfile m
	randomfile y
		
	checkscripts
}

test_m_additions()
{
	randomfiles

	echo "mine" >> m
	echo "mine" >> m
	echo "mine" >> m
	echo "mine" >> m

	checkscripts
}

test_y_additions()
{
	randomfiles

	echo "yours" >> m
	echo "yours" >> m
	echo "yours" >> m
	echo "yours" >> m

	checkscripts
}

test_my_same_additions()
{
	randomfiles

	echo "both" >> m
	echo "both" >> m
	echo "both" >> m
	echo "both" >> m

	echo "both" >> y
	echo "both" >> y
	echo "both" >> y
	echo "both" >> y

	checkscripts
}

test_my_conflicting_additions()
{
	randomfiles

	echo "mine" >> m
	echo "mine" >> m
	echo "mine" >> m
	echo "mine" >> m

	echo "yours" >> y
	echo "yours" >> y
	echo "yours" >> y
	echo "yours" >> y

	checkscripts
}

test_same_delete()
{
	randomfiles

	edscript=""
	edscript="$edscript\n$(ed_delete 12 24)"
	echo -e $edscript | ed m
	echo -e $edscript | ed y

	checkscripts

}

random_ed_add()
{
	printf "%da%s\n"
}

ed_move()
{
	
	printf "%d,%dm%d" $1 $2 $3
}

ed_delete()
{
	
	printf "%d,%dd" $1 $2
}

test_ed_script()
{
	randomfiles

	edscript=""
	edscript="$edscript\n$(ed_delete 12 24)"
	edscript="$edscript\n$(ed_move 45 69 23)"
	edscript="$edscript\nwq\n"

	echo -e $edscript
	echo -e $edscript | ed m
	echo -e $edscript | ed y

	checkscripts
}

make -C ..

test_m_additions
test_y_additions
test_my_same_additions
test_my_conflicting_additions

test_ed_script

echo if we got here all the tests passed

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Errors
SeverityLocationCodeMessage
Errorusr.bin/diff3/diff3.1:169MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/long-A.out:5MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/long-A.out:18MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/long-merge.out:13MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/long-merge.out:24MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/tao-A.out:6MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/tao-A.out:22MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/tao-merge.out:2MERGECONFLICT1Unresolved merge conflict
Errorusr.bin/diff3/tests/tao-merge.out:20MERGECONFLICT1Unresolved merge conflict
Unit
No Unit Test Coverage
Build Status
Buildable 44625
Build 41513: arc lint + arc unit

Event Timeline

thj requested review of this revision.Mar 3 2022, 10:30 AM

I spotted one thing in the manual page, otherwise it looks good to me.

usr.bin/diff3/diff3.1
121

Macros need to go on their own line.

I'll take a look in a bit more detail later... I think for this one, I'm going to apply it locally and just review the whole -A/-m paths as a whole rather than trying to review what's been added/removed here...

usr.bin/diff3/diff3.1
76

A should sort before a

From the summary: "Add tests, first using the original provided 1.txt, 2.txt, 3.txt, but also add
lao tzu tao tests from the gnu diff3 how to guide and finally some manually
written tests which excised other cases."

Unless you really mean "precisely or surgically cut off with a sharp knife" (literally or figuratively), that should be "exercised".

Since (per https://reviews.freebsd.org/D34411#change-nX1rdgK113Nt) mg was explicitely put in the public domain, you could use it instead of lao tzu and tao.

usr.bin/diff3/diff3.1
41

If you swap a and A per https://reviews.freebsd.org/D34421#inline-212610, do it here as well.

From the summary: "Add tests, first using the original provided 1.txt, 2.txt, 3.txt, but also add
lao tzu tao tests from the gnu diff3 how to guide and finally some manually
written tests which excised other cases."

Unless you really mean "precisely or surgically cut off with a sharp knife" (literally or figuratively), that should be "exercised".

Since (per https://reviews.freebsd.org/D34411#change-nX1rdgK113Nt) mg was explicitely put in the public domain, you could use it instead of lao tzu and tao.

I don't understand how the linked file (I think the readme) helps as an example of a 3 way merge.

In D34421#780272, @thj wrote:

I don't understand how the linked file (I think the readme) helps as an example of a 3 way merge.

Giving you enough text for making 3 related-but still different files.

I am not sure about the license of the lao, tzu, tao example, I feel like it is
ancient and should be in the public domain. I'll rewrite it to produce the same
test case with a public domain poem if we need to.

The original is ancient, but the copy we're referring to is an English translation and as such it's possibly subject to copyright. From what I understand, the translation was published in 1963 and has entered public domain unless the copyrights were renewed (and I don't know if they were).
This is still a concern for any other translation, so I suggest using something originally written in English and under public domain, of course.

There doesn't seem to be any specification of the diff3 behavior, so I spent some time looking for some test cases. GNU doesn't provide much directly in the source (https://git.savannah.gnu.org/cgit/diffutils.git/plain/tests/diff3) and also the mailing list isn't rich in corner cases relating to diff3. I've found some test cases for git's implementation (https://github.com/git/git/search?l=Shell&q=diff3) but I only skimmed that so I don't know how useful they are.

There doesn't seem to be any specification of the diff3 behavior, so I spent some time looking for some test cases. GNU doesn't provide much directly in the source (https://git.savannah.gnu.org/cgit/diffutils.git/plain/tests/diff3) and also the mailing list isn't rich in corner cases relating to diff3. I've found some test cases for git's implementation (https://github.com/git/git/search?l=Shell&q=diff3) but I only skimmed that so I don't know how useful they are.

The git tests looks to only use diff3 as a name or as as the style of comment. I am not sure if they can be used to test our diff3