| Message ID | 20190501101403.20294-1-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | read-tree: improve untracked file support | expand |
On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > These two patches teach read-tree how to avoid overwriting untracked > files when doing '--reset -u' and also how to respect all of git's > standard excludes files. I'd like to see the porcelain commands stop > overwriting untracked files, this is a first step on the way. I'm not > sure if we want to add options to the porcelain commands to protect > untracked files or just change their behavior and add an option to > override that. I'm leaning towards the latter but I'd be interested to > hear what others think. For new commands like git-restore, it's definitely a good thing to not overwrite untracked files. For existing commands I guess we have to go over them one by one. For "git reset --hard", it should really just overwrite whatever needed to get back to the known good state. "git checkout -f" , not so sure (seems weird that we need force-level-two option to override the protection provided by -f, if we change default behavior)
On 01/05/2019 11:31, Duy Nguyen wrote: > On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> These two patches teach read-tree how to avoid overwriting untracked >> files when doing '--reset -u' and also how to respect all of git's >> standard excludes files. I'd like to see the porcelain commands stop >> overwriting untracked files, this is a first step on the way. I'm not >> sure if we want to add options to the porcelain commands to protect >> untracked files or just change their behavior and add an option to >> override that. I'm leaning towards the latter but I'd be interested to >> hear what others think. > > For new commands like git-restore, it's definitely a good thing to not > overwrite untracked files. I agree, unfortunately this series does not help with git-restore, only git-switch. For restore on an index without conflicts I think it could just use the pathspec in struct unpack_trees_options and set opts.rest = UNPACK_RESET_PROTECT_UNTRACKED but that does not help if we want to handle conflicted paths differently to non-conflicted paths. > For existing commands I guess we have to go > over them one by one. For "git reset --hard", it should really just > overwrite whatever needed to get back to the known good state. "git > checkout -f" , not so sure (seems weird that we need force-level-two > option to override the protection provided by -f, if we change default > behavior) I think it's fine for "checkout -f" to overwrite untracked files (and if "switch --discard-changes" does not then there is no pressing need to add such a mode to checkout), --force is a good name for an option that nukes everything that gets in it's way. For "reset --hard" I'm not so sure, if I have changes to an untracked file I don't wont them overwritten by default. There is no porcelain equivalent to "read-tree --reset --protect-untracked -u" and I was hoping "reset --hard" may become that porcelain equivalent with a new --force or --overwrite-untracked option. For the various "foo --abort" some (most?) are using "reset --merge" which I think declines to overwrite untracked files but rebase uses "reset --hard" which I'd like to change to protect untracked files in the same way that rebase does for the initial checkout and when picking commits. I haven't thought about stash. Best Wishes Phillip
On Wed, May 1, 2019 at 9:58 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > > > On 01/05/2019 11:31, Duy Nguyen wrote: > > On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> > >> > >> These two patches teach read-tree how to avoid overwriting untracked > >> files when doing '--reset -u' and also how to respect all of git's > >> standard excludes files. I'd like to see the porcelain commands stop > >> overwriting untracked files, this is a first step on the way. I'm not > >> sure if we want to add options to the porcelain commands to protect > >> untracked files or just change their behavior and add an option to > >> override that. I'm leaning towards the latter but I'd be interested to > >> hear what others think. > > > > For new commands like git-restore, it's definitely a good thing to not > > overwrite untracked files. > > I agree, unfortunately this series does not help with git-restore, only > git-switch. For restore on an index without conflicts I think it could > just use the pathspec in struct unpack_trees_options and set opts.rest = > UNPACK_RESET_PROTECT_UNTRACKED but that does not help if we want to > handle conflicted paths differently to non-conflicted paths. Right. I got confused. You did mention "git checkout <rev> :/" in 1/2, which is the same as "git restore --source <rev> --staged --worktree :/" and can also potentially overwrite untracked files, even though it does not use unpack-trees and cannot be fixed with this. Never mind. Let's leave git-restore out of the discussion for now. > > For existing commands I guess we have to go > > over them one by one. For "git reset --hard", it should really just > > overwrite whatever needed to get back to the known good state. "git > > checkout -f" , not so sure (seems weird that we need force-level-two > > option to override the protection provided by -f, if we change default > > behavior) > > I think it's fine for "checkout -f" to overwrite untracked files (and if > "switch --discard-changes" does not then there is no pressing need to > add such a mode to checkout), --force is a good name for an option that > nukes everything that gets in it's way. For "reset --hard" I'm not so > sure, if I have changes to an untracked file I don't wont them > overwritten by default. There is no porcelain equivalent to "read-tree > --reset --protect-untracked -u" and I was hoping "reset --hard" may > become that porcelain equivalent with a new --force or > --overwrite-untracked option. My (biased, obviously) view is that "git reset --hard" is very dangerous and I'm not trying to change that, especially when its behavior has been like this since forever and I'm sure it's used in scripts. Instead "git restore" should be used when you need "git reset --hard HEAD", the most often use case. And since it's new, changing default behavior is not a problem. Which brings us back to git-restore :) But either way, git-restore or git-reset, I still don't see why untracked files are more valuable in this case than tracked ones to change the default. I can see that sometimes you may want to restore just tracked files, or untracked files, almost like filtering with pathspec. > For the various "foo --abort" some (most?) are using "reset --merge" > which I think declines to overwrite untracked files but rebase uses > "reset --hard" which I'd like to change to protect untracked files in > the same way that rebase does for the initial checkout and when picking > commits. I haven't thought about stash. Yeah it looks like cherry-pick and revert use "reset --merge" too (reset_for_rollback function). That's all of them. Probably a stupid question, why can't rebase just use "rebase --merge" like everybody else? > > Best Wishes > > Phillip >
On 02/05/2019 11:53, Duy Nguyen wrote: > On Wed, May 1, 2019 at 9:58 PM Phillip Wood <phillip.wood123@gmail.com> wrote: >> On 01/05/2019 11:31, Duy Nguyen wrote: >>> On Wed, May 1, 2019 at 5:14 PM Phillip Wood <phillip.wood123@gmail.com> wrote: >>>> >>>> From: Phillip Wood <phillip.wood@dunelm.org.uk> >>>> >>>> These two patches teach read-tree how to avoid overwriting untracked >>>> files when doing '--reset -u' and also how to respect all of git's >>>> standard excludes files. I'd like to see the porcelain commands stop >>>> overwriting untracked files, this is a first step on the way. I'm not >>>> sure if we want to add options to the porcelain commands to protect >>>> untracked files or just change their behavior and add an option to >>>> override that. I'm leaning towards the latter but I'd be interested to >>>> hear what others think. >>> >>> For new commands like git-restore, it's definitely a good thing to not >>> overwrite untracked files. >> >> I agree, unfortunately this series does not help with git-restore, only >> git-switch. For restore on an index without conflicts I think it could >> just use the pathspec in struct unpack_trees_options and set opts.rest = >> UNPACK_RESET_PROTECT_UNTRACKED but that does not help if we want to >> handle conflicted paths differently to non-conflicted paths. > > Right. I got confused. You did mention "git checkout <rev> :/" in 1/2, > which is the same as "git restore --source <rev> --staged --worktree > :/" and can also potentially overwrite untracked files, even though > it does not use unpack-trees and cannot be fixed with this. Never > mind. Let's leave git-restore out of the discussion for now. > >>> For existing commands I guess we have to go >>> over them one by one. For "git reset --hard", it should really just >>> overwrite whatever needed to get back to the known good state. "git >>> checkout -f" , not so sure (seems weird that we need force-level-two >>> option to override the protection provided by -f, if we change default >>> behavior) >> >> I think it's fine for "checkout -f" to overwrite untracked files (and if >> "switch --discard-changes" does not then there is no pressing need to >> add such a mode to checkout), --force is a good name for an option that >> nukes everything that gets in it's way. For "reset --hard" I'm not so >> sure, if I have changes to an untracked file I don't wont them >> overwritten by default. There is no porcelain equivalent to "read-tree >> --reset --protect-untracked -u" and I was hoping "reset --hard" may >> become that porcelain equivalent with a new --force or >> --overwrite-untracked option. > > My (biased, obviously) view is that "git reset --hard" is very > dangerous and I'm not trying to change that, especially when its > behavior has been like this since forever and I'm sure it's used in > scripts. > > Instead "git restore" should be used when you need "git reset --hard > HEAD", the most often use case. And since it's new, changing default > behavior is not a problem. Which brings us back to git-restore :) Does restore clean up the branch state like reset? It's tricky because you only want to do that if there is no pathspec (or the pathspec is :/ or equivalent - I can't remember if restore always requires paths or not) > But either way, git-restore or git-reset, I still don't see why > untracked files are more valuable in this case than tracked ones to > change the default. My issue is that is easy to see what changes you're going to lose in tracked files by running diff. For untracked files diff just says a new file will be created, it ignores the current contents as the path is in the index so it is easy to overwrite changes without realizing. There's also a philosophical point that git should not be stomping on paths that it is not tracking though that's a bit moot if a path is tracked in one revision but not another. > I can see that sometimes you may want to restore > just tracked files, or untracked files, almost like filtering with > pathspec. > >> For the various "foo --abort" some (most?) are using "reset --merge" >> which I think declines to overwrite untracked files but rebase uses >> "reset --hard" which I'd like to change to protect untracked files in >> the same way that rebase does for the initial checkout and when picking >> commits. I haven't thought about stash. > > Yeah it looks like cherry-pick and revert use "reset --merge" too > (reset_for_rollback function). That's all of them. Probably a stupid > question, why can't rebase just use "rebase --merge" like everybody > else? I'm not sure - if --merge works for the others I can't see why it shouldn't work for rebase as well. Best Wishes Phillip >> Best Wishes >> >> Phillip >> > >
On Tue, May 7, 2019 at 5:02 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > My (biased, obviously) view is that "git reset --hard" is very > > dangerous and I'm not trying to change that, especially when its > > behavior has been like this since forever and I'm sure it's used in > > scripts. > > > > Instead "git restore" should be used when you need "git reset --hard > > HEAD", the most often use case. And since it's new, changing default > > behavior is not a problem. Which brings us back to git-restore :) > > Does restore clean up the branch state like reset? It's tricky because > you only want to do that if there is no pathspec (or the pathspec is :/ > or equivalent - I can't remember if restore always requires paths or not) Nope. git-restore cares about files, not branches. Yes git-restore always requires paths, just in case people type "git restore" and expect to see help usage or something. > > But either way, git-restore or git-reset, I still don't see why > > untracked files are more valuable in this case than tracked ones to > > change the default. > > My issue is that is easy to see what changes you're going to lose in > tracked files by running diff. For untracked files diff just says a new > file will be created, it ignores the current contents as the path is in > the index so it is easy to overwrite changes without realizing. There's > also a philosophical point that git should not be stomping on paths that > it is not tracking though that's a bit moot if a path is tracked in one > revision but not another. Ah good point about diff. If only we had "git reset --dry-run" (that shows the diff, including untracked files; or perhaps --diff would be a better name for that imaginary option)
From: Phillip Wood <phillip.wood@dunelm.org.uk> These two patches teach read-tree how to avoid overwriting untracked files when doing '--reset -u' and also how to respect all of git's standard excludes files. I'd like to see the porcelain commands stop overwriting untracked files, this is a first step on the way. I'm not sure if we want to add options to the porcelain commands to protect untracked files or just change their behavior and add an option to override that. I'm leaning towards the latter but I'd be interested to hear what others think. Phillip Wood (2): read-tree --reset: add --protect-untracked read-tree: add --exclude-standard Documentation/git-read-tree.txt | 19 ++++++++-- builtin/am.c | 8 +++-- builtin/checkout.c | 2 +- builtin/read-tree.c | 61 +++++++++++++++++++++++++++++-- builtin/rebase.c | 2 +- builtin/reset.c | 2 +- builtin/stash.c | 7 ++-- t/lib-read-tree.sh | 11 ++++++ t/t1005-read-tree-reset.sh | 63 +++++++++++++++++++++++++++++++-- t/t1013-read-tree-submodule.sh | 3 +- unpack-trees.c | 3 +- unpack-trees.h | 10 ++++-- 12 files changed, 170 insertions(+), 21 deletions(-)