Re: [PATCH 0/2] read-tree: improve untracked file support

From: Phillip Wood <phillip.wood123@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 0/2] read-tree: improve untracked file support
Date: Tue, 7 May 2019 11:01:59 +0100	[thread overview]
Message-ID: <994d4687-39ed-91b9-cdf9-d4be39cbb2ba@gmail.com> (raw)
In-Reply-To: <CACsJy8AAj8cRAc+PnS4OJy8M=K7FjCHUsL6Cx-6+RTAbTA_1fQ@mail.gmail.com>

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
>>
> 
> 