Matthieu Moy, 2007-07-09: > First of all, I don't see why you see the possibility to commit from a > wrong diff buffer as a real problem. OK, it doesn't really make sense, > but does it harm? We've been developing DVC, and its predecessor Xtla > for 3 or 4 years now, and we never got a complain about this > possibility. Has it ever been a problem for you?
Yes. I once accidentally committed the wrong set of files because DVC allowed me to selectively commit files even though the diff shown was unrelated to what was going to be committed. This was confusing. > I don't mean a safety > check would not be interesting, but it really doesn't seem to be a > priority to me. It isn't very urgent to me either, but a clean design that makes sense should be a medium-term goal. DVC's operations should be meaningful. It should be impossible to select files to commit from a diff that is known to be unrelated to the workspace. I'm not the only person to have noticed the possibility of confusion here: http://www.mail-archive.com/[EMAIL PROTECTED]/msg07706.html (Bruce Stephens, 2007-04-12). > Now, suppose I want to view the diff between two revisions, A and B. > With your proposal, I can view the diff, but I have to forget about > the summary at the top, What do you mean by "forget"? As I said before, the diff summary is not a problem. Under my proposal, it would stay. Only the possibility to select files and to go to the commit log buffer (and trigger other workspace operations) would be removed. >> We could also restructure the command definitions to have shared code >> handle the check of this variable, but that would be a nontrivial >> refactoring. > > That *would* be a trivial refactoring. Adding a wrapper function for > the unified commands would take ~5 lines of code for each of the > functions. > > (defun dvc-safe-whatever (...) > (if (dvc-commit-is-possible) > (dvc-whatever ...) > (error "Command dissallowed here"))) And then you need to look at every call site of or reference to dvc-whatever and figure out whether it should call the "safe" variant or not. Perhaps this is trivial. My main objection to this is that it reduces maintainability by adding yet another obscure convention to the code, namely the distinction between "safe" and not-"safe" commands. >>> BTW, commiting from a diff buffer which isn't against the head can be >>> meaningfull too. For example, "git commit --amend" commits against >>> the ancestor of the head, and discards the current head (I don't >>> think we have a good interface for this in DVC). >> Whether the user wants --amend is unrelated to which diff he's looking >> at. It's probably relatively common to look at the diff relative to the >> HEAD while still wanting --amend. > > In the best case, you'd look at both: HEAD to check that you are > actually commiting a fix for the previous commit, and against its > ancestor (HEAD^ in the git syntax) to check that the combination of > both makes sense. It depends on the use case. I also sometimes re-work > a patch, and don't want to record the intermediate steps. Then, I only > look at the diff against HEAD^. > > And commiting from a diff against the last but one revision is indeed > a common use-case: I often used it (that's when I wrote > tla-changes-last-revision) to fix or comment other people's work after > merging it. What do you propose for this senario with back-ends not > supporting "status" against the ancestor of HEAD (bzr for example)? Look at the diff against the last but one revision, and use dvc-commit. The problem is not with looking at diffs and committing, but only with selecting files. It makes no sense to select files from the diff you're referring to, because the set of changed files in that diff is (in general and even in this case) unrelated to the set of files that you can meaningfully commit (i.e. files that have changed in the workspace relative to its base revision). This use case doesn't seem to add any requirements for any of the features involved. > I'm repeating myself, but you're trying to add a safety check for > something which is a non-problem, and this would remove many > interesting possibilities from DVC. It is a problem. It is a problem that I have encountered. What I'm talking about is not just a safety check, it's clean design of the interaction of DVC's features. Selecting files to commit from any historic diff should never have been possible in the first place. The current implementation was made without any consideration of this interaction. And we should fix that. Let's look at another alternative. We can define three different modes: * `dvc-diff-mode' for historic diffs. Shows the diff summary, but does not allow file selection or workspace operations. A read-only view, because a read-write view doesn't make sense when looking at history. * `dvc-status-mode' for status buffers. Shows the status and allows workspace operations. Keybindings that make sense only for diff buffers are not available here. * `dvc-review-changes-mode' (or `dvc-status-with-diff-mode' or something) for the interface that you seem to prefer for commits: diff summary (or status?) at the top, diff at the bottom. This mode allows workspace operations. The difference between this and `dvc-diff-mode' is that it's impossible to invoke this mode on historic diffs; this mode always shows the current diff and the status of the workspace. Thus, it is possible to meaningfully select files to revert/commit/rename/etc. With this design, the set of operations available in each mode is well-defined, and it supports your desired workflow. (In effect, the distinction between `dvc-diff-mode' and `dvc-review-changes-mode' would be similar to the buffer-local "workspace operations allowed" variable you suggested. Creating separate modes obviates the need for setting and checking such a variable, though, and simplifies the code by providing a clearer separation between different modes with different purposes: One is a history view, the other is a commit interface.) Christian. _______________________________________________ Dvc-dev mailing list [email protected] https://mail.gna.org/listinfo/dvc-dev
