| Message ID | 20190425094600.15673-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add new command 'restore' | expand |
Hiya, On Thu, Apr 25, 2019 at 04:45:44PM +0700, Nguyễn Thái Ngọc Duy wrote: > v3 changes are really small > > - gitcli.txt is updated to mention the --staged/--worktree pair, in > comparison to --cached/--index which is also mention there. > > - The patch 'rm --staged' is dropped. It could come back. But if it > does, it should be in a series where commands that take > --cached/--index will now take both --staged/--worktree. Those are > 'rm', 'apply', 'check-attr', 'grep', 'diff' and maybe 'ls-files'. > > In other words we support --staged/--worktree everywhere while > --cached/--index still remains because of muscle memory. This is > of course only a good move if --staged/--worktree is much better > than --cached/--index. > > - Since there's a chance this series may end up in 'master' and get > released, and I'm still worried that I screwed up somewhere, the last > patch declares the two commands experimental for maybe one or two > release cycles. > > If the reception is good, we revert that patch. If something > horrible is found, we can still change the default behavior without > anybody yelling at us. Or worst case scenario, we remove both > commands and declare a failed experiment. > > PS. the intent-to-add support is still not in. But it should be in > before the experimental status is removed. I've got a usability comment, as we're giving restore a try within Google for now. I found myself in a situation where I had accidentally staged all my changes to tracked files (I think resulting from a stash pop which generated a merge conflict?) and didn't see a good way to unstage everything using restore. I tried out `git restore --staged *` and it tried to restore every build artifact in my working tree, all of which should be ignored, made a lot of noisy errors, and left me with my changes still staged. Then, figuring that I only had a few files, I thought I'd type them each, with the exception of a .c/.h pair: g restore --staged builtin/log.c builtin/walken.c revision.* Because I had build artifacts present, this vomited while trying to unstage revision.o, and again left me with all my changes still staged. revision.o is validly ignored: $ g check-ignore revision.o revision.o Finally explicitly naming each file which I needed to restore worked, but I have very little interest in doing this for more than a handful of files, especially since the output of `git status` is not easy to paste into the command line (due to the "modified:" etc marker). I was definitely still able to make it do what I wanted with `git reset HEAD` but thought you may be interested. It'd be cool to have a hint which advises how you can unstage everything, or else to pay attention to the gitignore and not try to unstage those files, or else to have a command to unstage everything. (I looked for one by trying `git restore --staged -a` but that doesn't exist :) ) Thanks! Emily
Emily Shaffer <emilyshaffer@google.com> writes: > Then, figuring that I only had a few files, I thought I'd type them > each, with the exception of a .c/.h pair: > > g restore --staged builtin/log.c builtin/walken.c revision.* > > Because I had build artifacts present, this vomited while trying to > unstage revision.o, and again left me with all my changes still staged. > revision.o is validly ignored: > > $ g check-ignore revision.o > revision.o Often a claim "validly ignored" by users needs to be taken with a grain of salt. When it is in the index, it is no longer ignored. Assuming that you do not have revision.o in the index, and if "restore" is still "checkout -- <pathspec>" in disguise, then I think you could ask "git" not your shell to expand the asterisk, by protecting it from the shell, i.e. $ git restore --staged revision.\* and the pathspec should not match untracked revision.o. And if that does not work, you found a bug in restore, I think ;-).
On Tue, May 7, 2019 at 9:21 AM Emily Shaffer <emilyshaffer@google.com> wrote: > I've got a usability comment, as we're giving restore a try within > Google for now. Thanks. I thought I was the only guinea pig :D and obviously not a good one since I know too much (which is not a good thing as a tester). > I found myself in a situation where I had accidentally > staged all my changes to tracked files (I think resulting from a stash > pop which generated a merge conflict?) and didn't see a good way to > unstage everything using restore. > > I tried out `git restore --staged *` and it tried to restore every build > artifact in my working tree, all of which should be ignored, made a lot of > noisy errors, and left me with my changes still staged. For the record, "git restore --staged :/" should do the trick and it is documented as an example (but without --staged). Either way. I think you raise a good point about "*" (or patterns matching more than expected in general). I need to sleep on it and see if the old way of handling pattern matching failure is still a good way to go. > Finally explicitly naming each file which I needed to restore worked, > but I have very little interest in doing this for more than a handful of > files, especially since the output of `git status` is not easy to paste > into the command line (due to the "modified:" etc marker). For just a couple files, you could also use "-p" to go through them and either "a" to restore all or "d" to skip the file. Maybe adding "-i/--interactive" to git-restore too, just like "git add". Hmm...
On Tue, May 07, 2019 at 05:36:56PM +0700, Duy Nguyen wrote: > On Tue, May 7, 2019 at 9:21 AM Emily Shaffer <emilyshaffer@google.com> wrote: > > I've got a usability comment, as we're giving restore a try within > > Google for now. > > Thanks. I thought I was the only guinea pig :D and obviously not a > good one since I know too much (which is not a good thing as a > tester). > > > I found myself in a situation where I had accidentally > > staged all my changes to tracked files (I think resulting from a stash > > pop which generated a merge conflict?) and didn't see a good way to > > unstage everything using restore. > > > > I tried out `git restore --staged *` and it tried to restore every build > > artifact in my working tree, all of which should be ignored, made a lot of > > noisy errors, and left me with my changes still staged. > > For the record, "git restore --staged :/" should do the trick and it > is documented as an example (but without --staged). Yeah, this worked, and today I also noted `git restore --staged .` works, as does Junio's suggestion on the other mail (`git restore --staged revision.\*`), and quoting the * (`git restore --staged '*'`). So maybe I didn't think outside the box enough before mailing :) > > Either way. I think you raise a good point about "*" (or patterns > matching more than expected in general). I need to sleep on it and see > if the old way of handling pattern matching failure is still a good > way to go. I think it's worth considering, especially as `git reset HEAD *` and `git reset HEAD` both work. (`git restore --staged` complains that it hasn't got any paths, but reset seems to figure you just mean everything.) It was a surprising failure to me coming away from years of using reset. > > > Finally explicitly naming each file which I needed to restore worked, > > but I have very little interest in doing this for more than a handful of > > files, especially since the output of `git status` is not easy to paste > > into the command line (due to the "modified:" etc marker). > > For just a couple files, you could also use "-p" to go through them > and either "a" to restore all or "d" to skip the file. Maybe adding > "-i/--interactive" to git-restore too, just like "git add". Hmm... > -- > Duyh Anyways, maybe it's not a particularly weighty complaint, since there are still ways to mass unstage, and if I had just RTFM I would have found them. :) It's a new workflow, after all, and the old one still works, so it's probably not reasonable to expect it all to just work the same way with s/reset HEAD/restore --staged/. Thanks! - Emily
On Wed, May 8, 2019 at 1:31 AM Emily Shaffer <emilyshaffer@google.com> wrote: > > > I found myself in a situation where I had accidentally > > > staged all my changes to tracked files (I think resulting from a stash > > > pop which generated a merge conflict?) and didn't see a good way to > > > unstage everything using restore. > > > > > > I tried out `git restore --staged *` and it tried to restore every build > > > artifact in my working tree, all of which should be ignored, made a lot of > > > noisy errors, and left me with my changes still staged. > > > > For the record, "git restore --staged :/" should do the trick and it > > is documented as an example (but without --staged). > > Yeah, this worked, and today I also noted `git restore --staged .` > works, as does Junio's suggestion on the other mail (`git restore > --staged revision.\*`), and quoting the * (`git restore --staged '*'`). > So maybe I didn't think outside the box enough before mailing :) No it's good. It actually got me thinking. Actually I take that back. It's bad, I'm quite stuck at thinking here :D > > Either way. I think you raise a good point about "*" (or patterns > > matching more than expected in general). I need to sleep on it and see > > if the old way of handling pattern matching failure is still a good > > way to go. > > I think it's worth considering, especially as `git reset HEAD *` and > `git reset HEAD` both work. (`git restore --staged` complains that it > hasn't got any paths, but reset seems to figure you just mean > everything.) It was a surprising failure to me coming away from years of > using reset. Yeah I agree that doing it the "git reset" way makes sense for --staged because shell expansion is tied to worktree files, and --staged is completely disconnected from worktree. Shell expansion cannot get this case right, no point punishing the user because of it. My problem is "what about the other two cases, --worktree alone or --worktree with --staged (iow do we care about consistency)? what about git-checkout? does the old way even make sense for git-checkout? any better way to do if the reason behind is still valid?". I obviously haven't worked it all out yet.
v3 changes are really small - gitcli.txt is updated to mention the --staged/--worktree pair, in comparison to --cached/--index which is also mention there. - The patch 'rm --staged' is dropped. It could come back. But if it does, it should be in a series where commands that take --cached/--index will now take both --staged/--worktree. Those are 'rm', 'apply', 'check-attr', 'grep', 'diff' and maybe 'ls-files'. In other words we support --staged/--worktree everywhere while --cached/--index still remains because of muscle memory. This is of course only a good move if --staged/--worktree is much better than --cached/--index. - Since there's a chance this series may end up in 'master' and get released, and I'm still worried that I screwed up somewhere, the last patch declares the two commands experimental for maybe one or two release cycles. If the reception is good, we revert that patch. If something horrible is found, we can still change the default behavior without anybody yelling at us. Or worst case scenario, we remove both commands and declare a failed experiment. PS. the intent-to-add support is still not in. But it should be in before the experimental status is removed. Nguyễn Thái Ngọc Duy (16): checkout: split part of it to new command 'restore' restore: take tree-ish from --source option instead restore: make pathspec mandatory restore: disable overlay mode by default checkout: factor out worktree checkout code restore: add --worktree and --staged restore: reject invalid combinations with --staged restore: default to --source=HEAD when only --staged is specified restore: replace --force with --ignore-unmerged restore: support --patch t: add tests for restore completion: support restore user-manual.txt: prefer 'merge --abort' over 'reset --hard' doc: promote "git restore" help: move git-diff and git-reset to different groups Declare both git-switch and git-restore experimental .gitignore | 1 + Documentation/config/interactive.txt | 3 +- Documentation/git-checkout.txt | 3 +- Documentation/git-clean.txt | 2 +- Documentation/git-commit.txt | 2 +- Documentation/git-format-patch.txt | 2 +- Documentation/git-reset.txt | 13 +- Documentation/git-restore.txt (new) | 185 +++++++++++++++ Documentation/git-revert.txt | 7 +- Documentation/git-switch.txt | 2 + Documentation/git.txt | 20 ++ Documentation/gitcli.txt | 16 +- Documentation/giteveryday.txt | 5 +- Documentation/gittutorial-2.txt | 4 +- Documentation/gittutorial.txt | 2 +- Documentation/user-manual.txt | 14 +- Makefile | 1 + builtin.h | 1 + builtin/checkout.c | 299 +++++++++++++++++++------ builtin/clone.c | 2 +- builtin/commit.c | 2 +- command-list.txt | 7 +- contrib/completion/git-completion.bash | 15 ++ git-add--interactive.perl | 52 +++++ git.c | 1 + t/lib-patch-mode.sh | 12 + t/t2070-restore.sh (new +x) | 99 ++++++++ t/t2071-restore-patch.sh (new +x) | 110 +++++++++ t/t7508-status.sh | 82 +++---- t/t7512-status-help.sh | 20 +- wt-status.c | 26 ++- 31 files changed, 850 insertions(+), 160 deletions(-) create mode 100644 Documentation/git-restore.txt create mode 100755 t/t2070-restore.sh create mode 100755 t/t2071-restore-patch.sh Range-diff dựa trên v2: 1: 41788fc2d2 ! 1: 0d5ea2b7fe checkout: split part of it to new command 'restore' @@ -342,6 +342,29 @@ Low-level commands (plumbing) ----------------------------- + diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt + --- a/Documentation/gitcli.txt + +++ b/Documentation/gitcli.txt +@@ + http://marc.info/?l=git&m=119150393620273 for further + information. + ++Some other commands that also work on files in the working tree and/or ++in the index can take `--staged` and/or `--worktree`. ++ ++* `--staged` is exactly like `--cached`, which is used to ask a ++ command to only work on the index, not the working tree. ++ ++* `--worktree` is the opposite, to ask a command to work on the ++ working tree only, not the index. ++ ++* The two options can be specified together to ask a command to work ++ on both the index and the working tree. ++ + GIT + --- + Part of the linkgit:git[1] suite + diff --git a/Makefile b/Makefile --- a/Makefile +++ b/Makefile 2: 253dfb42ae = 2: c3c6e7762a restore: take tree-ish from --source option instead 3: 0266c4d982 = 3: 0fbf963e7d restore: make pathspec mandatory 4: ae4dd4c24e = 4: 2509bebf32 restore: disable overlay mode by default 5: c51dd232c0 = 5: 206c507f7d checkout: factor out worktree checkout code 6: 6b19ddb1b3 = 6: 656bfcd659 restore: add --worktree and --staged 7: 3f1031cc23 = 7: f2d3aa1027 restore: reject invalid combinations with --staged 8: d6d9bed95c = 8: 57efad405d restore: default to --source=HEAD when only --staged is specified 9: fca91f3cca = 9: 3c6b32c223 restore: replace --force with --ignore-unmerged 10: 079273e2df = 10: 6665d7523b restore: support --patch 11: 9d28d167fa = 11: d7bda0c0cc t: add tests for restore 12: acd490f8b0 = 12: e4622aff3d completion: support restore 13: 336a7d8921 = 13: 787b0e485e user-manual.txt: prefer 'merge --abort' over 'reset --hard' 14: 3b81b27255 = 14: 0df020c2c8 doc: promote "git restore" 15: ce7e890524 < -: ---------- rm: add --staged as alias for --cached 16: 763aa1d6f1 = 15: 3302b1b0e1 help: move git-diff and git-reset to different groups -: ---------- > 16: ffeea858a7 Declare both git-switch and git-restore experimental