[3/8] entry: support CE_WT_REMOVE flag in checkout_entry
diff mbox series

Message ID 20181209200449.16342-4-t.gummerer@gmail.com
State New
Headers show
Series
  • introduce no-overlay and cached mode in git checkout
Related show

Commit Message

Thomas Gummerer Dec. 9, 2018, 8:04 p.m. UTC
'checkout_entry()' currently only supports creating new entries in the
working tree, but not deleting them.  Add the ability to remove
entries at the same time if the entry is marked with the CE_WT_REMOVE
flag.

Currently this doesn't have any effect, as the CE_WT_REMOVE flag is
only used in unpack-tree, however we will make use of this in a
subsequent step in the series.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 entry.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Duy Nguyen Dec. 10, 2018, 3:58 p.m. UTC | #1
On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> 'checkout_entry()' currently only supports creating new entries in the
> working tree, but not deleting them.  Add the ability to remove
> entries at the same time if the entry is marked with the CE_WT_REMOVE
> flag.
>
> Currently this doesn't have any effect, as the CE_WT_REMOVE flag is
> only used in unpack-tree, however we will make use of this in a
> subsequent step in the series.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  entry.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/entry.c b/entry.c
> index 3ec148ceee..cd1c6601b6 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -441,6 +441,13 @@ int checkout_entry(struct cache_entry *ce,
>         static struct strbuf path = STRBUF_INIT;
>         struct stat st;
>
> +       if (ce->ce_flags & CE_WT_REMOVE) {
> +               if (topath)
> +                       BUG("Can't remove entry to a path");
> +               unlink_entry(ce);
> +               return 0;
> +       }

This makes the path counting in nd/checkout-noisy less accurate. But
it's not your fault of course.

Junio, do you still want to merge that series down to 'next' or drop
it? If it will be merged down, I'll keep a note and fix it once this
one lands too.

> +
>         if (topath)
>                 return write_entry(ce, topath, state, 1);
>
> --
> 2.20.0.405.gbc1bbc6f85
>
Elijah Newren Dec. 10, 2018, 5:49 p.m. UTC | #2
On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> 'checkout_entry()' currently only supports creating new entries in the
> working tree, but not deleting them.  Add the ability to remove
> entries at the same time if the entry is marked with the CE_WT_REMOVE
> flag.
>
> Currently this doesn't have any effect, as the CE_WT_REMOVE flag is
> only used in unpack-tree, however we will make use of this in a
> subsequent step in the series.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  entry.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/entry.c b/entry.c
> index 3ec148ceee..cd1c6601b6 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -441,6 +441,13 @@ int checkout_entry(struct cache_entry *ce,
>         static struct strbuf path = STRBUF_INIT;
>         struct stat st;
>
> +       if (ce->ce_flags & CE_WT_REMOVE) {
> +               if (topath)
> +                       BUG("Can't remove entry to a path");

Minor nit: This error message is kinda hard to parse, for someone not
that familiar with all the *_entry functions, like myself.  Maybe add
a comment before this line:
    /* No content and thus no path to create, so we have no pathname
to return */
or reword the error slightly?  Or maybe it's fine and I was just
confused from lack of code familiarity, but I'll throw it out there
since I stumbled on it a bit.

> +               unlink_entry(ce);
> +               return 0;
> +       }
> +
>         if (topath)
>                 return write_entry(ce, topath, state, 1);
>
> --
> 2.20.0.405.gbc1bbc6f85
Junio C Hamano Dec. 11, 2018, 2:28 a.m. UTC | #3
Duy Nguyen <pclouds@gmail.com> writes:

>> +       if (ce->ce_flags & CE_WT_REMOVE) {
>> +               if (topath)
>> +                       BUG("Can't remove entry to a path");
>> +               unlink_entry(ce);
>> +               return 0;
>> +       }
>
> This makes the path counting in nd/checkout-noisy less accurate. But
> it's not your fault of course.

When we check out absense of one path, how do we want to count it?
Do we say "one path checked out?" when we remove one path?

> Junio, do you still want to merge that series down to 'next' or drop
> it? If it will be merged down, I'll keep a note and fix it once this
> one lands too.

Sure, I still agree with you that "git checkout" that reports what
it did when given a "<branch>", but does not report what it did when
given a "<pathspec>", is being inconsistent.  If it makes it easier
to manage, I can kick nd/checkout-noisy out of 'next' to be rebased
on whatever more appropriate when rewinding its tip.

Thanks.
Thomas Gummerer Dec. 11, 2018, 10 p.m. UTC | #4
On 12/10, Elijah Newren wrote:
> On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > 'checkout_entry()' currently only supports creating new entries in the
> > working tree, but not deleting them.  Add the ability to remove
> > entries at the same time if the entry is marked with the CE_WT_REMOVE
> > flag.
> >
> > Currently this doesn't have any effect, as the CE_WT_REMOVE flag is
> > only used in unpack-tree, however we will make use of this in a
> > subsequent step in the series.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >  entry.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/entry.c b/entry.c
> > index 3ec148ceee..cd1c6601b6 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -441,6 +441,13 @@ int checkout_entry(struct cache_entry *ce,
> >         static struct strbuf path = STRBUF_INIT;
> >         struct stat st;
> >
> > +       if (ce->ce_flags & CE_WT_REMOVE) {
> > +               if (topath)
> > +                       BUG("Can't remove entry to a path");
> 
> Minor nit: This error message is kinda hard to parse, for someone not
> that familiar with all the *_entry functions, like myself.  Maybe add
> a comment before this line:
>     /* No content and thus no path to create, so we have no pathname
> to return */
> or reword the error slightly?  Or maybe it's fine and I was just
> confused from lack of code familiarity, but I'll throw it out there
> since I stumbled on it a bit.

I'll try to make it more clear in the new round, thanks!

> > +               unlink_entry(ce);
> > +               return 0;
> > +       }
> > +
> >         if (topath)
> >                 return write_entry(ce, topath, state, 1);
> >
> > --
> > 2.20.0.405.gbc1bbc6f85
Duy Nguyen Dec. 12, 2018, 6:16 a.m. UTC | #5
On Tue, Dec 11, 2018 at 3:28 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> >> +       if (ce->ce_flags & CE_WT_REMOVE) {
> >> +               if (topath)
> >> +                       BUG("Can't remove entry to a path");
> >> +               unlink_entry(ce);
> >> +               return 0;
> >> +       }
> >
> > This makes the path counting in nd/checkout-noisy less accurate. But
> > it's not your fault of course.
>
> When we check out absense of one path, how do we want to count it?
> Do we say "one path checked out?" when we remove one path?

It is still "checked out" according to this non-overlay concept.
Although we could make it clear by saying "5 paths updated, 2 deleted"
(but that may make us say "3 paths added" as well, hmm). Or maybe just
"%d paths updated" where updates include file creation and deletion.
Junio C Hamano Dec. 12, 2018, 7:36 a.m. UTC | #6
Duy Nguyen <pclouds@gmail.com> writes:

> Although we could make it clear by saying "5 paths updated, 2 deleted"
> (but that may make us say "3 paths added" as well, hmm). Or maybe just
> "%d paths updated" where updates include file creation and deletion.

Yeah, the last one is the simplest and good.

Patch
diff mbox series

diff --git a/entry.c b/entry.c
index 3ec148ceee..cd1c6601b6 100644
--- a/entry.c
+++ b/entry.c
@@ -441,6 +441,13 @@  int checkout_entry(struct cache_entry *ce,
 	static struct strbuf path = STRBUF_INIT;
 	struct stat st;
 
+	if (ce->ce_flags & CE_WT_REMOVE) {
+		if (topath)
+			BUG("Can't remove entry to a path");
+		unlink_entry(ce);
+		return 0;
+	}
+
 	if (topath)
 		return write_entry(ce, topath, state, 1);