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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Glen Choo wrote:
> This patch adds another instance of copying fields from "struct
> repository_format" to "struct repository", so I think that we should
> start doing this with a helper function instead of copy-pasting the
> logic.
> 
> As for what should be in the helper function, the above hunks suggest
> that we should copy .hash_algo, .partial_clone, and .worktree_config.
> However...
> 

...

> 
> This hunk does not copy .hash_algo. I initially wondered if it is safe
> to just copy .hash_algo here too, but I now suspect that we shouldn't
> have done the_repository setup in discover_git_directory() in the first
> place. It isn't used by the setup.c machinery - its one caller in "git"
> (it's used by "scalar") is read_early_config(), which is supposed to
> work without a fully set up repository, and bears a comment saying that
> "no global state is changed" by calling discover_git_directory() (which
> stopped being true in ebaf3bcf1ae). It looks like
> discover_git_directory() is just a lightweight entrypoint into the
> setup.c machinery. 16ac8b8db6 (setup: introduce the
> discover_git_directory() function, 2017-03-13)) says "Let's just provide
> a convenient wrapper function with an easier signature that *just*
> discovers the .git/ directory. We will use it in a subsequent patch to
> fix the early config."
> 
> If I'm wrong and we _should_ be doing the_repository setup, then I'm
> guessing it's safe to copy .hash_algo here too. So either way, I think
> we should introduce a helper function to do the copying, especially
> because we will probably need to repeat this process yet again for
> "repository_format_precious_objects".

Thanks for pointing this out & sharing your findings! 

I agree with the desire to reduce code duplication, but the reason I avoided
that refactor when putting these patches together is because of the subtle
differences across the different repository format assignment blocks.

For example, in addition to what you mentioned here w.r.t. '.hash_algo',
there are also differences in how 'repository_format_partial_clone' is
assigned: it's deep-copied in 'check_repository_format', but shallow-copied
(then subsequently NULL'd in the 'struct repository_format' to avoid freeing
the pointer when the struct is disposed of) in 'discover_git_directory()' &
'setup_git_directory_gently()'. 

If we were to settle on a single "copy repository format settings" function,
it's not obvious what the "right" approach is. We could change
'check_repository_format()' to the shallow-copy-then-null like the others:
its two callers (in 'init-db.c' and 'path.c') don't use the value of
'repository_format_partial_clone' in 'struct repository_format' after
calling 'check_repository_format()'. But, if we did that, it'd introduce a
side effect to the input 'struct repository_format', which IMO would be
surprising behavior for a function called 'check_<something>()'. Conversely,
unifying on a deep copy or adding a flag to toggle deep vs. shallow copy
feels like unnecessary complexity if we don't actually need a deep copy.

Beyond the smaller subtleties, there's the larger question (that you sort of
get at with the questions around 'discover_git_directory()') as to whether
we should more heavily refactor or consolidate these setup functions. The
similar code implies "yes", but such a refactor feels firmly out-of-scope
for this series. A smaller change (e.g. just moving the assignments into
their own function) could be less of a diversion, but any benefit seems like
it'd be outweighed by the added churn/complexity of a new function.

In any case, sorry for the long-winded response. I'd initially tried to
implement your feedback, but every time I did I'd get stopped up on the
things I mentioned above. So, rather than continue to put off responding to
this thread, I tried to capture what kept stopping me from moving forward -
hopefully it makes (at least a little bit of) sense!




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux