From: Duy Nguyen <pclouds@gmail.com>
To: Kevin Willford <kewillf@microsoft.com>
Cc: Kevin Willford <kcwillford@gmail.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
"gitster@pobox.com" <gitster@pobox.com>,
"peff@peff.net" <peff@peff.net>
Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.
Date: Sun, 16 Apr 2017 11:25:30 +0700 [thread overview]
Message-ID: <CACsJy8CbxGPeWHgR8s-ix_wxPODvisw6eaLnEUV4LDmoeXt5BQ@mail.gmail.com> (raw)
In-Reply-To: <SN2PR03MB2333B9A0A7A3888F0402594DB7030@SN2PR03MB2333.namprd03.prod.outlook.com>
On Wed, Apr 12, 2017 at 10:37 PM, Kevin Willford <kewillf@microsoft.com> wrote:
>
>> -----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.
I know. But in my view, it should. All updates from a tree object to
the index should happen through unpack_trees().
>> > 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.
I think it's ok to just look at the diff (from update_index_from_diff)
and restore the on-disk version for now. I'd like to make --mixed use
unpack_trees() too but I haven't studied this code long enough to see
why it went with "diff" instead of "read-tree" (which translates
directly to unpack_trees). Maybe there is some subtle reason for that.
Though it looks like it was more convenient to do "diff" in the
git-reset.sh version, and that got translated literally to C when the
command was rewritten.
--
Duy
next prev parent reply other threads:[~2017-04-16 4:26 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-07 19:23 [PATCH 0/3] fix working directory file issues while using sparse-checkout Kevin Willford
2017-04-07 19:23 ` [PATCH 1/3] merge-recursive.c: conflict using sparse should update file Kevin Willford
2017-04-10 10:36 ` Duy Nguyen
2017-04-07 19:23 ` [PATCH 2/3] apply.c: do not checkout file when skip-worktree bit set Kevin Willford
2017-04-07 22:28 ` Stefan Beller
2017-04-10 10:11 ` Duy Nguyen
2017-04-07 19:23 ` [PATCH 3/3] reset.c: update files when using sparse to avoid data loss Kevin Willford
2017-04-07 22:41 ` Stefan Beller
2017-04-10 10:24 ` Duy Nguyen
2017-04-11 22:30 ` Kevin Willford
2017-04-12 13:21 ` Duy Nguyen
2017-04-12 15:37 ` Kevin Willford
2017-04-16 4:25 ` Duy Nguyen [this message]
2017-04-17 12:09 ` Duy Nguyen
2017-04-07 19:27 ` [PATCH 0/3] fix working directory file issues while using sparse-checkout Stefan Beller
2017-04-07 19:27 ` Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACsJy8CbxGPeWHgR8s-ix_wxPODvisw6eaLnEUV4LDmoeXt5BQ@mail.gmail.com \
--to=pclouds@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=kcwillford@gmail.com \
--cc=kewillf@microsoft.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.