Fix TODO about making enforce_view_invariants() return an error

[kevincliao]

Repo: Fix TODO about making enforce_view_invariants() return an error instead of panicking

At Google, we run jj lib on our servers. If jj lib panics, it causes the servers to crash, so we are trying to fix those. In this case, a network issue causes self.index.heads() to return an error, and unwrapping it crashed our servers. As part of the fix I also had to change several functions like view() to also return a Result<>, so this PR modifies many files.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added/updated tests to cover my changes
• I fully understand the code that I am submitting (what it does,
  how it works, how it's organized), including any code drafted by an LLM.
• For any prose generated by an LLM, I have proof-read and copy-edited with
  an eye towards deleting anything that is irrelevant, clarifying anything
  that is confusing, and adding details that are relevant. This includes,
  for example, commit descriptions, PR descriptions, and code comments.

[github-actions]

The following commits do not follow our format for subject lines:

• 71b819e: Fix TODO about making enforce_view_invariants() return an error instead of panicking

Commits should have a subject line following the format <topic>: <description>. Please review the commit guidelines for more information.

All commits are now correctly formatted. Thank you for your contribution!

[PhilipMetzger]

imo, this is a lib: or repo: level change, since fix: is quasi reserved for the aforementioned command.

[kevincliao]

imo, this is a lib: or repo: level change, since fix: is quasi reserved for aforementioned command.

Ah I assumed fix was referring to fixing a bug rather than the fix command. I changed the description to 'repo:' since this also changes some functions in cli crate to also return an error.

[martinvonz]

For reference, here's @yuja's opinion from when the TODO was introduced: #5360 (comment)

[kevincliao]

For reference, here's @yuja's opinion from when the TODO was introduced: #5360 (comment)

@yuja Can you please take a look at this PR?

[martinvonz]

Btw, note that GitHub's UI seems to open the PR itself if you click the link, but if you choose "open in a new tab", it will take you to the comment I linked to. But let me just copy that comment to here:

I don't think it would make sense to propagate error from this function, at leas with the current design. enforce_view_invariants() exists for lazy computation, so it can be called from arbitrary places where the method call is supposed to be immutable & infallible.

I'm not sure what alternative design Yuya had in mind.

[kevincliao]

Btw, note that GitHub's UI seems to open the PR itself if you click the link, but if you choose "open in a new tab", it will take you to the comment I linked to. But let me just copy that comment to here:

I don't think it would make sense to propagate error from this function, at leas with the current design. enforce_view_invariants() exists for lazy computation, so it can be called from arbitrary places where the method call is supposed to be immutable & infallible.

I'm not sure what alternative design Yuya had in mind.

Thanks. I have the habit of always opening in a new tab, so I didn't even realize of the bizarre github behavior.

Just to clarify where I'm standing, I consider myself a Rust rookie, and I'm happy to change the code to whatever the best pattern is for Rust. But first I'd need someone (@yuja) to share which direction I should move forwards to.

[yuja]

I don't think it would make sense to propagate error from this function, at leas with the current design. enforce_view_invariants() exists for lazy computation, so it can be called from arbitrary places where the method call is supposed to be immutable & infallible.

Perhaps we can allow duplicates in mut_repo.view().heads() and add a function like mut_repo.resolve_heads() -> Result to handle deduplication. Alternatively, we can add forwarding functions like repo.bookmarks() { self.view.bookmarks() } to bypass enforce_view_invariants().

[martinvonz]

I suppose another option is to have resolve_heads() method Yuya suggested and also have a dirty flag and make view() panic if the dirty bit is set. Then we avoid the Result on view() (and on bookmarks() etc) while still not making the API silently return redundant heads or non-heads.

[yuja]

I suppose another option is to have resolve_heads() method Yuya suggested and also have a dirty flag and make view() panic if the dirty bit is set.

That makes sense if redundant heads cause problems. Since we often calculate ::<heads>, I think the lack of enforce_view_invariants() is usually fine.

[martinvonz]

I don't like the idea of writing duplicate heads or non-heads to the operation at least. If it's just incorrect in memory, then we can at least fix it after we get a bug report. If it's incorrect on disk, then it becomes much more annoying.

[yuja]

I don't like the idea of writing duplicate heads or non-heads to the operation at least.

Agreed. The immutable view heads should be canonical at least. If we add something like repo.resolve_heads(), its immutable implementation would just return view.heads().

[kevincliao]

If we add something like repo.resolve_heads(), its immutable implementation would just return view.heads().

Sorry for the delay, was out yesterday. I think I'm not following how this works. Is this saying that we should add a resolve_heads() function and update existing callers of MutableRepo to call it whenever view can have duplicates?

Also, enforce_view_invariants is currently called in 4 places in MutableRepo. Should some of these functions like merge or consume also be calling resolve_heads()?

• view()
• has_changes()
• merge()
• consume()

I suppose another option is to have resolve_heads() method Yuya suggested and also have a dirty flag and make view() panic if the dirty bit is set. Then we avoid the Result on view() (and on bookmarks() etc) while still not making the API silently return redundant heads or non-heads.

Sidetracking a bit - Do you think it's a good idea to make jj lib not panic at all, even for human program errors? The crash Google had this time was caused by a neighboring team's servers (Piper) overloaded and rejecting requests. The painful thing about this is, the unwrap() apparently existed for over a year, but only triggered recently when it met the circumstances. And when this happened, our servers repeatedly crashes, and there's no way for us to mitigate the issue immediately since creating a fix can take hours or days. If we can make jj lib not panic at all, it would be much more friendly to use on the server.

[martinvonz]

Is this saying that we should add a resolve_heads() function and update existing callers of MutableRepo to call it whenever view can have duplicates?

Exactly (well, I didn't write it, but I assume that's what Yuya meant).

Should some of these functions like merge or consume also be calling resolve_heads()?

If we make it the updater's responsibility to call it, then I think we should just panic if the dirty bit is set in those places.

Do you think it's a good idea to make jj lib not panic at all, even for human program errors?

Strictly speaking no, but I think this particular example is a good one. The reason I think we shouldn't strictly disallow panics even when there are programming errors is that programming errors can be nonobvious but they can also be obvious. Things like let x = Some(1); let y = x.unwrap() are obviously safe, for example. The panic you ran into was far from as obvious, but it was clear enough that we had a TODO about it. If we make the change I suggested above, it would be much less obvious if we could still panic. So perhaps we should do what Yuya suggested and allow these functions to return incorrect results, and then just make sure that we call resolve_heads() before we create the operation object, so it's at least correct in storage.

Btw, perhaps resolve_heads() is better called normalize_heads()?

[yuja]

Is this saying that we should add a resolve_heads() function and update existing callers of MutableRepo to call it whenever view can have duplicates?

My idea is that, if the caller expects resolved/normalized heads, it should call resolve_heads(). I'm not sure whether resolve_heads() is a function that returns a set of resolved heads (and cache the result).

repo.resolve_heads()?;
let heads = repo.view().heads();
# or
let heads = repo.resolve_heads()?;

The latter doesn't have to mutate view.data.head_ids because the normalized heads can be cached locally in MutableRepo, which might make the implementation simpler.

If we make dirty view() access panic, resolve_heads() will have to be executed by mutation functions.

Also, enforce_view_invariants is currently called in 4 places in MutableRepo. Should some of these functions like merge or consume also be calling resolve_heads()?

* view()
* has_changes()
* merge()
* consume()

consume() should return a view with resolved/normalized heads, so yes, it should call resolve_heads(). While merge() works without normalization, it would also be good to normalize the heads eagerly there. It's probably okay for has_changes() to report changes without performing normalization.

Btw, perhaps resolve_heads() is better called normalize_heads()?

+1

[kevincliao]

Thanks for the explanations, I will give this a shot.

[kevincliao]

I was able to update has_changes, merge and consume successfully, but I run into questions when trying to update view. As discussed above, I should make the callers call normalize_heads() before they attempt to access view() with a dirty cell. But how soon should they call that?

One answer that I believe is wrong is to update as soon as the cell is dirty. I think the existing code tries to avoid that, probably because it wants to reduce the number of calls to the indexer? Let me know whether this is right.

My guess to the answer is to call normalize_heads() as late as possible. Is this correct? I'm stuck here because I don't know where that should be. I also wonder if there are interleaving function calls that makes the view dirty and reads view() again, which makes it hard for a caller to know when to call normalize_heads()

Another question is even if we make view() panic, we have to be careful of MutableRepo member functions that calls self.view instead of self.view(), thus getting duplicates. Is this a concern?

[yuja]

I was able to update has_changes, merge and consume successfully, but I run into questions when trying to update view. As discussed above, I should make the callers call normalize_heads() before they attempt to access view() with a dirty cell. But how soon should they call that?

So IMHO, it seems better to let the callers decide, and allow duplicates in the in-memory mutable view.heads() (rather than making view() panic).

// Use normalize/normalized_heads() because the caller expects non-head
// revisions don't exist in the set. ReadonlyRepo just forwards view.heads.
repo.view().normalize_heads()?.contains(working_commit_id)

// Redundant heads are okay here.
let old_heads = RevsetExpression::commits(old_view.heads().iter().cloned().collect());
let new_heads = RevsetExpression::commits(new_view.heads().iter().cloned().collect());
new_heads.range(&old_heads)

// consume() should return normalized view because the view data will be saved to disk.
view.store_view_mut().heads = ...; // copy normalized heads back to view
...

[kevincliao]

// Use normalize/normalized_heads() because the caller expects non-head
// revisions don't exist in the set. ReadonlyRepo just forwards view.heads.
repo.view().normalize_heads()?.contains(working_commit_id)

Did you mean repo.normalize_heads() instead of repo.view().normalize_heads()? I ask because the View object don't have access to the indexer.

// consume() should return normalized view because the view data will be saved to disk.
view.store_view_mut().heads = ...; // copy normalized heads back to view

Do you mean that the caller should remember the results from repo.view().normalize_heads() and copy it back here? Why not make normalize_heads internally update the view object?

Also, since it's now entirely the caller's responsibility to do the right thing, should MutableRepo.view now be just a View object, and the DirtyCell data structure is now useless and can be removed?

[yuja]

// Use normalize/normalized_heads() because the caller expects non-head
// revisions don't exist in the set. ReadonlyRepo just forwards view.heads.
repo.view().normalize_heads()?.contains(working_commit_id)

Did you mean repo.normalize_heads() instead of repo.view().normalize_heads()? I ask because the View object don't have access to the indexer.

Yes. repo.normalize_heads() in addition to (potentially unnormalized) repo.view().heads().

// consume() should return normalized view because the view data will be saved to disk.
view.store_view_mut().heads = ...; // copy normalized heads back to view

Do you mean that the caller should remember the results from repo.view().normalize_heads() and copy it back here?

No. MutableRepo would cache the normalized heads and copy them back here. consume() will do normalization as needed.

Why not make normalize_heads internally update the view object?

I just assumed it would be easier to implement if view.data.heads didn't have to be updated by immutable normalize_heads(&self). Alternatively, maybe we can add normalize_heads(&mut self) only to MutableRepo. In that case, (non-mut) repo.view().heads() will be considered normalized.

repo.view().heads()      // returns normalized heads
mut_repo.view().heads()  // potentially contains duplicates
mut_repo.normalize_heads()?; mut_repo.view().heads()  // normalized heads

// revset.rs
RevsetExpression::VisibleHeads => {
    if is_head_normalized { // add flag to Repo trait?
        visible_heads
    } else {
        Heads(visible_heads)
    }
}

EDIT: The head_normalized flag can be managed by View instead of MutableRepo. I'm not sure which is better overall.

pub struct View {
    data: op_store::View,
    head_normalized: bool,
}

impl View {
    pub fn normalize_heads(&mut self, index, root_commit_id) -> Result<(), _>
}