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

[Stefan Beller <sbeller@google.com>]

On Wed, Sep 6, 2017 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Yes. The intent was to deliver patches that are easiest to review as
that is usually the crux with long series. To do that we came up with this
way:
* patches  07-24 can be reviewed easily by only looking at
   the textual replacement, no need to think about implications.
   It should be boring(in a good way) to review these.
* patches 25-39, that allow arbitrary repositories to be handled
   only need to be reviewed inside its function bounds, because
   each function call that takes a repository argument will be verified
   by the compiler, precisely because the above won't work!
   That way the reviewer can rely on the compile output
   that the conversion was done in the correct order and no
   small function was missed. All review attention can go into
   just looking at the one function that is converted in such a
   patch. no need to double check with other parts of the code.
   We discussed we'd send the patches 25-39 with --function-context.
   The follow ups will do so.

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

No. We did these two different phases to ease review specifically.

Note how the call site is the function itself, ( --function-context would
have been really neat here)


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

The absence of a repository argument is not sufficient that a function
(or any function called by it) does not rely on some global state, which
needs to be put into the repository struct.

So when converting function A (above), the careful reviewer needs to
inspect B, C, D... if they touch global state. Using the current approach,
most functions (maybe just B and C here) would take a repository struct
already, and the compiler would yell if these were not ready for anything
except the_repository. So the reviewer only has to worry about D.

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

agreed.

> (2) it wouldn't have made the patch that longer,

but more complicated to review (and write actually; as it was easy
to reason about functions to be ready for conversion, not using
global state)

> 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?"

Jonathan is currently OOO, but last week we continued developing this
series and ended up with ca. 90 patches in the series.

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

We'll revisit if we can squash commits before sending out.

Thanks for the feedback,
Stefan