Thread (26 messages) 26 messages, 5 authors, 2023-06-13

Re: [PATCH v2 3/3] repository: move 'repository_format_worktree_config' to repo scope

From: Glen Choo <hidden>
Date: 2023-06-12 20:24:00

Victoria Dye [off-list ref] writes:
Even if we updated the only other 'repository_format' value
('repository_format_precious_objects') to be copied the same way, the
benefit we'd get from eliminating a couple of lines of code duplication
wouldn't necessarily outweigh the the extra complexity of a new abstraction
- which may or may not need special-casing based on who's calling it -
and/or the risk associated with changing behavior if we want to eliminate
those special cases. IOW, I don't feel it's a definitive net improvement in
this situation.
I see. In the process of doing this digging, I've become quite convinced
that the risk is minimal. I definitely want the refactor to happen, but
I suppose it's not reasonable for you to bear the risk.

I'll send a follow up patch on top of your series that implements the
cleanup I hope to see, and I'd be happy to give _that_ series a
Reviewed-by (though it's a bit weird since one of the patches will be
mine). It'll touch the same lines twice, but at least the patches will
be owned by the people who care about them the most.
quoted
E.g. we could support both deep and shallow copying, like:

  /*
   * Copy members from a repository_format to repository.
   *
   * If 'src' will no longer be read after copying (e.g. it will be
   * cleared soon), pass a nonzero value so that pointer members will be
   * moved to 'dest' (NULL-ed and shallow copied) instead of being deep
   * copied.
   */
  void copy_repository_format(struct repository *dest,
                              struct repository_format *src,
                              int take_ownership);
Unless we find that we *need* to support both, this approach would be more
harmful than helpful. If it doesn't matter whether the copy is shallow or
deep, this design proliferates that meaningless distinction in a way that
can easily confuse developers (or at least create more work for them as try
to try to understand it) if they ever want to change or use the function.
Fair enough. I agree we're better off figuring out if the need exists
before trying to support it.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help