RE: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

From: Kevin Willford <kewillf@microsoft.com>
Date: Apr 12, 2017, 15:37 UTC
Message-ID: <SN2PR03MB2333B9A0A7A3888F0402594DB7030@SN2PR03MB2333.namprd03.prod.outlook.com>
In-Reply-To: <CACsJy8Dw8qfS6G71y3FoT9w7H=FABMx=5R=v8JcPCEHtv1vLcw@mail.gmail.com>

Show 26 quoted lines

> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Duy Nguyen
> Sent: Wednesday, April 12, 2017 7:21 AM
> To: Kevin Willford <kewillf@microsoft.com>
> Cc: Kevin Willford <kcwillford@gmail.com>; git@vger.kernel.org;
> gitster@pobox.com; peff@peff.net
> Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid
> data loss.
> 
> On Wed, Apr 12, 2017 at 5:30 AM, Kevin Willford <kewillf@microsoft.com>
> wrote:
> > The loss of the skip-worktree bits is part of the problem if you are
> > talking about modified files.  The other issue that I was having is
> > when running a reset and there were files added in the commit that is
> > being reset, there will not be an entry in the index and not a file on
> > disk so the data for that file is completely lost at that point.
> > "status" also doesn't include anything about this loss of data.  On
> > modified files status will at least have the file as deleted since
> > there is still an index entry but again the previous version of the file and it's
> data is lost.
> 
> Well, we could have "deleted" index entries, those marked with
> CE_REMOVE. They are never written down to file though, so 'status'
> won't benefit from that. Hopefully we can restore the file before the index
> file is written down and we really lose skip-worktree bits?

Because this is a reset --mixed it will never run through unpack_trees and The entries are never marked with CE_REMOVE.

Show 17 quoted lines

> 
> > To me this is totally unexpected behavior, for example if I am on a
> > commit where there are only files that where added and run a reset
> > HEAD~1 and then a status, it will show a clean working directory.
> > Regardless of skip-worktree bits the user needs to have the data in
> > the working directory after the reset or data is lost which is always bad.
> 
> I agree we no longer have a place to save the skip-worktree bit, we have to
> restore the state back as if skip-worktree bit does not exist.
> It would be good if we could keep the logic in unpack_trees() though.
> For example, if the file is present on disk even if skip-worktree bit is on,
> unpack_trees() would abort instead of silently overwriting it.
> This is a difference between skip-worktree and assume-unchanged bits.
> If you do explicit checkout_entry() you might have to add more checks to
> keep behavior consistent.
> --
> Duy

Because this is a reset --mixed it will follow the code path calling read_from_tree and ends up calling update_index_from_diff in the format_callback of the diff, so unpack_trees() is never called in the --mixed case. This code change also only applies when the file does not exist and the skip-worktree bit is on and the updated index entry either will be missing (covers the added scenario) or was not missing before (covers the modified scenario). If there is a better way to get the previous index entry to disk than what I am doing, I am happy to implement it correctly.

Thanks, Kevin