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

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

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.