garydgregory commented on PR #427:
URL: https://github.com/apache/commons-codec/pull/427#issuecomment-4138270635

   > @garydgregory,
   > 
   > What would you say about an API like the one below? It would have the 
advantage of being reusable in other contexts. For example Commons Compress 
could use it to compute a SWHID without extracting an archive.
   > 
   > ```java
   > public final class GitId {
   > 
   >     public enum FileMode {
   > 
   >         /** Regular, non-executable file ({@code 100644}). */
   >         REGULAR_FILE("100644"),
   > 
   >         /** Executable file ({@code 100755}). */
   >         EXECUTABLE_FILE("100755"),
   > 
   >         /** Symbolic link ({@code 120000}). */
   >         SYMBOLIC_LINK("120000"),
   > 
   >         /** Directory / subtree ({@code 40000}). */
   >         DIRECTORY("40000");
   > 
   >     }
   > 
   >     public static byte[] blobId(MessageDigest digest, byte[] content);
   > 
   >     public static byte[] blobId(MessageDigest digest, InputStream input) 
throws IOException;
   > 
   >     public static byte[] blobId(MessageDigest digest, Path path) throws 
IOException;
   > 
   >     public static TreeBuilder treeBuilder(MessageDigest digest);
   > 
   > 
   >     public static final class TreeBuilder {
   > 
   >         public TreeBuilder addFile(String name, FileMode mode, byte[] 
content);
   > 
   >         public TreeBuilder addFile(String name, FileMode mode, InputStream 
input) throws IOException;
   > 
   >         public TreeBuilder addFile(String name, FileMode mode, Path path) 
throws IOException;
   > 
   >         public TreeBuilder addDirectory(String name, TreeBuilder subtree);
   > 
   >         public byte[] build();
   >     }
   > }
   > ```
   
   Hi @ppkarwasz 
   
   I'm not sure what Commons component the above should belong. I think you 
mean it to belong in Codec but I can't tell what's supposed to be an interface 
vs. implementation. Would this PR be reimplemented in terms of the above? Or 
would this PR provide the implementation for the above? 
   
   The name TreeBuilder is confusing to me without Javadoc. It's not building a 
tree, it's building a byte array. Do you mean it processes a directory tree? I 
can't tell.
   
   In the PR description, you write:
   
   > In particular, the swh:1:cnt: (content) and swh:1:dir: (directory) 
identifier types defined by [SWHID (ISO/IEC 
18670)](https://www.swhid.org/specification/v1.2/5.Core_identifiers/) are 
currently compatible with Git blob and tree identifiers respectively (using 
SHA-1), and can be used to generate canonical, persistent identifiers for 
unpacked source and binary distributions.
   
   Since Git has been migrating to SHA-256, does this still matter? You only 
mention SHA-1 in the above.
    
   From API design, the API inflation is already present with byte[], 
InputStream, Path, and hints that File, Channel, Buffer, and URI should also be 
available, which is the problem Commons IOs builder package attempts to solve.
   
   Aside from that, the current PR seems focused on narrow functionality 
without introducing framework code, so it fits in nicely. Let me review it 
again in the morning.
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to