Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for SHA-256 repositories #1302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koutcher
Copy link
Collaborator

@koutcher koutcher commented Nov 3, 2023

With Git 2.42, SHA-256 repositories are no longer experimental. This PR adds support to Tig for SHA-256 repositories.

src/tree.c Outdated
@@ -58,7 +58,7 @@ push_tree_stack_entry(struct view *view, const char *name, struct position *posi
*/

#define SIZEOF_TREE_ATTR \
STRING_SIZE("100644 blob f931e1d229c3e185caad4449bf5b66ed72462657\t")
STRING_SIZE("100644 blob ") + repo.hash_len + 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe

Suggested change
STRING_SIZE("100644 blob ") + repo.hash_len + 1
STRING_SIZE("100644 blob ") + repo.hash_len + STRING_SIZE("\t")

@@ -203,6 +203,8 @@ following variables.
|%(repo:is-inside-work-tree)
|Whether Tig is running inside a work tree,
either `true` or `false`.
|%(repo:hash_len) |The hash algorithm used for the repository, e.g. `sha1`
or `sha256`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be called %(repo:extensions.objectformat) to use the standard Git term.
Actually, is there even a need to expose this at all?
I don't think we expose any other config options. Users can already consult something like
git --git-dir=%(repo:git-dir) config extensions.objectformat

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @krobelus. %(repo:hash_len) is actually a by-product of storing hash_len in the repo struct, but it's the best compromise I have found. I'll make it more elegant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it is exposed by the REPO_INFO macro. Maybe define it outside that

src/options.c Outdated
@@ -1531,6 +1531,9 @@ read_repo_config_option(char *name, size_t namelen, char *value, size_t valuelen
else if (!strcmp(name, "core.abbrev"))
parse_int(&opt_id_width, value, 0, SIZEOF_REV - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really important: we have two places where we call
parse_int() with a max of SIZEOF_REV - 1.
The effective max should maybe be lower for sha1 repos.
But I'm not sure if that would cause unnecessary friction, so better keep it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial code used hash_len but we haven't read extension.objectFormat yet here, so we can't use it. I'll improve this part so that the same configuration can be used in global files for both sha1 and sha256 repositories.

@@ -19,7 +19,7 @@
#include "tig/util.h"

struct commit {
char id[SIZEOF_REV]; /* SHA1 ID. */
char id[SIZEOF_REV]; /* Hash ID. */
Copy link
Contributor

@krobelus krobelus Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good apart from one comment about %(repo:hash_len).
I haven't used sha256, I'll try it if I get around to.
It's a shame we can't reuse existing unit tests without modification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually git fast-export and git fast-import allow to port the tests without too much effort and also to get a nice sandbox from an existing repository.

@koutcher koutcher force-pushed the sha-256 branch 4 times, most recently from f4c4757 to 6475d09 Compare November 21, 2023 21:13
@koutcher koutcher force-pushed the sha-256 branch 2 times, most recently from 98b589d to 357af1a Compare March 30, 2024 14:59
Use variable extensions.objectformat, if available, to determine the
length of the hash id in the current repository.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants