mirror of
https://github.com/DreamMaoMao/maomaowm.git
synced 2026-05-02 06:46:29 -04:00
Fix markdown linting errors in documentation files
Co-authored-by: squassina <8495707+squassina@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
parent
d344ab8a17
commit
8a7fa8dce2
3 changed files with 156 additions and 87 deletions
|
|
@ -23,24 +23,28 @@ maintainability.
|
|||
**File:** `src/dispatch/bind_define.h:846`
|
||||
**Effort:** 5 minutes
|
||||
|
||||
#### Change Made:
|
||||
#### Change Made
|
||||
|
||||
```diff
|
||||
- if (wordexp(token, &p, 0) == 0 && p.we_wordc > 0) {
|
||||
+ if (wordexp(token, &p, WRDE_NOCMD) == 0 && p.we_wordc > 0) {
|
||||
```
|
||||
|
||||
#### Security Impact:
|
||||
#### Security Impact
|
||||
|
||||
- **Prevents:** Command injection via command substitution (e.g., `$(malicious)`)
|
||||
- **Maintains:** Tilde expansion (`~`) and glob patterns (`*.txt`)
|
||||
- **Risk Mitigation:** Closes medium-priority security vulnerability
|
||||
|
||||
#### Why This Matters:
|
||||
#### Why This Matters
|
||||
|
||||
Without `WRDE_NOCMD`, an attacker who can control spawn arguments (through
|
||||
config file or IPC) could execute arbitrary commands using shell command
|
||||
substitution. This flag blocks that attack vector while preserving useful
|
||||
shell expansion features.
|
||||
|
||||
#### Testing:
|
||||
#### Testing
|
||||
|
||||
- Code compiles successfully
|
||||
- clang-format applied and passed
|
||||
- Change is minimal and localized
|
||||
|
|
@ -55,7 +59,8 @@ shell expansion features.
|
|||
**File:** `meson.build`
|
||||
**Effort:** 15 minutes
|
||||
|
||||
#### Changes Made:
|
||||
#### Changes Made
|
||||
|
||||
Translated 10 Chinese comment lines to English:
|
||||
|
||||
1. Line 18: `"如果 sysconfdir 以 prefix 开头，去掉 prefix"`
|
||||
|
|
@ -88,7 +93,8 @@ Translated 10 Chinese comment lines to English:
|
|||
10. Line 91: `"链接参数（根据 debug 状态添加 ASAN）"`
|
||||
→ `"Link arguments (add ASAN based on debug state)"`
|
||||
|
||||
#### Impact:
|
||||
#### Impact
|
||||
|
||||
- **Accessibility:** International contributors can now understand build system
|
||||
- **Consistency:** Matches English-only comments in source code
|
||||
- **Collaboration:** Reduces language barriers for new contributors
|
||||
|
|
@ -103,10 +109,11 @@ Translated 10 Chinese comment lines to English:
|
|||
**File:** `TECHNICAL_DEBT.md` (new)
|
||||
**Effort:** 30 minutes
|
||||
|
||||
#### What Was Created:
|
||||
#### What Was Created
|
||||
|
||||
A comprehensive tracking document for all TODO/FIXME items in the codebase.
|
||||
|
||||
#### Items Documented:
|
||||
#### Items Documented
|
||||
|
||||
1. **Mouse Scroll Wheel Support** (`src/mango.c:1803`)
|
||||
- Priority: Low
|
||||
|
|
@ -133,14 +140,16 @@ A comprehensive tracking document for all TODO/FIXME items in the codebase.
|
|||
- Effort: Medium-Large (4-8 hours)
|
||||
- Impact: User experience improvement
|
||||
|
||||
#### Document Structure:
|
||||
#### Document Structure
|
||||
|
||||
- Clear descriptions of each item
|
||||
- Code location and context
|
||||
- Priority and effort estimates
|
||||
- Impact analysis
|
||||
- Contribution guidelines
|
||||
|
||||
#### Benefits:
|
||||
#### Benefits
|
||||
|
||||
- **Visibility:** All technical debt in one place
|
||||
- **Prioritization:** Clear priority levels for contributors
|
||||
- **Onboarding:** New contributors can easily find improvement opportunities
|
||||
|
|
@ -150,7 +159,7 @@ A comprehensive tracking document for all TODO/FIXME items in the codebase.
|
|||
|
||||
## Files Modified
|
||||
|
||||
```
|
||||
```text
|
||||
TECHNICAL_DEBT.md | 143 +++++++++++++++++++++++++++++++++++++++
|
||||
meson.build | 20 ++++++------
|
||||
src/dispatch/bind_define.h | 9 +++---
|
||||
|
|
@ -162,14 +171,17 @@ src/dispatch/bind_define.h | 9 +++---
|
|||
## Quality Assurance
|
||||
|
||||
### Code Style ✅
|
||||
|
||||
- clang-format applied to all C code changes
|
||||
- Formatting passes repository standards
|
||||
|
||||
### Build System ✅
|
||||
|
||||
- meson.build changes maintain build compatibility
|
||||
- Comments improved without affecting functionality
|
||||
|
||||
### Git Hygiene ✅
|
||||
|
||||
- Descriptive commit message
|
||||
- Co-authored with repository maintainer
|
||||
- Changes pushed to feature branch
|
||||
|
|
@ -179,16 +191,19 @@ src/dispatch/bind_define.h | 9 +++---
|
|||
## Impact Assessment
|
||||
|
||||
### Security
|
||||
|
||||
**Before:** Medium-priority vulnerability (command injection possible)
|
||||
**After:** Vulnerability mitigated with WRDE_NOCMD flag
|
||||
**Risk Reduction:** Significant
|
||||
|
||||
### Maintainability
|
||||
|
||||
**Before:** Chinese comments, undocumented technical debt
|
||||
**After:** English-only comments, tracked technical debt
|
||||
**Improvement:** Substantial
|
||||
|
||||
### Code Quality
|
||||
|
||||
**Before:** Good overall, with noted improvement areas
|
||||
**After:** Excellent with recommendations implemented
|
||||
**Grade Improvement:** A- → A
|
||||
|
|
@ -198,19 +213,22 @@ src/dispatch/bind_define.h | 9 +++---
|
|||
## Next Steps
|
||||
|
||||
### Immediate (Completed) ✅
|
||||
|
||||
1. ✅ Security fix implemented
|
||||
2. ✅ Comments translated
|
||||
3. ✅ Technical debt documented
|
||||
|
||||
### Short Term (Optional)
|
||||
|
||||
1. Consider addressing Medium-priority technical debt item #5
|
||||
2. Review other wordexp() usage in codebase for consistency
|
||||
3. Update REVIEW_FINDINGS.md to mark recommendations as completed
|
||||
|
||||
### Long Term (Optional)
|
||||
4. Address Low-priority technical debt items as time permits
|
||||
5. Add automated security scanning to CI/CD pipeline
|
||||
6. Consider adding unit tests for utility functions
|
||||
|
||||
1. Address Low-priority technical debt items as time permits
|
||||
2. Add automated security scanning to CI/CD pipeline
|
||||
3. Consider adding unit tests for utility functions
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ separation of concerns. This review identifies both strengths and areas for
|
|||
potential improvement.
|
||||
|
||||
**Overall Assessment:**
|
||||
|
||||
- ✅ **Security:** GOOD - No critical vulnerabilities found
|
||||
- ✅ **Performance:** GOOD - Well-optimized for real-time rendering
|
||||
- ✅ **Clarity:** GOOD - Clear structure with comprehensive comments
|
||||
|
|
@ -30,6 +31,7 @@ potential improvement.
|
|||
### ✅ Strengths
|
||||
|
||||
#### Memory Safety
|
||||
|
||||
- **Checked Allocations:** All memory allocations use `ecalloc()` wrapper that
|
||||
checks for allocation failures and terminates gracefully
|
||||
- Location: `src/common/util.c:31-37`
|
||||
|
|
@ -46,6 +48,7 @@ potential improvement.
|
|||
- Location: `src/config/parse_config.h:1250`
|
||||
|
||||
#### Process Spawning
|
||||
|
||||
- **Shell Command Execution:** Uses `execlp()` properly with shell as
|
||||
intermediary
|
||||
- Location: `src/dispatch/bind_define.h:796-821` (`spawn_shell()`)
|
||||
|
|
@ -59,6 +62,7 @@ potential improvement.
|
|||
- Proper cleanup of allocated strings on failure
|
||||
|
||||
#### Input Validation
|
||||
|
||||
- **Regex Matching:** Uses PCRE2 library with proper error handling
|
||||
- Location: `src/common/util.c:53-79`
|
||||
- UTF-8 support enabled: `PCRE2_UTF` flag
|
||||
|
|
@ -73,6 +77,7 @@ potential improvement.
|
|||
### ⚠️ Areas of Concern
|
||||
|
||||
#### 1. wordexp() Security Risk (MEDIUM)
|
||||
|
||||
**Location:** `src/dispatch/bind_define.h:846`
|
||||
|
||||
```c
|
||||
|
|
@ -89,18 +94,23 @@ substitution. If an attacker can control the config file or IPC commands, they
|
|||
could inject shell commands.
|
||||
|
||||
**Risk Assessment:**
|
||||
|
||||
- **Likelihood:** LOW - Config file is user-owned (~/.config/mango/config.conf)
|
||||
- **Impact:** HIGH - Could execute arbitrary commands as the user
|
||||
- **Overall:** MEDIUM risk
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
- Use `WRDE_NOCMD` flag to disable command substitution:
|
||||
|
||||
```c
|
||||
if (wordexp(token, &p, WRDE_NOCMD) == 0 && p.we_wordc > 0) {
|
||||
```
|
||||
|
||||
- This maintains tilde/glob expansion while blocking command execution
|
||||
|
||||
#### 2. Signal Handler Safety (LOW)
|
||||
|
||||
**Location:** `src/dispatch/bind_define.h:802-804, 830-832`
|
||||
|
||||
```c
|
||||
|
|
@ -113,6 +123,7 @@ signal(SIGILL, SIG_IGN);
|
|||
could aid debugging.
|
||||
|
||||
**Risk Assessment:**
|
||||
|
||||
- **Likelihood:** N/A - Design choice
|
||||
- **Impact:** LOW - Only affects debugging
|
||||
- **Overall:** LOW concern
|
||||
|
|
@ -122,17 +133,20 @@ configurable for development builds. Core dumps are valuable for debugging
|
|||
crashes in spawned processes.
|
||||
|
||||
#### 3. XWayland Attack Surface (LOW)
|
||||
|
||||
**Location:** `meson.build:87-89`, `src/mango.c:90-94`
|
||||
|
||||
**Issue:** XWayland support increases attack surface by including X11 protocol
|
||||
handling.
|
||||
|
||||
**Risk Assessment:**
|
||||
|
||||
- **Likelihood:** LOW - XWayland is optional (compile-time flag)
|
||||
- **Impact:** MEDIUM - X11 protocol has historical security issues
|
||||
- **Overall:** LOW risk
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
- Document security implications of enabling XWayland
|
||||
- Consider disabling by default for security-conscious deployments
|
||||
- Current implementation is acceptable with compile-time option
|
||||
|
|
@ -150,9 +164,10 @@ handling.
|
|||
|
||||
## 2. Performance Review
|
||||
|
||||
### ✅ Strengths
|
||||
### ✅ Performance Strengths
|
||||
|
||||
#### Rendering Optimization
|
||||
|
||||
- **Scene Graph Architecture:** Uses wlroots scene graph for efficient rendering
|
||||
- Delegates to SceneFX library for GPU-accelerated effects
|
||||
- Location: Scene setup throughout `src/mango.c`
|
||||
|
|
@ -169,6 +184,7 @@ handling.
|
|||
- Smooth 60+ FPS animations without recalculating curves
|
||||
|
||||
#### Memory Management
|
||||
|
||||
- **Efficient Allocations:** Uses `ecalloc()` wrapper that zeros memory
|
||||
- Prevents uninitialized memory bugs
|
||||
- Location: `src/common/util.c:31-37`
|
||||
|
|
@ -181,6 +197,7 @@ handling.
|
|||
- Proper `malloc()`/`free()` pattern with cleanup
|
||||
|
||||
#### Time Management
|
||||
|
||||
- **Monotonic Clock:** Uses `CLOCK_MONOTONIC` for timing
|
||||
- Location: `src/common/util.c:85-94`
|
||||
- Immune to system time adjustments
|
||||
|
|
@ -189,13 +206,16 @@ handling.
|
|||
### ⚠️ Performance Notes
|
||||
|
||||
#### 1. Temporary Array Allocations (MINOR)
|
||||
|
||||
**Locations:**
|
||||
|
||||
- `src/layout/vertical.h:294` - `tempClients = malloc(n * sizeof(Client *))`
|
||||
- `src/layout/horizontal.h:308` - Similar pattern
|
||||
|
||||
**Observation:** Layout functions allocate temporary arrays on every arrange call.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- **Frequency:** Triggered on window open/close/resize/tag change
|
||||
- **Cost:** Small - allocations are typically < 100 windows
|
||||
- **Overall:** ACCEPTABLE for current implementation
|
||||
|
|
@ -204,18 +224,20 @@ handling.
|
|||
for common cases (e.g., < 32 windows).
|
||||
|
||||
#### 2. Config Parsing Uses realloc() (MINOR)
|
||||
|
||||
**Location:** `src/config/parse_config.h` (multiple instances)
|
||||
|
||||
**Observation:** Configuration arrays grown with `realloc()` during parsing.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- **Frequency:** Only during startup and config reload
|
||||
- **Cost:** Acceptable - config parsing is not performance-critical
|
||||
- **Overall:** ACCEPTABLE
|
||||
|
||||
**Note:** This is fine for config parsing, which is not a hot path.
|
||||
|
||||
### ✅ Good Practices Observed
|
||||
### ✅ Performance Best Practices
|
||||
|
||||
1. **Render Loop Efficiency:** Only renders when needed (`need_more_frames` flag)
|
||||
2. **Data Structure Choice:** Wayland linked lists for O(1) insertion/removal
|
||||
|
|
@ -227,11 +249,13 @@ for common cases (e.g., < 32 windows).
|
|||
|
||||
## 3. Clarity Review
|
||||
|
||||
### ✅ Strengths
|
||||
### ✅ Code Clarity Strengths
|
||||
|
||||
#### Code Organization
|
||||
|
||||
- **Modular Structure:** Clear separation of concerns
|
||||
```
|
||||
|
||||
```text
|
||||
src/
|
||||
├── animation/ # Animation system
|
||||
├── client/ # Window/client management
|
||||
|
|
@ -248,6 +272,7 @@ for common cases (e.g., < 32 windows).
|
|||
- Clear that functions are not part of public API
|
||||
|
||||
#### Naming Conventions
|
||||
|
||||
- **Clear Function Names:** Self-documenting
|
||||
- Examples: `spawn_shell()`, `focusclient()`, `arrangelayers()`
|
||||
- Follows consistent verb-noun pattern
|
||||
|
|
@ -261,6 +286,7 @@ for common cases (e.g., < 32 windows).
|
|||
- Location: `src/common/util.h:7` comment documents this
|
||||
|
||||
#### Comments and Documentation
|
||||
|
||||
- **Function Documentation:** Most functions have purpose comments
|
||||
- Example: Animation functions explain curve types
|
||||
- Location: Throughout `src/animation/`
|
||||
|
|
@ -274,6 +300,7 @@ for common cases (e.g., < 32 windows).
|
|||
- Examples: `ISTILED`, `VISIBLEON`, `CLEANMASK`
|
||||
|
||||
#### Code Formatting
|
||||
|
||||
- **Consistent Style:** Uses clang-format for formatting
|
||||
- Configuration: `.clang-format` present in repository
|
||||
- Script: `format.sh` for easy reformatting
|
||||
|
|
@ -282,7 +309,9 @@ for common cases (e.g., < 32 windows).
|
|||
### ⚠️ Areas for Improvement
|
||||
|
||||
#### 1. TODO/FIXME Items (LOW PRIORITY)
|
||||
|
||||
**Locations found:**
|
||||
|
||||
- `src/mango.c:1803` - "TODO: allow usage of scroll wheel for mousebindings"
|
||||
- `src/mango.c:3537` - "TODO handle other input device types"
|
||||
- `src/mango.c:3545` - "TODO do we actually require a cursor?"
|
||||
|
|
@ -290,14 +319,17 @@ for common cases (e.g., < 32 windows).
|
|||
- `src/mango.c:5982` - "FIXME: figure out why cursor image is at 0,0"
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
- Create GitHub issues for each TODO/FIXME
|
||||
- Track as technical debt items
|
||||
- Not urgent - code functions correctly despite TODOs
|
||||
|
||||
#### 2. Some Chinese Comments in meson.build (MINOR)
|
||||
|
||||
**Location:** `meson.build:18, 22, 27-29, 44`
|
||||
|
||||
**Examples:**
|
||||
|
||||
- Line 18: `# 如果 sysconfdir 以 prefix 开头，去掉 prefix`
|
||||
- Line 22: `# 确保 sysconfdir 是绝对路径`
|
||||
- Line 27-29: Debug output comments
|
||||
|
|
@ -306,21 +338,25 @@ for common cases (e.g., < 32 windows).
|
|||
**Impact:** Reduces accessibility for international contributors
|
||||
|
||||
**Recommendation:** Translate to English for consistency
|
||||
|
||||
- Translation examples:
|
||||
- Line 18: "If sysconfdir starts with prefix, remove prefix"
|
||||
- Line 22: "Ensure sysconfdir is an absolute path"
|
||||
- Line 44: "Get version information"
|
||||
|
||||
#### 3. Magic Numbers (VERY MINOR)
|
||||
|
||||
**Examples:**
|
||||
|
||||
- `src/dispatch/bind_define.h:838` - `char *argv[64]` - Why 64?
|
||||
- `src/animation/common.h` - Various curve point counts
|
||||
|
||||
**Recommendation:** Define named constants for magic numbers
|
||||
|
||||
- Example: `#define MAX_SPAWN_ARGS 64`
|
||||
- Improves maintainability and documents rationale
|
||||
|
||||
### ✅ Good Practices Observed
|
||||
### ✅ Clarity Best Practices
|
||||
|
||||
1. **English Comments:** Primary codebase comments are in English
|
||||
2. **Consistent Naming:** Functions, variables, and types follow conventions
|
||||
|
|
@ -333,6 +369,7 @@ for common cases (e.g., < 32 windows).
|
|||
## 4. Additional Observations
|
||||
|
||||
### Build System (meson.build)
|
||||
|
||||
- **ASAN Support:** Optional AddressSanitizer for memory debugging
|
||||
- Flag: `get_option('asan')`
|
||||
- Lines 79-85, 92-95
|
||||
|
|
@ -346,19 +383,24 @@ for common cases (e.g., < 32 windows).
|
|||
- Good practice: prevents incompatible versions
|
||||
|
||||
### Testing
|
||||
|
||||
**Observation:** No test suite found in repository.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- Makes refactoring riskier
|
||||
- Manual testing required for regressions
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
- Consider adding integration tests for critical paths
|
||||
- Unit tests for utility functions (regex_match, time functions)
|
||||
- Not urgent for a compositor (difficult to test), but valuable long-term
|
||||
|
||||
### Documentation
|
||||
|
||||
**Present:**
|
||||
|
||||
- `README.md` - Project overview and setup
|
||||
- `COMMANDS.md` - Command reference (1209 lines)
|
||||
- `USAGE.md` - User guide (819 lines)
|
||||
|
|
@ -371,6 +413,7 @@ for common cases (e.g., < 32 windows).
|
|||
## 5. Recommendations Summary
|
||||
|
||||
### High Priority (Security) ✅ COMPLETED
|
||||
|
||||
1. **Add WRDE_NOCMD flag to wordexp()** - Prevents command injection
|
||||
- File: `src/dispatch/bind_define.h:846`
|
||||
- Change: `wordexp(token, &p, WRDE_NOCMD)`
|
||||
|
|
@ -378,28 +421,31 @@ for common cases (e.g., < 32 windows).
|
|||
- **Status:** ✅ Implemented in commit d97ec4a
|
||||
|
||||
### Medium Priority (Code Quality) ✅ COMPLETED
|
||||
2. **Translate Chinese comments to English** - Improves international
|
||||
|
||||
1. **Translate Chinese comments to English** - Improves international
|
||||
collaboration
|
||||
- File: `meson.build`
|
||||
- Estimated effort: 15 minutes
|
||||
- **Status:** ✅ Implemented in commit d97ec4a (10 lines translated)
|
||||
|
||||
3. **Convert TODO/FIXME to GitHub issues** - Track technical debt
|
||||
2. **Convert TODO/FIXME to GitHub issues** - Track technical debt
|
||||
- Create issues for 5 TODO items
|
||||
- Estimated effort: 30 minutes
|
||||
- **Status:** ✅ Implemented in commit d97ec4a (documented in TECHNICAL_DEBT.md)
|
||||
|
||||
### Low Priority (Nice to Have)
|
||||
4. **Replace magic numbers with named constants**
|
||||
|
||||
1. **Replace magic numbers with named constants**
|
||||
- Various files
|
||||
- Estimated effort: 1-2 hours
|
||||
|
||||
5. **Consider adding basic tests**
|
||||
2. **Consider adding basic tests**
|
||||
- Start with utility function tests
|
||||
- Estimated effort: Several days (ongoing)
|
||||
|
||||
### Optional (Documentation)
|
||||
6. **Document XWayland security implications**
|
||||
|
||||
1. **Document XWayland security implications**
|
||||
- Add security section to README
|
||||
- Estimated effort: 30 minutes
|
||||
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ these notes.
|
|||
**Location:** `src/mango.c:1803-1804`
|
||||
|
||||
**Current Code:**
|
||||
|
||||
```c
|
||||
/* TODO: allow usage of scroll whell for mousebindings, it can be
|
||||
* implemented checking the event's orientation and the delta of the event
|
||||
|
|
@ -36,6 +37,7 @@ require checking the event's orientation and delta values.
|
|||
**Location:** `src/mango.c:3537`
|
||||
|
||||
**Current Code:**
|
||||
|
||||
```c
|
||||
/* TODO handle other input device types */
|
||||
```
|
||||
|
|
@ -56,6 +58,7 @@ there may be edge cases or newer device types not yet handled.
|
|||
**Location:** `src/mango.c:3545`
|
||||
|
||||
**Current Code:**
|
||||
|
||||
```c
|
||||
/* TODO do we actually require a cursor? */
|
||||
```
|
||||
|
|
@ -76,6 +79,7 @@ needed.
|
|||
**Location:** `src/mango.c:4782-4783`
|
||||
|
||||
**Current Code:**
|
||||
|
||||
```c
|
||||
/* TODO hack to get cursor to display in its initial location (100, 100)
|
||||
* instead of (0, 0) and then jumping. still may not be fully
|
||||
|
|
@ -99,6 +103,7 @@ be properly fixed.
|
|||
**Location:** `src/mango.c:5982-5983`
|
||||
|
||||
**Current Code:**
|
||||
|
||||
```c
|
||||
/* FIXME: figure out why the cursor image is at 0,0 after turning all
|
||||
* the monitors on.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue