[1/9] tmp_objdir: add a helper function for discarding all contained objects

Commit Message

Elijah Newren Dec. 21, 2021, 6:05 p.m. UTC

From: Elijah Newren <[e-mail]>

Signed-off-by: Elijah Newren <[e-mail]>
---
 tmp-objdir.c | 5 +++++
 tmp-objdir.h | 6 ++++++
 2 files changed, 11 insertions(+)

Comments

Junio C Hamano Dec. 21, 2021, 11:26 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> From: Elijah Newren <[e-mail]>
>
> Signed-off-by: Elijah Newren <[e-mail]>
> ---
>  tmp-objdir.c | 5 +++++
>  tmp-objdir.h | 6 ++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/tmp-objdir.c b/tmp-objdir.c
> index 3d38eeab66b..adf6033549e 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -79,6 +79,11 @@ static void remove_tmp_objdir_on_signal(int signo)
>  	raise(signo);
>  }
>  
> +void tmp_objdir_discard_objects(struct tmp_objdir *t)
> +{
> +	remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
> +}
> +

OK.

Without a caller, it is a bit hard to judge if a separate helper
makes the caller easier to read and understand, or becomes an extra
layer of abstraction that obscures the logic.  Hopefully, having a
more specific function name with "tmp" and "discard" in it makes the
intent at callers more clear than the function that is named after
the detail of the operation.

> +/*
> + * Remove all objects from the temporary object directory, while leaving it
> + * around so more objects can be added.
> + */
> +void tmp_objdir_discard_objects(struct tmp_objdir *);
> +
>  /*
>   * Add the temporary object directory as an alternate object store in the
>   * current process.

Elijah Newren Dec. 21, 2021, 11:51 p.m. UTC | #2

On Tue, Dec 21, 2021 at 3:26 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > From: Elijah Newren <[e-mail]>
> >
> > Signed-off-by: Elijah Newren <[e-mail]>
> > ---
> >  tmp-objdir.c | 5 +++++
> >  tmp-objdir.h | 6 ++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/tmp-objdir.c b/tmp-objdir.c
> > index 3d38eeab66b..adf6033549e 100644
> > --- a/tmp-objdir.c
> > +++ b/tmp-objdir.c
> > @@ -79,6 +79,11 @@ static void remove_tmp_objdir_on_signal(int signo)
> >       raise(signo);
> >  }
> >
> > +void tmp_objdir_discard_objects(struct tmp_objdir *t)
> > +{
> > +     remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
> > +}
> > +
>
> OK.
>
> Without a caller, it is a bit hard to judge if a separate helper
> makes the caller easier to read and understand, or becomes an extra
> layer of abstraction that obscures the logic.  Hopefully, having a
> more specific function name with "tmp" and "discard" in it makes the
> intent at callers more clear than the function that is named after
> the detail of the operation.

This isn't just a convenience; since tmp_objdir is defined in
tmp-objdir.c rather than tmp-objdir.h, t->path is not accessible
outside of tmp-objdir.c.  Because of this, some kind of helper
function is necessary.

Junio C Hamano Dec. 22, 2021, 6:23 a.m. UTC | #3

Elijah Newren <[e-mail]> writes:

>> > +void tmp_objdir_discard_objects(struct tmp_objdir *t)
>> > +{
>> > +     remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
>> > +}
>> > +
>>
>> OK.
>>
>> Without a caller, it is a bit hard to judge if a separate helper
>> makes the caller easier to read and understand, or becomes an extra
>> layer of abstraction that obscures the logic.  Hopefully, having a
>> more specific function name with "tmp" and "discard" in it makes the
>> intent at callers more clear than the function that is named after
>> the detail of the operation.
>
> This isn't just a convenience; since tmp_objdir is defined in
> tmp-objdir.c rather than tmp-objdir.h, t->path is not accessible
> outside of tmp-objdir.c.  Because of this, some kind of helper
> function is necessary.

But adding this function as an extra level of abstration is *not*
the only way to expose the feature.  Instead the internal of "struct
tmp_objdir" could be exposed to the caller that wants to discard the
files inside the path.

I think we now have enough material to fill between these two lines
to help readers ;-)

>> > From: Elijah Newren <[e-mail]>
>> >
>> > Signed-off-by: Elijah Newren <[e-mail]>

Thanks.

Elijah Newren Dec. 25, 2021, 2:29 a.m. UTC | #4

On Tue, Dec 21, 2021 at 10:23 PM Junio C Hamano <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
>
> >> > +void tmp_objdir_discard_objects(struct tmp_objdir *t)
> >> > +{
> >> > +     remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
> >> > +}
> >> > +
> >>
> >> OK.
> >>
> >> Without a caller, it is a bit hard to judge if a separate helper
> >> makes the caller easier to read and understand, or becomes an extra
> >> layer of abstraction that obscures the logic.  Hopefully, having a
> >> more specific function name with "tmp" and "discard" in it makes the
> >> intent at callers more clear than the function that is named after
> >> the detail of the operation.
> >
> > This isn't just a convenience; since tmp_objdir is defined in
> > tmp-objdir.c rather than tmp-objdir.h, t->path is not accessible
> > outside of tmp-objdir.c.  Because of this, some kind of helper
> > function is necessary.
>
> But adding this function as an extra level of abstration is *not*
> the only way to expose the feature.  Instead the internal of "struct
> tmp_objdir" could be exposed to the caller that wants to discard the
> files inside the path.

Ah, yes, we talked about that during the tmp-objdir discussion back in
September/October.  Peff didn't want struct tmp_objdir exposed, and I
was operating with that in mind.

> I think we now have enough material to fill between these two lines
> to help readers ;-)

I've restructured the series a bit based on Ævar's feedback, and this
function is now only introduced along with its caller.  Hopefully that
makes it a bit clearer.

Patch

diff --git a/tmp-objdir.c b/tmp-objdir.c
index 3d38eeab66b..adf6033549e 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -79,6 +79,11 @@  static void remove_tmp_objdir_on_signal(int signo)
 	raise(signo);
 }
 
+void tmp_objdir_discard_objects(struct tmp_objdir *t)
+{
+	remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
+}
+
 /*
  * These env_* functions are for setting up the child environment; the
  * "replace" variant overrides the value of any existing variable with that
diff --git a/tmp-objdir.h b/tmp-objdir.h
index cda5ec76778..76efc7edee5 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -46,6 +46,12 @@  int tmp_objdir_migrate(struct tmp_objdir *);
  */
 int tmp_objdir_destroy(struct tmp_objdir *);
 
+/*
+ * Remove all objects from the temporary object directory, while leaving it
+ * around so more objects can be added.
+ */
+void tmp_objdir_discard_objects(struct tmp_objdir *);
+
 /*
  * Add the temporary object directory as an alternate object store in the
  * current process.