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

Elijah Newren [off-list ref] writes:

quoted

quoted

+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 ;-)

quoted

quoted

From: Elijah Newren <redacted>

Signed-off-by: Elijah Newren <redacted>

Thanks.

On Tue, Dec 21, 2021 at 10:23 PM Junio C Hamano [off-list ref] wrote:

Elijah Newren [off-list ref] writes:

quoted

quoted

quoted

+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.