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

From: Stefan Beller <sbeller@google.com>
Date: Apr 7, 2017, 22:41 UTC
Message-ID: <CAGZ79kYJvLQxTcL2-hLS+MOoWQoshxpeLzNoNXbH6zQqGbbCdw@mail.gmail.com>
In-Reply-To: <20170407192357.948-4-kewillf@microsoft.com>

On Fri, Apr 7, 2017 at 12:23 PM, Kevin Willford <kcwillford@gmail.com> wrote:

Show 39 quoted lines

> When using the sparse checkout feature the git reset command will add
> entries to the index that will have the skip-worktree bit off but will
> leave the working directory empty.  File data is lost because the index
> version of the files has been changed but there is nothing that is in the
> working directory.  This will cause the next status call to show either
> deleted for files modified or deleting or nothing for files added.
> The added files should be shown as untracked and modified files should
> be shown as modified.
>
> To fix this when the reset is running if there is not a file in the
> working directory and if it will be missing with the new index entry
> or was not missing in the previous version, we create the previous index
> version of the file in the working directory so that status will report
> correctly and the files will be availble for the user to deal with.
>
> Signed-off-by: Kevin Willford <kewillf@microsoft.com>
> ---
>  builtin/reset.c                  | 34 +++++++++++++++++++++++
>  t/t7114-reset-sparse-checkout.sh | 58 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
>  create mode 100755 t/t7114-reset-sparse-checkout.sh
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 8ab915bfcb..8ba97999f4 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -21,6 +21,7 @@
>  #include "parse-options.h"
>  #include "unpack-trees.h"
>  #include "cache-tree.h"
> +#include "dir.h"
>
>  static const char * const git_reset_usage[] = {
>         N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
> @@ -117,12 +118,45 @@ static void update_index_from_diff(struct diff_queue_struct *q,
>                 struct diff_options *opt, void *data)
>  {
>         int i;
> +       int pos;

You could declare this inside the for loop.

Show 21 quoted lines

>         int intent_to_add = *(int *)data;
>
>         for (i = 0; i < q->nr; i++) {
>                 struct diff_filespec *one = q->queue[i]->one;
> +               struct diff_filespec *two = q->queue[i]->two;
>                 int is_missing = !(one->mode && !is_null_oid(&one->oid));
> +               int was_missing = !two->mode && is_null_oid(&two->oid);
>                 struct cache_entry *ce;
> +               struct cache_entry *ceBefore;
> +               struct checkout state = CHECKOUT_INIT;
> +
> +               /*
> +                * When using the sparse-checkout feature the cache entries that are
> +                * added here will not have the skip-worktree bit set.
> +                * Without this code there is data that is lost because the files that
> +                * would normally be in the working directory are not there and show as
> +                * deleted for the next status or in the case of added files just disappear.
> +                * We need to create the previous version of the files in the working
> +                * directory so that they will have the right content and the next
> +                * status call will show modified or untracked files correctly.
> +                */

This comment also belongs to the commit message IMHO, as it describes the bug on a high level, and when it is in the code it wastes screen real estate; commit messages however have low prices for screen real estate. ;-)

> +               if (core_apply_sparse_checkout && !file_exists(two->path))
> +               {

Coding style: Git prefers { at the end of the line:

  if (..) {
    ..

Show 5 quoted lines

> +test_expect_success 'reset after deleting file without skip-worktree bit' '
> +       git checkout -f endCommit &&
> +       git clean -xdf &&
> +       echo "/c
> +/m" >.git/info/sparse-checkout &&

huh? Did your mailer wrap lines here or do you intend to have a LF in the output?

Assuming the latter, I think we prefer cat with here-doc for multi line content, i.e.

    cat >.git/info/sparse-checkout <<-\EOF
    /c
    /m
    EOF
    test_config ...
    ...

Thanks, Stefan