Skip to content

Make MetadataWriter more robust#782

Open
jeremypw wants to merge 13 commits intomasterfrom
jeremypw/metadatawriter-replace_assert
Open

Make MetadataWriter more robust#782
jeremypw wants to merge 13 commits intomasterfrom
jeremypw/metadatawriter-replace_assert

Conversation

@jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Nov 22, 2024

  • Add some null checks
  • Remove asserts and replace with critical messages

The codebase contains a lot of places where the app just terminates due to an assertion failure. This does not seem necessary for production code and these have been replaced with critical messages or throwing an error and the subsequent code either omitted or continued as seems appropriate. If reviewers feel an assertion failure would cause user data corruption on continued use then it should be replaced with a call to AppWindow.panic which at least logs a message before terminating.

There were a few places where a null could theoretically be dereferenced.

It is difficult to know whether assertion failure or null dereferencing could occur in practice as the code is extremely convoluted.

@stsdc
Copy link
Member

stsdc commented Nov 23, 2024

At this point I'm expecting you also to approve your PR 😄

obraz

@stsdc stsdc requested a review from zeebok November 23, 2024 10:54
@jeremypw
Copy link
Collaborator Author

On further consideration it is not necessary to explicitly null check in the blacklist_file functions - the Vala compiler automatically inserts a null check. One disadvantage of relying on this is that is not immediately obvious what is returned if the parameter requirements are not met when the function signature returns something. In fact it seems that boolean functions return false (safe enough) and functions returning a pointer return null (might be unsafe).

}
}

// Vala compiler returns false if metadata null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Vala do this check because the argument isn't ending with ? or something else?

assert (removed);

assert (!dirty.contains (photo));
if (!removed || dirty.contains (photo)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking dirty seems redundant here since it is within an if-statement of the same check

@zeebok
Copy link
Contributor

zeebok commented Feb 26, 2025

I just have a couple comments, and there's a conflict to fix. Otherwise this seems good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants