partitioning: fix #9201 /etc/fstab entry (double comma)#9488
partitioning: fix #9201 /etc/fstab entry (double comma)#9488
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNormalization of mount option concatenation in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/image/partitioning.sh (1)
345-345: Avoid overridingmountopts[btrfs]inside the btrfs block.Line 345 duplicates the default from Line 85 and can silently override custom mount options set earlier via hooks. Consider removing this reassignment and keeping a single source of truth.
♻️ Proposed cleanup
- mountopts[$ROOTFS_TYPE]=',commit=120'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/functions/image/partitioning.sh` at line 345, Inside the btrfs-specific block you are reassigning mountopts[$ROOTFS_TYPE] to ',commit=120' which overrides any earlier customizations; remove that reassignment (or change it to only set a default when mountopts[$ROOTFS_TYPE] is empty, e.g., test -z "${mountopts[$ROOTFS_TYPE]}" && mountopts[$ROOTFS_TYPE]=',commit=120') so mountopts[btrfs] remains the single source of truth and hooks can override it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/functions/image/partitioning.sh`:
- Line 345: Inside the btrfs-specific block you are reassigning
mountopts[$ROOTFS_TYPE] to ',commit=120' which overrides any earlier
customizations; remove that reassignment (or change it to only set a default
when mountopts[$ROOTFS_TYPE] is empty, e.g., test -z
"${mountopts[$ROOTFS_TYPE]}" && mountopts[$ROOTFS_TYPE]=',commit=120') so
mountopts[btrfs] remains the single source of truth and hooks can override it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d935693f-6500-43c3-a9e1-ba6e343e4c89
📒 Files selected for processing (1)
lib/functions/image/partitioning.sh
mountopts[] values start with a leading comma by convention (line 26), but all call sites added an explicit comma before the expansion, producing e.g. `defaults,,commit=120,errors=remount-ro` for ext4. Also removed duplicate mountopts[btrfs] override (line 345 duplicated line 85 and would silently clobber options set by hooks). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
iav
force-pushed
the
fix/9201-fstab-double-comma
branch
from
ee8bec9 to
e267a65
Compare
1 participant
Summary
Fixes #9201: double-comma bug in generated
/etc/fstabentries (e.g.defaults,,commit=120,errors=remount-rofor ext4).The root cause:
mountopts[]values start with a leading comma by convention (see line 26 comment), but all call sites were adding an explicit comma before the expansion, doubling it.This is the complete fix suggested by @EvilOlaf in #9227 (#9227 (comment)) — removing the hardcoded
,afterdefaultsat all call sites, rather than patching only line 369.Changes
mountandfstabentrymountandfstabentries (not covered by partitioning: fix malformed fstab double comma and switch to relatime #9227)mountoptsoverride was missing the leading comma, accidentally compensated by the extra comma at call sites — fixed to follow the conventionSummary by CodeRabbit