Re: [PATCH] setup: copy repository_format using helper

[Junio C Hamano <gitster@pobox.com>]

Glen Choo <chooglen@google.com> writes:

> Victoria Dye <vdye@github.com> 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.