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

files: fix use of onChange with directory source #2031

Merged
merged 1 commit into from
May 29, 2021
Merged

Conversation

rycee
Copy link
Member

@rycee rycee commented May 20, 2021

Description

Previously, the comparison would not handle directory comparison correctly, always finding that the source and target differed. This would trigger the onChange script on each activation.

Fixes #2004

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

Author approved the change here: #2004 (comment)

Previously, the comparison would not handle directory comparison
correctly, always finding that the source and target differed. This
would trigger the `onChange` script on each activation.

Fixes #2004
@rycee rycee merged commit 07ad6a4 into master May 29, 2021
@rycee rycee deleted the onchange-dirs branch May 29, 2021 22:42
@rycee
Copy link
Member Author

rycee commented May 29, 2021

@berbiche Thanks for the ping! Merged now.

@bb010g
Copy link
Contributor

bb010g commented May 30, 2021

@rycee Why was the recursive assertion added?

@rycee
Copy link
Member Author

rycee commented May 30, 2021

The existing code doesn't handle onChange in a reasonable way when recursive = true so I figured it would be best to stop allowing its use. To do it properly one would have to compare the included files one by one. Perhaps something like

find $sourceDir -type l -printf '%P\n' | xargs -i cmp --quiet "$sourceDir/{}" "$targetDir/{}"

But yeah, I guess it should be safe to remove the assertion anyway. It'll always run the onChange as before, which shouldn't be a problem, since it should be idempotent. I'll make a PR for it.

rycee added a commit that referenced this pull request May 30, 2021
chisui pushed a commit to chisui/home-manager that referenced this pull request May 31, 2021
rycee added a commit that referenced this pull request Jun 4, 2021
See discussion in

  #2031

(cherry picked from commit d2aaeac)
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.

xdg.configFile.<name>.onChange always triggered
4 participants