Re: [PATCH v4 02/17] pack-mtimes: support reading .mtimes files

From: Taylor Blau <me@ttaylorr.com>
Date: May 20, 2022, 22:37 UTC
Message-ID: <YogYP03hDH8hZqUV@nand.local>
In-Reply-To: <220520.86bkvs3bfm.gmgdl@evledraar.gmail.com>

On Fri, May 20, 2022 at 09:32:50AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 49 quoted lines

>
> On Thu, May 19 2022, Junio C Hamano wrote:
>
> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >
> >> On Wed, May 18 2022, Taylor Blau wrote:
> >>
> >> Nit:
> >>
> >>> +  - A 4-byte magic number '0x4d544d45' ('MTME').
> >>> +
> >>> +  - A 4-byte version identifier (= 1).
> >>> +
> >>> +  - A 4-byte hash function identifier (= 1 for SHA-1, 2 for SHA-256).
> >>
> >> Here we let it suffice that later we'll say "All 4-byte numbers are in
> >> network order".
> >>
> >>> +  - A table of 4-byte unsigned integers in network order. The ith
> >>
> >> But here we call out "network order" explicitly, shouldn't this just be
> >> s/ in network order//?
> >>
> >>> +    value is the modification time (mtime) of the ith object in the
> >>> +    corresponding pack by lexicographic (index) order. The mtimes
> >>> +    count standard epoch seconds.
> >>> +
> >>> +  - A trailer, containing a checksum of the corresponding packfile,
> >>> +    and a checksum of all of the above (each having length according
> >>> +    to the specified hash function).
> >>> +
> >>> +All 4-byte numbers are in network order.
> >>
> >> I.e. this is sufficient.
> >
> > Very good eyes.  One explicit mention among several others can
> > indeed be misleading the readers.
> >
> > When asked for "network order", all your search engines show are
> > entries about "network byte order", so let's use that longer form of
> > spelling.
>
> *Nod*, note that "network order" is on "master" already though,
> i.e. this section re-used a template introduced in 2f4ba2a867f
> (packfile: prepare for the existence of '*.rev' files, 2021-01-25) just
> above this hunk.
>
> Before that change the rest of the file used "network byte order"
> consistently.

Hmm. e0d1bcf825 (multi-pack-index: add format details, 2018-07-12) (which predates 2f4ba2a867f by a few years) introduced the first use of "network order" as opposed to "network byte order".

I think it's worth cleaning this up, but let's do it in two parts. I'll send a rerolled version of tb/cruft-packs that moves the "All 4-byte numbers are in network order" to the top of that section, switching "network order" for "network byte order", and dropping other mentions of "network [byte] order" from that section.

Then, we can come back later and perhaps do something like the following (but I don't want to do it now and tie up this series with semi-related cleanups):

--- 8< ---

diff --git a/Documentation/technical/pack-format.txt b/Documentation/technical/pack-format.txt
index b520aa9c45..2591a410fd 100644
--- a/Documentation/technical/pack-format.txt
+++ b/Documentation/technical/pack-format.txt
@@ -276,6 +276,8 @@ Pack file entry: <+

 == pack-*.rev files have the format:

+All 4-byte numbers are in network byte order.
+
   - A 4-byte magic number '0x52494458' ('RIDX').

   - A 4-byte version identifier (= 1).
@@ -283,8 +285,8 @@ Pack file entry: <+
   - A 4-byte hash function identifier (= 1 for SHA-1, 2 for SHA-256).

   - A table of index positions (one per packed object, num_objects in
-    total, each a 4-byte unsigned integer in network order), sorted by
-    their corresponding offsets in the packfile.
+    total, each a 4-byte unsigned integer), sorted by their
+    corresponding offsets in the packfile.

   - A trailer, containing a:

@@ -292,8 +294,6 @@ Pack file entry: <+

     a checksum of all of the above.

-All 4-byte numbers are in network order.
-
 == pack-*.mtimes files have the format:

 All 4-byte numbers are in network byte order.
@@ -322,7 +322,7 @@ the body into "chunks" and provide a lookup table at the beginning of the
 body. The header includes certain length values, such as the number of packs,
 the number of base MIDX files, hash lengths and types.

-All 4-byte numbers are in network order.
+All 4-byte numbers are in network byte order.

 HEADER:

@@ -397,8 +397,8 @@ CHUNK DATA:

 	[Optional] Bitmap pack order (ID: {'R', 'I', 'D', 'X'})
 	    A list of MIDX positions (one per object in the MIDX, num_objects in
-	    total, each a 4-byte unsigned integer in network byte order), sorted
-	    according to their relative bitmap/pseudo-pack positions.
+	    total, each a 4-byte unsigned integer), sorted according to their
+	    relative bitmap/pseudo-pack positions.

 TRAILER:


--- >8 ---

Thanks,
Taylor