Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overwrite+remove deleting the wrong file on relative path. #235

Merged

Conversation

EXio4
Copy link
Contributor

@EXio4 EXio4 commented Aug 18, 2024

Fixes #234

The issue seems to be happening due to:

    auto oldFilePath = pathToOldAppImage(pathToAppImage.value(), newFilePath);

checking in pathToOldAppImage if the paths are [trivially] equal with a == before, but newFilePath is gathered through pathToNewFile, which gets the data from the zSyncClient object:

    bool Updater::pathToNewFile(std::string& path) const {
        // only available update method is via ZSync
        if (d->zSyncClient)
            return d->zSyncClient->pathToNewFile(path);

        return false;
    }

which internally calls applyCwdToPathToLocalFile, which one of the 3 places where pathToLocalFile is modified:

  1. constructor, when the file needs to be overwritten
  2. populatePathToLocalFileFromZSyncFile
  3. applyCwdToPathToLocalFile
        void applyCwdToPathToLocalFile() {
            if (strncmp(pathToLocalFile.c_str(), "/", 1) == 0)
                return;

            auto oldPath = pathToLocalFile;
            pathToLocalFile = cwd;
            if (!endsWith(pathToLocalFile, "/"))
                pathToLocalFile += "/";
            pathToLocalFile += oldPath;
        }

which will make relative paths absolute, and keep absolute paths 100% equal

I haven't touched this codebase ever, so I'm just going based off vibes for the solution, if there's a better place to put to abspath - to keep the change minimal, that could likely be better, from what I can tell there's quite a few places in updater.cpp where the paths are normalized, like Updater::restoreOriginalFile and Updater::copyPermissionsToNewFile 😅

I'm not using Linux - so @Samueru-sama has been testing the code 🫡

Issue was caused by updater.pathToNewFile() fetching the path directly from zSyncClient, which will keep absolute paths as-is
and change relative paths to be different than the parameter value would be.
@Samueru-sama
Copy link

This might also fix #174 however I wasn't able to replicate that issue exactly during testing, by that I mean with only with the -r flag instead of -Or.

This PR does indeed fix the issue with -Or though.

@TheAssassin TheAssassin enabled auto-merge (rebase) September 4, 2024 21:52
@TheAssassin
Copy link
Member

Thanks!

@TheAssassin TheAssassin enabled auto-merge (squash) September 4, 2024 21:52
@TheAssassin TheAssassin merged commit f716590 into AppImageCommunity:main Sep 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

appimageupdatetool removes the wrong file
3 participants