Re: [PATCH] setup: copy repository_format using helper

Glen Choo [off-list ref] writes:

Victoria Dye [off-list ref] writes:

quoted

So, shouldn't it be safe to shallow-copy-and-NULL? But as I noted earlier
[1], if you do that it'll make the name 'check_repository_format()' a bit
misleading (since it's actually modifying its arg in place). So, if you
update to always shallow copy, 'check_repository_format()' should be renamed
to reflect its side effects.

My understanding of check_repository_format() is that it serves double
duty of doing a) setup of the_repository and b) populating an "out"
parameter with the appropriate values. IMO a) is the side effect that
could warrant the rename, and b) is the expected, "read-only" use case.

From that perspective, doing a shallow copy here isn't really
introducing a weird side-effect (because the arg to an "out" parameter
should be zero-ed out to begin with), but it's returning a 'wrong'
value. You're right that it's safe because the NULL-ed value isn't read
back right now, but it's not any good if this function gains more
callers.

Thanks for having this discussion.  The above makes perfect sense to
me.

The helper function might not be a good idea yet, but I'm convinced that
removing the setup from discover_git_directory() is a good idea. I think
this series would be in a better state if we get rid of the wrong
pattern instead of extending it.
...

quoted

I think you may be missing changes to 'discover_git_directory()'? Like I
mentioned above, though, if you don't think 'discover_git_directory()' needs
to set up 'the_repository', then those assignments should just be removed
(not replaced with 'setup_repository_from_format()').

Ah sorry, yes they were meant to be removed. I somehow missed those as I
was preparing the patch.

It looks like you two are in agreement at the end.  It does feel
that the change to make discover purely about discovering extends
the scope a bit too much, but it would be a good direction to go in
the longer term.

Thanks.