Glen Choo <chooglen@xxxxxxxxxx> writes: > Victoria Dye <vdye@xxxxxxxxxx> writes: > >> 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. > ... >> 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.
