Skip to content

Fix some “Stage All” special cases#1037

Merged
lucasderraugh merged 5 commits into
git-up:masterfrom
Cykelero:fix-staging-special-cases
Feb 18, 2025
Merged

Fix some “Stage All” special cases#1037
lucasderraugh merged 5 commits into
git-up:masterfrom
Cykelero:fix-staging-special-cases

Conversation

@Cykelero
Copy link
Copy Markdown
Contributor

@Cykelero Cykelero commented Dec 3, 2024

This fixes two issues with the Stage All button.

Reproducing

Reproducing the conflict issue:

  • Cause a conflict from another app (e.g. by running a git pull)
  • Delete the file that's in conflict
  • In GitUp, try to Stage All: the operation aborts with a “No such file or directory” error

Reproducing the submodule issue:

  • In a repository, copy another repository (just copy a repo folder; don't use submodule features or anything)
  • In GitUp, try to Stage All in the outer repository: the operation aborts with a “submodule 'SUB_REPO_PATH' has not been added yet” error

Notes

The fixes look at the error messages to decide what to do—that might not be ideal, but:

  • The specific submodule error is emitted by libgit2, with no distinguishing metadata. To avoid reading the error message, we'd have to essentially reimplement that libgit2 behavior. The fix should hold over time, as libgit2 seems to change existing code fairly rarely, and is English-only. The fallout if the fix does break is minimal.
  • The “no such file” error also doesn't have distinguishing features that I could find (its code is a generic -1). That one feels flakier, as Apple could change the error message at any time, and the message could be localized if the app runs in a different language; but in practice, Apple is unlikely to change that message, and GitUp is in English only.

(Not always but) repository folders that weren't registered in .gitmodules would cause the method to fail: “libgit2 error (-4): submodule 'submodules/transfer-photo-albums/' has not been added yet”
When addFileInWorkingDirectory encountered an error, it would return YES regardless.
Comment on lines -82 to +83
if (failed && shouldWriteRepository) {
if (shouldWriteRepository) {
if (shouldWriteRepository) {
if (failed) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey Nathan (@Cykelero), apologies for taking forever on this. I'm trying to wrap my head around the logic here. Previously if there was any failure we wouldn't write anything, but that's no longer the case. However, the logic in the for loop is to break on a the first failed thing to add.

With the changes made here, is it now more appropriate to continue rather than break (or else statement)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problem for the delay, of course.

I think you're right about it being a better idea to continue—I had carried over a habit from Retcon (as soon as anything goes wrong, it tries to immediately stop touching anything) but that's not a good fit in GUK. Fixed!

(also, I renamed shouldWriteRepository to needsToWriteIndex, for clarity)

Also, rename `shouldWriteRepository` to the more appropriate `needsToWriteIndex`.
GCSubmodule* submodule = [self lookupSubmoduleWithName:delta.canonicalPath error:error];
if (!submodule || ![self addSubmoduleToRepositoryIndex:submodule error:error]) {
return NO;
if (![[*error localizedDescription] hasSuffix:@"' has not been added yet"]) {
Copy link
Copy Markdown
Collaborator

@lucasderraugh lucasderraugh Feb 10, 2025

Choose a reason for hiding this comment

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

Also, is it possible error is NULL here? If I remember my C-isms well enough, dereferencing a NULL error would crash, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I fixed both instances of that bug.

Of note, my fix is a bit too simple: it effectively disables this PR's bugfix, when no error is passed. But, that new bug is niche enough that it should be OK; handling it right would mean more code, and some pointer juggling I'm not completely comfortable with.

@lucasderraugh lucasderraugh merged commit 92960f1 into git-up:master Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants