Re: [PATCH 25/39] sha1_file: allow alt_odb_usable to handle arbitrary repositories

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2017, 20:01 UTC
Message-ID: <xmqqd1734mvg.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170830070812.GZ153983@aiede.mtv.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 27 quoted lines

> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  sha1_file.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index e7c86e5363..b854cad970 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -25,6 +25,7 @@
>  #include "repository.h"
>  #include "object-store.h"
>  #include "streaming.h"
> +#include "path.h"
>  #include "dir.h"
>  #include "mru.h"
>  #include "list.h"
> @@ -281,17 +282,18 @@ static const char *alt_sha1_path(struct alternate_object_database *alt,
>  /*
>   * Return non-zero iff the path is usable as an alternate object database.
>   */
> -#define alt_odb_usable(r, p, n) alt_odb_usable_##r(p, n)
> -static int alt_odb_usable_the_repository(struct strbuf *path,
> -					 const char *normalized_objdir)
> +static int alt_odb_usable(struct repository *r, struct strbuf *path,
> +			  const char *normalized_objdir)

These token-pasting macros introduced in 07-24/39 were certainly cute but they may be rather misleading and useless. e.g. a natural expectaion would be

	struct repository *r = &the_repository;
	prepare_alt_odb(r);

to work, but obviously it wouldn't. And this step and later in 25-39/39 corrects them one by one.

I suspect that you used the token-pasting cuteness because you thought that the callsite would not have to change in the later step like 25/39 that removes the trick. I also suspect that you thought that it may be a good thing that prepare_alt_odb(r) does not work immediately after 07/39, as the callee is not prepared. But both of these are misguided.

If you have two functions, A and B, that we want to update to eventually take a "struct repository *" argument, and if A uses B in its implementation, the endgame would be

	A(struct repository *r, ...)
	{
		...
		B(r, ...);
		...
	}

but with the ##r approach, the call to B in A in the initial token-pasting phase would say B(the_repository, ...) so the call will need to be changed in the step you update A's implementation to take a caller-supplied respotory object anyway. And the fact that a function's first parameter is not the_repository (i.e. leaving B()'s signature unchanged while it is not ready to take a repository) is sufficient to mark that a function is not yet prepared to take a caller-supplied repository object.

I found that this aspect of the structure of the series was somewhat irritating in that (1) combining the earlier ##r thing with its fix would have made the code that needs to be reviewed much cleaner, (2) it wouldn't have made the patch that longer, and most importantly (3) it is unclear why 24-7 != 39-25, i.e. it is hard to answer "is there any ##r hack still remaining after this series? if so why?"

The end-result of the whole series makes me think that it is going in the right direction, though.

Thanks.