support WORKING and STAGED in three-dot revisions

[elianddb]

Three-dot revisions such as DOLT_DIFF('WORKING...bar') rejected the WORKING and STAGED pseudo-refs with "branch not found". They now resolve to the branch HEAD commit when computing the merge base.
Fix #11204

[coffeegoddd]

@elianddb DOLT

read_tests	from_latency	to_latency	percent_change
covering_index_scan	2.3	2.3	0.0
groupby_scan	142.39	144.97	1.81
index_join	2.0	2.0	0.0
index_join_scan	1.34	1.34	0.0
index_scan	219.36	219.36	0.0
oltp_point_select	0.24	0.24	0.0
oltp_read_only	4.91	4.91	0.0
select_random_points	0.55	0.55	0.0
select_random_ranges	0.63	0.62	-1.59
table_scan	196.89	193.38	-1.78
types_table_scan	458.96	450.77	-1.78

write_tests	from_latency	to_latency	percent_change
oltp_delete_insert	6.09	6.09	0.0
oltp_insert	3.13	3.13	0.0
oltp_read_write	11.24	11.04	-1.78
oltp_update_index	3.3	3.3	0.0
oltp_update_non_index	2.97	2.97	0.0
oltp_write_only	6.21	6.21	0.0
types_delete_insert	6.79	6.79	0.0

[coffeegoddd]

@elianddb DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
9e54544	ok	5937471

version	total_tests
9e54544	5937471

correctness_percentage
100.0

[coffeegoddd]

@elianddb DOLT

test_name	from_latency_p95	to_latency_p95	percent_change
tpcc-scale-factor-1	45.79	44.98	-1.77

test_name	from_server_name	from_server_version	from_tps	to_server_name	to_server_version	to_tps	percent_change
tpcc-scale-factor-1	dolt	a16fe3b	53.0	dolt	5361c1f	53.26	0.49

[coffeegoddd]

@elianddb DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
ec4376d	ok	5937471

version	total_tests
ec4376d	5937471

correctness_percentage
100.0

[coffeegoddd]

@elianddb DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
bae8664	ok	5937471

version	total_tests
bae8664	5937471

correctness_percentage
100.0

[nicktobey]

I'm worried this change is overly broad and might result in us treating WORKING or STAGED as HEAD in places we probably shouldn't.

For instance, with this change, HAS_ANCESTOR('HEAD','WORKING') returns true, even though it shouldn't. I think it's debatable whether or not "STAGED" and "WORKING" should be valid values for the ancestor field here, but they definitely shouldn't be treated as "HEAD"

Similarly, DOLT_DIFF('STAGED...WORKING', ...) now uses HEAD as the merge base and will display already-staged changes in the diff. This seems wrong.

I think if the purpose of this function is to generate commits, then it shouldn't accept things that aren't refs to commits. WORKING and STAGED should be checked for at a level above this. Perhaps we need a wrapper around the MergeBase logic that accepts these strings.

[NoTuxNoBux]

They now resolve to the branch HEAD commit

I have a question: does this mean I cannot get the resulting diff to include the WORKING changes on a branch and instead this will become identical to using HEAD instead? This was mostly my use case in #11204 😄 .

Or perhaps this was just when computing the base and I misunderstood?

[elianddb]

They now resolve to the branch HEAD commit

I have a question: does this mean I cannot get the resulting diff to include the WORKING changes on a branch and instead this will become identical to using HEAD instead? This was mostly my use case in #11204 😄 .

Or perhaps this was just when computing the base and I misunderstood?
You'd have to put WORKING on the right hand side, since when on the left it's used as the fork point.

[coffeegoddd]

@elianddb DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
b461da5	ok	5937471

version	total_tests
b461da5	5937471

correctness_percentage
100.0

[coffeegoddd]

@elianddb DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
5361c1f	ok	5937471

version	total_tests
5361c1f	5937471

correctness_percentage
100.0

[nicktobey]

While this fixes the examples from my previous comment, I'm still a bit skeptical of the implementation. The additional checks make the logic more complicated and the comments are misleading. For instance, calling STAGED...WORKING "ambiguous" is misleading: there's nothing ambiguous about what it means, we just don't currently have the logic to display that diff correctly because we require the merge base to be a commit.

I think there's some things we can do to clean it up. ResolveCommitSpecStrForMergeBase probably shouldn't be a public function: it should be an implementation detail in how we compute merge bases. We can deduplicate the common logic in MERGE_BASE and DOLT_DIFF into a single function, where no references to the Commit values escape this new function. The use in DOLT_LOG might be a bit trickier to remove, but is worth attempting.