Re: [PATCH 5/8] checkout: introduce --{,no-}overlay option

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Dec 11, 2018, 22:42 UTC
Message-ID: <20181211224239.GV4883@hank.intra.tgummerer.com>
In-Reply-To: <CACsJy8BWZyFWZ70y4beCEYvjbc2+X-2K+gOiY+=toK0y3xYzKw@mail.gmail.com>

On 12/10, Duy Nguyen wrote:

Show 37 quoted lines

> On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > @@ -302,15 +310,29 @@ static int checkout_paths(const struct checkout_opts *opts,
> >                 ce->ce_flags &= ~CE_MATCHED;
> >                 if (!opts->ignore_skipworktree && ce_skip_worktree(ce))
> >                         continue;
> > -               if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
> > -                       /*
> > -                        * "git checkout tree-ish -- path", but this entry
> > -                        * is in the original index; it will not be checked
> > -                        * out to the working tree and it does not matter
> > -                        * if pathspec matched this entry.  We will not do
> > -                        * anything to this entry at all.
> > -                        */
> > -                       continue;
> > +               if (opts->source_tree && !(ce->ce_flags & CE_UPDATE)) {
> > +                       if (!opts->overlay_mode &&
> > +                           ce_path_match(&the_index, ce, &opts->pathspec, ps_matched)) {
> > +                               /*
> > +                                * "git checkout --no-overlay <tree-ish> -- path",
> > +                                * and the path is not in tree-ish, but is in
> > +                                * the current index, which means that it should
> > +                                * be removed.
> > +                                */
> > +                               ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
> > +                               continue;
> > +                       } else {
> 
> In non-overlay mode but when pathspec does not match, we come here too.
> 
> > +                               /*
> > +                                * "git checkout tree-ish -- path", but this
> > +                                * entry is in the original index; it will not
> 
> I think the missing key point in this comment block is "..is in the
> original index _and it's not in tree-ish_". In non-overlay mode, if
> pathspec does not match then it's safe to ignore too. But this logic
> starts too get to complex and hurt my brain.

Yes, that would make it a bit easier to read. I took a while to try and refactor this to make it easier to read, but couldn't come up with anything much better unfortunately. I'll have another stab at simplifying the logic a bit for v2.

Show 19 quoted lines

> > +                                * be checked out to the working tree and it
> > +                                * does not matter if pathspec matched this
> > +                                * entry.  We will not do anything to this entry
> > +                                * at all.
> > +                                */
> > +                               continue;
> > +                       }
> > +               }
> >                 /*
> >                  * Either this entry came from the tree-ish we are
> >                  * checking the paths out of, or we are checking out
> 
> > @@ -1266,6 +1299,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> >                             "checkout", "control recursive updating of submodules",
> >                             PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
> >                 OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
> > +               OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
> 
> maybe add " (default)" to the help string.

Makes sense, will add.

Show 5 quoted lines

> >                 OPT_END(),
> >         };
> >
> -- 
> Duy