guix: Make guix-clean more careful

hodlinator commented at 10:12 am on March 9, 2026: contributor

Show preview and ask for confirmation before git clean unless used with “–force”
Error out when user tries to pass args such as “guix-clean –help”

DrahtBot added the label Build system on Mar 9, 2026

DrahtBot commented at 10:12 am on March 9, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	kevkevinpal, sedited, willcl-ark, janb84
Stale ACK	svanstaa

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34790 (guix: introduce –exclude parameter to guix-clean by kevkevinpal)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hodlinator renamed this:
~~guix: Make guix-clean less destructive~~
guix: Make guix-clean more cautious
on Mar 9, 2026

hodlinator renamed this:
~~guix: Make guix-clean more cautious~~
guix: Make guix-clean more careful
on Mar 9, 2026

willcl-ark commented at 10:18 am on March 9, 2026: member

I am using guix-clean in scripts, and I think this would the ability to do that. I would prefer, if we make this change, to also accept a --force or -y flag which means my automation would still be able to clean automagically :)

hodlinator marked this as a draft on Mar 9, 2026

hodlinator commented at 10:21 am on March 9, 2026: contributor

Thanks for the feedback, will rework it.

hodlinator force-pushed on Mar 9, 2026

DrahtBot added the label CI failed on Mar 9, 2026

hodlinator force-pushed on Mar 9, 2026

hodlinator marked this as ready for review on Mar 9, 2026

hodlinator commented at 10:53 am on March 9, 2026: contributor

Now supports non-interactive scripts through “–force” in response to #34776 (comment).

hodlinator force-pushed on Mar 9, 2026

DrahtBot removed the label CI failed on Mar 9, 2026

kevkevinpal commented at 3:43 pm on March 9, 2026: contributor

A good follow-up would be to add a --exclude-dir or --exclude option to ignore specific files/directories

kevkevinpal commented at 3:45 pm on March 9, 2026: contributor

tACK beb87b9

Was able to test that both --force and the promt for yes or no worked.

I like the addition of the dry run to explain what is going to be deleted aswell

sedited commented at 5:14 pm on March 9, 2026: contributor

Seems fine, I think the first time I ran the script I was also confused about what it actually removes.

svanstaa commented at 6:44 pm on March 9, 2026: none

tACK beb87b9

Tested:

unknown argument gets rejected
prompt for y works as expected
prompt for n aborts the script
argument –force works

Nit: line 91 uses single brackets which is inconsistent with the double brackets used elsewhere. Since the script is bash-only, I suggest using [[ ]] throughout:

0if [[ $FORCE == 0 ]]; then

Also, before I was not aware that guix-clean actually nukes all untracked files in the whole Bitcoin repo, not just the Guix build artifacts! So the name is a bit misleading. Good call to warn the user before about what exactly will be deleted.

guix: Make guix-clean less destructive

* Show preview and ask for confirmation before git clean unless used with "--force"
* Error out when trying to pass args such as "guix-clean --help"

be6d24ec22

hodlinator force-pushed on Mar 9, 2026

hodlinator commented at 7:23 pm on March 9, 2026: contributor

Nit: line 91 uses single brackets which is inconsistent with the double brackets used elsewhere. Since the script is bash-only, I suggest using [[ ]] throughout:

Aargh, I couldn’t live with this inconsistency - fixed in latest push. Double brackets indeed seem like the way to go. Thanks for spotting!

A good follow-up would be to add a --exclude-dir or --exclude option to ignore specific files/directories

Indeed, that would be useful. Would be tempting to switch to Python argparse for that though. With this PR as it is currently, one can just abort after the preview and manually rm -rf <copied> <subset> <from> <preview> <list>.

janb84 commented at 10:22 am on March 10, 2026: contributor

~0 Not sure this is the way to go, given the functionality and that it will force people to change scripts (add –force) to get the same functionality.

The functionality feels more like a --dry-run option, which would give users the same functionality but doesn’t break backwards compatibility. (The option used in git clean is even the -n or --dry-run)

NIT: The current commit message does not reflect functionality: “guix: Make guix-clean less destructive”. This insinuates that the current command is overly destructive (debatable) and that there is a code change to lessen it’s destructiveness.

hodlinator commented at 12:39 pm on March 10, 2026: contributor

Not sure this is the way to go, given the functionality and that it will force people to change scripts (add –force) to get the same functionality.

I think the cost of breaking non-interactive scripts through requiring adding --force is a price worth paying to protect innocent users running guix-clean to see what it does, having their un-indexed files wiped. Think I’ve managed to do something like that a couple of times.

guix-clean --force also works for scripts running on older versions/master, as any args are ignored there.

Requiring -n in order to opt-in to being less destructive would indeed match git clean but goes against the intent of the PR of making it less of a foot-gun.

One could detect whether both stdin & stdout are the terminal and change defaults based off that:

0FORCE=1
1# Don't force if we are being run in a terminal
2if [[ -t 0 && -t 1 ]]; then
3    FORCE=0
4fi

It’s not air-tight though since it will also turn forcing off when run through 1+ layers of bash script directly in the terminal. WDYT?

NIT: The current commit message does not reflect functionality: “guix: Make guix-clean less destructive”. This insinuates that the current command is overly destructive (debatable) and that there is a code change to lessen it’s destructiveness.

The insinuation is true IMO, we should be more careful, as we have been after the wallet migration bug (reminds me I should review #34439). Will synchronize commit message & PR titles if I re-touch.

kevkevinpal commented at 5:39 pm on March 10, 2026: contributor

reACK be6d24e

sedited approved

sedited commented at 1:42 pm on March 11, 2026: contributor

ACK be6d24ec22cc2ef8086098d6322b024e07c5758c

If someone has the clean script as part of an automatic deployment, I think it’s an ok trade off at this point to have them add the --force flag.

willcl-ark commented at 9:02 pm on March 11, 2026: member

If someone has the clean script as part of an automatic deployment, I think it’s an ok trade off at this point to have them add the --force flag.

Yes I agree, this is fine :)

willcl-ark approved

willcl-ark commented at 9:10 pm on March 11, 2026: member

ACK be6d24ec22cc2ef8086098d6322b024e07c5758c

Works for me.

janb84 commented at 9:21 pm on March 11, 2026: contributor

~0 Not sure this is the way to go, given the functionality and that it will force people to change scripts (add –force) to get the same functionality.

The functionality feels more like a --dry-run option, which would give users the same functionality but doesn’t break backwards compatibility. (The option used in git clean is even the -n or --dry-run)

NIT: The current commit message does not reflect functionality: “guix: Make guix-clean less destructive”. This insinuates that the current command is overly destructive (debatable) and that there is a code change to lessen it’s destructiveness.

Given the recent feedback here, my initial conclusion was not correct and it’s seems valuable enough to change the default behavior and/or changing scripts to use --force is a non-issue (happy to be wrong :) )

The code works and reminding the users what they are going to delete, maybe preventing losing valuable work, seems non harmful and good extra precaution.

ACK be6d24ec22cc2ef8086098d6322b024e07c5758c

fanquake added the label Needs Backport (31.x) on Mar 12, 2026

fanquake merged this on Mar 12, 2026

fanquake closed this on Mar 12, 2026

fanquake referenced this in commit 2724c39208 on Mar 12, 2026

fanquake removed the label Needs Backport (31.x) on Mar 12, 2026

fanquake commented at 3:10 pm on March 12, 2026: member

Backported to 31.x in #34800.

guix: Make guix-clean more careful #34776

Reviews

Conflicts