bamaer commented on issue #7807: URL: https://github.com/apache/hop/issues/7807#issuecomment-5886570232
## Scope and findings Git functionality is spread over three surfaces, and the differences between them are not deliberate: | | code | selection model | operates on | |---|---|---|---| | File Explorer git actions | `GitGuiPlugin` | single file/folder | a path prefix | | Git Commit perspective | `GitCommitPerspective` | multi, checkbox tree | individual changed files | | Git perspective | `GitPerspective` | single commit / ref / file | commits and refs | All three are in scope. Where an action exists in more than one place it should share an implementation and behave identically; where a perspective keeps something the others don't, that stays but gets documented. **Vocabulary:** git's own terms are leading — `restore` and `clean`, not `revert`. The explorer's *Revert* becomes *Restore*, and the overlap between *Restore* and *Clean* gets one consistent meaning across the three perspectives. ### 1. Explorer git actions ignore multi-selection The explorer tree is `SWT.MULTI` and exposes `getSelectedFiles()`, but `GitGuiPlugin` only calls `getSelectedFile()`. Selecting three files and choosing *Git Add* stages one, silently. Every explorer git action and `selectionContains()` moves to the plural form. ### 2. Two commit implementations with different semantics `GitGuiPlugin.gitCommit()` activates the commit perspective on desktop and otherwise falls back to `gitCommitOnWeb()` — the path used on Hop Web and when the perspective is disabled in `disabledGuiElements.xml`. The fallback diverges in ways that affect what ends up in the commit: - it calls `UIGit.commit()`, which commits the **entire index**, so unrelated already-staged files are swept in. The perspective uses `commitPaths()`, which scopes the commit and preserves `MERGE_HEAD`. - the pre-commit check runs *after* staging and leaves files staged on refusal; the perspective checks *before* staging and leaves nothing behind. - no amend, and no `getRepositoryState()` guard against partially committing a merge or cherry-pick. Highest-risk item here, and the one most able to damage a user's work. It wants its own PR and unit tests against a temp repository. ### 3. Action parity | action | Explorer | Git Commit | Git | |---|---|---|---| | Add / stage | yes | yes | – | | Unstage | no | yes | – | | Add to `.gitignore` | no | yes | – | | Restore | as *Revert* | yes | per file | | Clean | yes | no | – | | Delete | generic, no git awareness | yes, with reference check | – | | Text / graph diff | no | yes | yes | | Push / pull / fetch | push+pull, no fetch | commit+push only | all three | | Branch create/rename/merge/delete | yes | – | yes | Proposal: add *Unstage*, *Add to .gitignore* and the diff actions to the explorer (all pure delegations), add *fetch* where push/pull are offered, and keep *Clean* explorer-only as a documented difference. ### 4. Restore / Clean / Delete overlap Three actions with overlapping effects and three different confirmation shapes: - explorer *Revert* leaves new files on disk and points at *Clean*, confirming **after** the fact with a MessageBox; - commit *Restore* confirms up front and folds *Clean* in behind a "delete local copies of added files" toggle; - explorer *Clean* deletes untracked files **without** the reference check `deleteFiles()` performs. One vocabulary, one confirmation shape, and the reference check applied consistently. ### 5. Refresh is one-directional `GitGuiPlugin.refreshGitPerspective()` refreshes the Git perspective or the explorer, never the Git Commit perspective; `gitAdd()` bypasses it entirely and calls `ExplorerPerspective.refresh()` directly. Explorer Add/Revert/Clean/Pull therefore leave an open Git Commit perspective stale. The other direction is wired correctly. One fan-out helper called from every mutating action. ### 6. Three different feedback styles The explorer raises modal MessageBoxes on success; the commit perspective deliberately uses a non-modal inline status line; the Git perspective reports nothing at all. Proposal: adopt the commit perspective's model everywhere — inline status for success, `ErrorDialog` for failure. ### 7. `gitPush()` gives no sign it ran It calls `git.push()`, discards the result, and neither refreshes nor reports. `gitPull()` likewise discards the merged flag that the explorer elsewhere raises a dialog for. One shared push/pull/fetch helper with a single feedback contract. ### 8. Explorer branch deletion force-deletes without confirmation `gitDeleteBranch()` calls `deleteBranch(name, true)` — no confirmation, no unmerged-commit warning, no remote handling. `GitPerspective.deleteReference()` is considerably more careful and handles remote branches and tags. Same split for rename and merge. The explorer should delegate to the Git perspective's implementations. ### 9. Keyboard shortcuts on two surfaces out of three Commit perspective has Ctrl+Alt+A/U/Z, Del, Ctrl+D, Ctrl+Shift+D; Git perspective has Ctrl+D, Ctrl+Shift+D, Ctrl+T, F2; the explorer git actions have none. Same action, same binding, with a sweep for collisions. ### 10. Enablement rules `enableButtons()` derives enablement from a path-prefix scan; the commit perspective computes it per selected `UIFile` at menu-open time. Mostly resolves itself once (1) lands, but the two predicate sets need reconciling deliberately. ### 11. Both perspectives link to documentation that does not exist `GitCommitPerspective` points at `/hop-gui/perspective-git-commit.html` and `GitPerspective` at `/hop-gui/perspective-git.html`. Neither page exists — the only git documentation is `hop-gui-git.adoc`. Both Help buttons are 404s. Both pages need writing, including an explicit section on what differs between the perspectives and why. ## Order of work 1. Refresh fan-out (5) — self-contained, immediately visible. 2. Multi-selection (1) and enablement (10) — the structural change; before any action is added. 3. Commit path (2) — separate PR, with tests. 4. Restore/clean vocabulary (4) — user-visible labels and 10 message bundles. 5. Parity and polish (3, 6, 7, 9). 6. Branch operations (8) — separate PR; the remote handling is the most delicate code in the plugin. 7. Documentation (11) — last, describing what actually shipped. ## Out of scope i18n keys in `GitCommitPerspective` and `GitPerspective` are namespaced under `GitGuiPlugin.*`. Confusing to maintain, but renaming them across 10 bundles is churn with no user-visible effect and would collide with everything above. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
