Re: [PATCH v3 02/20] commit-graph: fix GRAPH_MIN_SIZE

[Jakub Narebski]

"brian m. carlson"  writes:

> On Sat, May 26, 2018 at 08:46:09PM +0200, Jakub Narebski wrote:
>> One issue: in the future when Git moves to NewHash, it could encounter
>> then both commit-graph files using SHA-1 and using NewHash.  What about
>> GRPH_OID_LEN then: for one of those it would be incorrect.  Unless it is
>> about minimal length of checksum, that is we assume that NewHash would
>> be longer than SHA-1, but ten why name it GRAPH_OID_LEN?
>
> My proposal is that whatever we're using in the .git directory is
> consistent.  If we're using SHA-1 for objects, then everything is SHA-1.
> If we're using NewHash for objects, then all data is stored in NewHash
> (except translation tables and such).  Any conversions between SHA-1 and
> NewHash require a lookup through the standard techniques.
>
> I agree that here it would be more helpful if it were a reference to
> the_hash_algo, and I've applied a patch to my object-id-part14 series to
> make that conversion.

All right, I can agree that it would make most sense to always use SHA-1
for OID, or always use NewHash for objects.  This would make
commit-graph file with SHA-1 hash invalid for NewHash-using Git version.

It would be nice, however, to avoid having to redo all the hard work,
like calculating generation numbers (from old commit-graph file, or from
server that does not support NewHash yet -- the latter is not
implemented, but IIUC planned feature).  But we can do it with explicit
conversion step, e.g. 'git commit-graph convert' or 'upgrade'.

But all that is in the future.
-- 
Jakub Narębski

Re: [PATCH v3 02/20] commit-graph: fix GRAPH_MIN_SIZE

[brian m. carlson]

On Sat, May 26, 2018 at 08:46:09PM +0200, Jakub Narebski wrote:
> One issue: in the future when Git moves to NewHash, it could encounter
> then both commit-graph files using SHA-1 and using NewHash.  What about
> GRPH_OID_LEN then: for one of those it would be incorrect.  Unless it is
> about minimal length of checksum, that is we assume that NewHash would
> be longer than SHA-1, but ten why name it GRAPH_OID_LEN?

My proposal is that whatever we're using in the .git directory is
consistent.  If we're using SHA-1 for objects, then everything is SHA-1.
If we're using NewHash for objects, then all data is stored in NewHash
(except translation tables and such).  Any conversions between SHA-1 and
NewHash require a lookup through the standard techniques.

I agree that here it would be more helpful if it were a reference to
the_hash_algo, and I've applied a patch to my object-id-part14 series to
make that conversion.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 02/20] commit-graph: fix GRAPH_MIN_SIZE

[Jakub Narebski]

Derrick Stolee  writes:

> The GRAPH_MIN_SIZE macro should be the smallest size of a parsable
> commit-graph file. However, the minimum number of chunks was wrong.
> It is possible to write a commit-graph file with zero commits, and
> that violates this macro's value.
>
> Rewrite the macro, and use extra macros to better explain the magic
> constants.
>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index a8c337dd77..82295f0975 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -33,10 +33,11 @@
>  
>  #define GRAPH_LAST_EDGE 0x8000
>  
> +#define GRAPH_HEADER_SIZE 8

Nice.

>  #define GRAPH_FANOUT_SIZE (4 * 256)
>  #define GRAPH_CHUNKLOOKUP_WIDTH 12
> -#define GRAPH_MIN_SIZE (5 * GRAPH_CHUNKLOOKUP_WIDTH + GRAPH_FANOUT_SIZE + \
> - GRAPH_OID_LEN + 8)
> +#define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
> + + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)

So in this case we have file header (4-byte signature, 1-byte version
number, 1-byte oid/hash version, 1-byte number of chunks, 1-byte
reserved: 4+1+1+1+1 = 8 bytes), chunk lookup: 3 required chunks plus
terminating label = 4 entries, constant-size fanout chunks, and
checksum.  Two remaining required chunks (OID Lookup and Commit Data)
can have length of 0.


One issue: in the future when Git moves to NewHash, it could encounter
then both commit-graph files using SHA-1 and using NewHash.  What about
GRPH_OID_LEN then: for one of those it would be incorrect.  Unless it is
about minimal length of checksum, that is we assume that NewHash would
be longer than SHA-1, but ten why name it GRAPH_OID_LEN?

This may be going too much in the future; there is no need to borrow
trouble now, where we have only SHA-1 supported as OID.  Still...

>  
>  char *get_commit_graph_filename(const char *obj_dir)
>  {

Best,
-- 
Jakub Narębski

[PATCH v3 02/20] commit-graph: fix GRAPH_MIN_SIZE

[Derrick Stolee]

The GRAPH_MIN_SIZE macro should be the smallest size of a parsable
commit-graph file. However, the minimum number of chunks was wrong.
It is possible to write a commit-graph file with zero commits, and
that violates this macro's value.

Rewrite the macro, and use extra macros to better explain the magic
constants.

Signed-off-by: Derrick Stolee 
---
 commit-graph.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index a8c337dd77..82295f0975 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -33,10 +33,11 @@
 
 #define GRAPH_LAST_EDGE 0x8000
 
+#define GRAPH_HEADER_SIZE 8
 #define GRAPH_FANOUT_SIZE (4 * 256)
 #define GRAPH_CHUNKLOOKUP_WIDTH 12
-#define GRAPH_MIN_SIZE (5 * GRAPH_CHUNKLOOKUP_WIDTH + GRAPH_FANOUT_SIZE + \
-   GRAPH_OID_LEN + 8)
+#define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
+   + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)
 
 char *get_commit_graph_filename(const char *obj_dir)
 {
-- 
2.16.2.329.gfb62395de6