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

Reviews

See the guideline for information on the review process.

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.

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 :)

Thanks for the feedback, will rework it.

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

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

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

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

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:

if [[ $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.

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>.

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:

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

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.

reACK be6d24e

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 :)

~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 be6d24e

Backported to 31.x in #34800.