Re: [PATCH] [PATCH] [Outreachy] builtin/patch-id.c: clarify SHA1 usage for patch IDs

[Junio C Hamano <gitster@pobox.com>]

Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:

> Patch IDs in Git must always use SHA1, regardless of the repository's
> object hash. Previously, the code relied on `the_hash_algo` which could
> vary depending on the repository, and included a NEEDSWORK comment
> suggesting this should be fixed.

I do not think that is what the comment suggests to do.

Read it again:

    ... should be removed in favor of converting the code that
    computes patch IDs to always use SHA1.

There are code paths that compute patch IDs elsewhere, and they are
not immediately below the NEEDSWORK comment.  The code relies on
the_hash_algo and that is why the "hack" makes repo_set_hash_algo()
call.  The suggestion is to convert that code that hashes patch to
compute patch IDs not to use the_hash_algo that is repository
dependeant.  I think get_one_pathcid() function in the same file is
one of them.

> This patch updates the comment to clearly state that SHA1 is required
> for patch IDs and sets the hash algorithm to SHA1 if it is not already
> set. This ensures consistent computation of patch IDs in accordance
> with git-patch-id(1).

And if it is already set?  I think what your first paragraph claims
to be problematic is that case, and the patch does not touch that
case at all.

Blindly setting the_hash_algo to SHA-1 may not be the end of the
"solution", so whoever wants to work on this needs to be extra
careful.  If the code after this point, starting from the call to
generate_id_list() we see in the post context, need to touch any Git
objects in the current repository (e.g., to obtain patch text or
some configuration data), such accesses need to use the hash that
the repository uses.  Only the final "now we have this patch, and we
learned what the configuration says how we should compute the
patch-id.  Let's hash the patch text following the specified
algorithm" step should use SHA-1 as the hash algorithm.

Perhaps we are lucky that this program has *no* need to access
objects in the repository (my quick scan says this seems to work on
an external text file and does not generate diffs locally out of
objects), and it may not depend on configuration data coming from
any objects in the repository (there are some configuration variables
whose values are blob object names that instructs Git to read such
an object).  In such a case, then the solution may be to always
make the code ignore the_hash_algo and unconditionally using SHA1.