Implements radicle_cli::git::get_abbrev(), which will get git’s core.abbrev, which can be applied to COB IDs and git OIDs (or any SHA1 abbreviation.)

This also respects special cases where core.abbrev can be legally set to “auto” or “no”, where “auto” gets an appropriate length based on a loose count of objects in the current repo, and “no” which doesn’t abbreviate the SHA1 at all.

Wasn’t able to find any good alternatives in calculating the true value for “auto”, so I’ll be opening this patch for review. See this zulip topic for context.

The Git documentation says that this should default to “auto”, and not to 7. The number 7 is actually not even mentioned. Quoting from https://git-scm.com/docs/git-config#Documentation/git-config.txt-coreabbrev:

If unspecified or set to “auto”, an appropriate value is computed based on the approximate number of packed objects in your repository, […]

Thus, if calls to git2::Config::open_default or git2::Config::get_entry fail, the logic for “auto” based on git rev-parse should be executed by setting abbrev to "auto". For the case where the git rev-parse invocation fails, I would suggest to go for a value greater than 7, maybe even 40.

Whoops.

I have been mostly making this patch with the specs from Issue# 1b3f17e8ad0247a047f162194ba406b11fb39f63 in mind, which specify defaulting to 7 if missing, but I haven’t read much into the docs for core.abbrev to realize this wasn’t the case with the actual core.abbrev specs.

I’ll update it in a bit, so thanks for the heads up. As for the rev-parse fallback, I think 10 should be safe enough.

Default to “auto” on config/entry failure change CONFIG_ABBRED_DEFAULT to 10

forgot to squash commits. whoops.

pub const CONFIG_ABBREV_DEFAULT: usize = 10;

Why did you opt for 10 here? I believe we’re currently using 7 at the moment.

/// Gets git's global `core.abbrev` config for formatting.

I’d actually expect for this to use a passed in git2::Config, so that we can use a working copy’s .git/config. That way it’s specific to the repository the individual is working within.

// UNSAFE: as we need to read from a mutable static.

I’m going to need some rationale as to why the static mut is needed for this. If it’s really needed, I believe we’re using once_cell for cached runtime statics.

abbrev = git::get_abbrev()

Cool! I didn’t know about this :D

@fintohaps :

Why did you opt for 10 here? I believe we’re currently using 7 at the moment.

It was at the suggestion of lorenz (see comments on revision#1e96844 of this patch) to go for a value >7. It kinda makes sense (to avoid birthday problems), but if the 7 is required then I can reset it back to issue spec.

I’m going to need some rationale as to why the static mut is needed for this. If it’s really needed, I believe we’re using once_cell for cached runtime statics.

As you might have already seen, this patch also modifies format::{cob(), oid()} so they also call get_abbrev(). The reason why I’m using a static mut for storing the CONFIG_ABBREV is most relevantly how many times we’d be calling functions that’d would eventually call get_abbrev(), and more importantly from where. I did get the locations from my NVIM lsp in which format::{cob(), oid()} are called, here are the locations:

format::cob():
radicle-cli/src/commands/inbox.rs|468 col 27| term::format::cob(id),
radicle-cli/src/commands/issue.rs|691 col 50| term::format::tertiary(term::format::cob(&id))
radicle-cli/src/commands/patch/checkout.rs|27 col 57| .join(RefString::try_from(term::format::cob(id).item).unwrap())),
radicle-cli/src/commands/patch/list.rs|143 col 46| term::format::tertiary(term::format::cob(id)).into(),
radicle-cli/src/commands/patch/review.rs|79 col 64| let patch_id_pretty = term::format::tertiary(term::format::cob(&patch_id));
radicle-cli/src/terminal/format.rs|54 col 8| pub fn cob(id: &ObjectId) -> Paint<String> {
radicle-remote-helper/src/push.rs|529 col 46| term::format::tertiary(term::format::cob(&patch_id)),

format::oid():
radicle-cli/src/commands/id.rs|437 col 40| let id = term::format::oid(r.id).into();
radicle-cli/src/commands/ls.rs|125 col 34| let head = term::format::oid(head).into();
radicle-cli/src/commands/patch/checkout.rs|96 col 41| term::format::dim(term::format::oid(revision_id)),
radicle-cli/src/commands/patch/list.rs|148 col 47| term::format::secondary(term::format::oid(revision.head())).into(),
radicle-cli/src/commands/patch/review/builder.rs|218 col 76| term::format::secondary(term::format::parens(term::format::oid(c.id()))),
radicle-cli/src/commands/sync.rs|354 col 31| term::format::oid(at.oid),
radicle-cli/src/commands/sync.rs|360 col 31| term::format::oid(remote.oid),
radicle-cli/src/git/unified_diff.rs|301 col 39| term::format::oid(old.oid),
radicle-cli/src/git/unified_diff.rs|302 col 39| term::format::oid(new.oid),
radicle-cli/src/git/unified_diff.rs|310 col 39| term::format::oid(old.oid),
radicle-cli/src/git/unified_diff.rs|311 col 39| term::format::oid(new.oid)
radicle-cli/src/git/unified_diff.rs|328 col 35| term::format::oid(Oid::zero()),
radicle-cli/src/git/unified_diff.rs|329 col 35| term::format::oid(new.oid),
radicle-cli/src/git/unified_diff.rs|349 col 35| term::format::oid(old.oid),
radicle-cli/src/git/unified_diff.rs|350 col 35| term::format::oid(Oid::zero())
radicle-cli/src/terminal/comment.rs|24 col 50| .child(term::Line::spaced([term::format::oid(*id)
radicle-cli/src/terminal/format.rs|30 col 8| pub fn oid(oid: impl Into<radicle::git::Oid>) -> Paint<String> {
radicle-cli/src/terminal/patch.rs|313 col 51| term::format::secondary(term::format::oid(commit.id()).into()),
radicle-cli/src/terminal/patch.rs|464 col 31| term::format::oid(commit.id()).into(),
radicle-cli/src/terminal/patch/timeline.rs|116 col 76| term::format::parens(term::format::secondary(term::format::oid(self.head))).into(),
radicle-cli/src/terminal/patch/timeline.rs|236 col 72| term::format::parens(term::format::secondary(term::format::oid(head))).into(),
radicle-cli/src/terminal/patch/timeline.rs|260 col 45| term::format::dim(term::format::oid(id)).into(),
radicle-cli/src/terminal/patch/timeline.rs|261 col 72| term::format::parens(term::format::secondary(term::format::oid(head))).into(),
radicle-cli/src/terminal/patch/timeline.rs|318 col 53| term::format::dim(term::format::oid(merge.revision)).into(),
radicle-cli/src/terminal/patch/timeline.rs|319 col 80| term::format::parens(term::format::secondary(term::format::oid(merge.commit)))
radicle-remote-helper/src/push.rs|660 col 45| term::format::dim(term::format::oid(revision)),

You could probably see why the static mut is there. If I had to remove the static mut and implement this cache elsewhere, then I’d also have to make that theoretical cache work to account for all of the above, and everyone else would have to take that cache into account when continuing to further develop the CLI frontend.

As for the once_cell, I haven’t heard of it when I have first written this patch, so I’ll take a look at it and see if I can get that to work for this as well.

I’d actually expect for this to use a passed in git2::Config, so that we can use a working copy’s .git/config. That way it’s specific to the repository the individual is working within.

Good point, but consider the above as well. If the local git config is expected to be used, then what I could do is to get the local git config within get_abbrev() (once), and then use its core.abbrev value to handle the rest or use the global config should it not be present locally.

Why did you opt for 10 here? I believe we’re currently using 7 at the moment.

It was at the suggestion of lorenz (see comments on revision#1e96844 of this patch) to go for a value >7. It kinda makes sense (to avoid birthday problems), but if the 7 is required then I can reset it back to issue spec.

Ah I see, I was under the impression that 7 was the default from the article I read, but indeed auto seems the better default. So, erm, is there a way we can use the default of auto if it’s not specified? :)

Regarding the other comments about static mut and the git2::Config, I’ll have to look at the patch again and form some more thoughts :)

So, erm, is there a way we can use the default of auto if it’s not specified? :)

I partially rewrote get_abbrev() a few days earlier to implement the once_cell (which also makes get_abbrev() safe btw) and also automatically resort to auto if it fails to obtain configs for the local repo, globally, and the underlying core.abbrev value, and defaults to CONFIG_ABBREV_DEFAULT (or 10) if even auto can’t be obtained.

I’ve been sitting on this for a bit until I had more of an idea on what to focus on as per review, but it seems I might have to make this git stash entry into a revision right about now.

Updates this patch to do the following:

- Use `std::sync::OnceLock` as a replacement for `static mut`
    - its actually in the std lib for rust as of 1.70.0, 
      see `https://github.com/rust-lang/rust/pull/105587`
    - the `once_cell` crate could prolly be removed as a dep in favor of this but im not
      sure if there are other uses of this dep that aren't covered in the std impl

- `CONFIG_ABBREV_DEFAULT` is now set to 10
- Will use the local config first (if it can be found) and uses the global config otherwise
  should the local config not be found
- Upon failing to obtain the local config, global config, or just the `core.abbrev` value,
  will simply default to "auto". If that also fails: `CONFIG_ABBREV_DEFAULT`.

Changes:

• Introduce Abbreviation type to make code clearer
• Use CONFIG_ABBREV_DEFAULT for default cases where any values are missing
• Make 7 the default value to ensure tests still pass

Changes:

• Improve Config search
• Add more constants to use
• Improve commit message