From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 27 Sep 2022 14:13:56 GMT
Subject: Re: [PATCH 05/10] evolve: add the change-table structure
Message-ID: <ae031714-ba51-fe39-6351-ebd638840c32@dunelm.org.uk>
In-Reply-To: <220927.865yh8zzux.gmgdl@evledraar.gmail.com>



On 27/09/2022 14:50, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Sep 27 2022, Phillip Wood wrote:
> 
>> On 23/09/2022 19:55, Stefan Xenos via GitGitGadget wrote:
>>> From: Stefan Xenos <sxenos@google.com>
>>> A change table stores a list of changes, and supports efficient
>>> lookup
>>> from a commit hash to the list of changes that reference that commit
>>> directly.
>>> It can be used to look up content commits or metacommits at the head
>>> of a change, but does not support lookup of commits referenced as part
>>> of the commit history.
>>> Signed-off-by: Stefan Xenos <sxenos@google.com>
>>> Signed-off-by: Chris Poucet <poucet@google.com>
>>
>>> diff --git a/change-table.h b/change-table.h
>>> new file mode 100644
>>> index 00000000000..166b5ed8073
>>> --- /dev/null
>>> +++ b/change-table.h
>>> @@ -0,0 +1,132 @@
>>> +#ifndef CHANGE_TABLE_H
>>> +#define CHANGE_TABLE_H
>>> +
>>> +#include "oidmap.h"
>>> +
>>> +struct commit;
>>> +struct ref_filter;
>>> +
>>> +/**
>>
>> We tend to just use '/*' rather than '/**'
> 
> No, we use both, and /** is correct here. It's an API-doc syntax, see
> e.g. strbuf.h.
> 
> It's explicitly meant for this sort of thing, i.e. comments on public
> structs in a header & the functions in a header (and struct members,
> etc.).

We don't do that consistently, we don't mention them in CodingGuidelines 
and we don't use anything that processes API-doc comments. It would be a 
lot simpler and it would be consistent with our coding guidelines just 
to use the same style everywhere. That would avoid problems where this 
series uses API-doc comments for in-code comments in .c files and where 
single line comments in header files do not use the API-doc syntax.

Best Wishes

Phillip

>>> + * This struct holds a list of change refs. The first element is
>>    stored inline,
>>> + * to optimize for small lists.
>>> + */
>>> +struct change_list {
>>> +	/**
>>> +	 * Ref name for the first change in the list, or null if none.
>>> +	 *
>>> +	 * This field is private. Use for_each_change_in to read.
>>> +	 */
>>> +	const char* first_refname;
>>> +	/**
> 
> While we're on nits we tend to add an extra \n before the next API
> comment...