feat: (partially) revamp todo

[mitchellwrosen]

Overview

Related: #5088

This PR deletes most of the old todo's functionality (related to patches) and adds back two features:

• todo tells you about conflicted names
• todo tells you about direct dependencies that don't have names

Left for future work are the other things specified in #5088: merge precondition violations and dependents of the todo builtin term

Test coverage

I unfortunately did not see a slam-dunk way to make a transcript for this.

• I think the right way to induce conflicted names would be some sort of alias.term.force command, as alias.term complains "a term by that name already exists".
• Similarly, I think the right way to induce a nameless direct dependency would be some sort of delete.term.force command, as delete.term currently does allow you to delete the last name for a direct dependency, but really shouldn't (right?!)

So, as of this moment, I've only manually tested todo, but it does appear to work.

[pchiusano]

@mitchellwrosen you can move a term to a subnamespace, then delete.namespace.force that subnamespace to get unnamed dependencies.

You could use merge.old to get a name conflict :)

[aryairani]

Can also use delete.term and change the transcript later if we change the behavior of that command.

[mitchellwrosen]

I've added a transcript that uses delete.namespace.force to show nameless direct deps, but not merge.old to show conflicted names. Rationale: don't want to make merge.old harder to rip out

[aryairani]

don't want to make merge.old harder to rip out

Agreed.

I really want to have good transcript coverage of our user-facing messages though; for example right now we can't easily review what the output is for that feature. How can we get this? A screenshot is a baseline starting point, but they can go out of date easily, thus transcripts.

What about a small test branch on share that the transcript pulls. I did this in unison-src/transcripts/pull-errors.md but probably we should consolidate them in the @unison org somewhere.

[mitchellwrosen]

How can we get this?

I still think this:

I think the right way to induce conflicted names would be some sort of alias.term.force command, as alias.term complains "a term by that name already exists".

Unless you mean how can we get this before merging?

[mitchellwrosen]

(btw, the output for that hasn't changed since the original todo; it also reported conflicted names)

[aryairani]

Unless you mean how can we get this before merging?

Yeah, whenever possible, I really want to have good transcript coverage of our user-facing messages when we merge them.

I understand that for this PR, that portion of the output should be the same as whatever it was before, but as far as I know we don't have any transcript coverage of that either. (Do we?)

How can we get this?

I still think this:

I think the right way to induce conflicted names would be some sort of alias.term.force command, as alias.term complains "a term by that name already exists".

Scheduling a separate task to create a new command just to produce the test case that we don't even want to be possible sounds like something we'll never do. 😅 (And arguably shouldn't do.)

Since it is possible to create this namespace today with merge.old, and that's the case we're trying to support, for legacy branches even when merge.old is deleted, could you use the clone technique I referenced in the last message to set up a transcript test in this PR — or any better method you prefer is also fine.

[mitchellwrosen]

Hrm, I don't like the clone technique - that seems like the wrong solution here.

I'd vote using merge.old over that, and perhaps implementing alias.term.force over using merge.old. I don't think that command would be terrible to have; it'd certainly be useful for this situation and capturing any other behavior that involves conflicted names.

[mitchellwrosen]

Ok, I added alias.term.force here: #5118

[aryairani]

Hrm, I don't like the clone technique - that seems like the wrong solution here.

^ Could you say more?

[mitchellwrosen]

Sure, I think minimizing the number of tests that hit the network is a good thing. In this case, there doesn't seem to be a need to hit the network. I think that perhaps your thinking is we already have some namespace that has conflicted names somewhere on Share, and we can just use that. Did I misunderstand all along what the proposal was?

A test seems a lot easier to understand and modify if it's all "local" and in the test file itself. If the test has to download some opaque thing and then assert properties about it, that's more indirect and confusing, and getting in there to adjust the thing that it downloads would be a chore. So, again, if I've misunderstood the proposal, let me know. Otherwise, I think all that stands.

[aryairani]

@mitchellwrosen wrote:
Sure, I think minimizing the number of tests that hit the network is a good thing. In this case, there doesn't seem to be a need to hit the network. I think that perhaps your thinking is we already have some namespace that has conflicted names somewhere on Share, and we can just use that. Did I misunderstand all along what the proposal was?

Maybe :) My proposal was to use legacy ucm to construct and post a minimal legacy namespace on Share for the test, enabling us to delete the undesired functionality from ucm, while still being able to demonstrate ucm output on conflicted namespaces.

@aryairani wrote:
What about a small test branch on share that the transcript pulls. I did this in unison-src/transcripts/pull-errors.md though probably we should consolidate them in the @unison org somewhere.

@aryairani wrote:
to create a new command just to produce the test case that we don't even want to be possible sounds like something we'll never do. 😅 (And arguably shouldn't do.)

Since it is possible to create this namespace today with merge.old, and that's the case we're trying to support, for legacy branches even when merge.old is deleted, could you use the clone technique I referenced in the last message to set up a transcript test in this PR

The issue you raised (don't want to make it harder to rip out merge.old) makes sense, but you're proposing making it harder to rip out alias.term.force, and also creating it special just for this purpose. We shouldn't keep either of them, but that doesn't have to stop us from demonstrating the todo output in a transcript. Also we would need an alias.type.force to fully demonstrate the output.

@mitchellwrosen wrote:
A test seems a lot easier to understand and modify if it's all "local" and in the test file itself. If the test has to download some opaque thing and then assert properties about it, that's more indirect and confusing, and getting in there to adjust the thing that it downloads would be a chore. So, again, if I've misunderstood the proposal, let me know. Otherwise, I think all that stands.

I agree, though it wouldn't be an opaque thing (it would be browsable on Share), and it shouldn't be adjusted either.

So my thinking is:

• yes to testing + covering user-facing messages with transcripts,
• yes to "minimize the number of tests that hit the network"
• no to "create commands that only exist to construct failing cases."

To me, "minimize the number of tests that hit the network" is the lowest priority of the three, so that's where the proposal came from.

[aryairani]

merging to get this in while we discuss next steps