Subject: Re: What's cooking in git.git (May 2024, #02; Fri, 3)

From: Patrick Steinhardt <[email-masked]>
Date: Mon, 6 May 2024 11:21:59 +0200

On Fri, May 03, 2024 at 05:27:57PM -0700, Junio C Hamano wrote:
> * ps/config-subcommands (2024-05-03) 14 commits
>  - builtin/config: display subcommand help
>  - builtin/config: introduce "edit" subcommand
>  - builtin/config: introduce "remove-section" subcommand
>  - builtin/config: introduce "rename-section" subcommand
>  - builtin/config: introduce "unset" subcommand
>  - builtin/config: introduce "set" subcommand
>  - builtin/config: introduce "get" subcommand
>  - builtin/config: introduce "list" subcommand
>  - builtin/config: pull out function to handle `--null`
>  - builtin/config: pull out function to handle config location
>  - builtin/config: use `OPT_CMDMODE()` to specify modes
>  - builtin/config: move "fixed-value" option to correct group
>  - builtin/config: move option array around
>  - config: clarify memory ownership when preparing comment strings
> 
>  The operation mode options (like "--get") the "git config" command
>  uses have been deprecated and replaced with subcommands (like "git
>  config get").
> 
>  Will merge to 'next'?
>  source: <[email-masked]>

I have just sent out a new version [1] that fixes a few commit messages.
Other than that I think this version should be ready to go.

[1]: https://lore.kernel.org/git/[email-masked]/T/#u

> 
> * ps/refs-without-the-repository (2024-05-03) 5 commits
>  - refs: remove functions without ref store
>  - cocci: apply rules to rewrite callers of "refs" interfaces
>  - cocci: introduce rules to transform "refs" to pass ref store
>  - refs: add `exclude_patterns` parameter to `for_each_fullref_in()`
>  - refs: introduce missing functions that accept a `struct ref_store`
> 
>  The refs API lost functions that implicitly assumes to work on the
>  primary ref_store by forcing the callers to pass a ref_store as an
>  argument.
> 
>  Will merge to 'next'?
>  source: <[email-masked]>

It depends. Personally, I'm fine to merge this as-is and make this a
hard breakage. I don't really feel like it's worth it to have extra
compatibility macros or something like that.

If we want to play nice with current in-flight topics I'd propose to
just drop the last patch and reintroduce it in the next release cycle.
The downside here is that we may end up adding new callers to the old
interfaces, which may make us work against a moving target.

> * ps/undecided-is-not-necessarily-sha1 (2024-04-30) 13 commits
>  . repository: stop setting SHA1 as the default object hash
>  . oss-fuzz/commit-graph: set up hash algorithm
>  . builtin/shortlog: don't set up revisions without repo
>  . builtin/diff: explicitly set hash algo when there is no repo
>  . builtin/bundle: abort "verify" early when there is no repository
>  . builtin/blame: don't access potentially unitialized `the_hash_algo`
>  . builtin/rev-parse: allow shortening to more than 40 hex characters
>  . remote-curl: fix parsing of detached SHA256 heads
>  . attr: fix BUG() when parsing attrs outside of repo
>  . attr: don't recompute default attribute source
>  . parse-options-cb: only abbreviate hashes when hash algo is known
>  . path: move `validate_headref()` to its only user
>  . path: harden validation of HEAD with non-standard hashes
> 
>  Before discovering the repository details, We used to assume SHA-1
>  as the "default" hash function, which has been corrected. Hopefully
>  this will smoke out codepaths that rely on such an unwarranted
>  assumptions.
> 
>  Seems to break t0003 with a NULL the_repository.
> 
>  Ejected out of 'seen' for now.
>  source: <[email-masked]>

Interesting, I couldn't reproduce this issue when rebasing the patches
onto "seen". There were merge conflicts though, both with
jc/no-default-attr-tree-in-bare and ps/the-index-is-no-more. So maybe
there was a mismerge involved somewhere?

The first one looks like the following. Given that this merge conflict
happened in "attr.c", I strongly suspect that this one was mis-merged
because t0003 is about attrs, as well.

    diff --cc attr.c
    index 6af7151088,9d911aeb31..0000000000
    --- a/attr.c
    +++ b/attr.c
    @@@ -1223,8 -1224,15 +1224,8 @@@ static int compute_default_attr_source(
            ignore_bad_attr_tree = 1;
        }
      
    - 	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
    - 		return;
     -	if (!default_attr_source_tree_object_name &&
     -	    startup_info->have_repository &&
     -	    is_bare_repository()) {
     -		default_attr_source_tree_object_name = "HEAD";
     -		ignore_bad_attr_tree = 1;
     -	}
     -
    + 	if (!default_attr_source_tree_object_name)
    + 		return 0;
      
        if (repo_get_oid_treeish(the_repository,
                     default_attr_source_tree_object_name,

And the second conflict looks like this:

    diff --cc repository.c
    index 2118f563e3,b65b1a8c8b..0000000000
    --- a/repository.c
    +++ b/repository.c
    @@@ -17,35 -22,19 +17,15 @@@
      
      /* The main repository */
      static struct repository the_repo;
     -struct repository *the_repository;
     -struct index_state the_index;
     +struct repository *the_repository = &the_repo;
      
     -void initialize_the_repository(void)
     +void initialize_repository(struct repository *repo)
      {
     -	the_repository = &the_repo;
     -
     -	the_repo.index = &the_index;
     -	the_repo.objects = raw_object_store_new();
     -	the_repo.remote_state = remote_state_new();
     -	the_repo.parsed_objects = parsed_object_pool_new();
     -
     -	index_state_init(&the_index, the_repository);
     +	repo->objects = raw_object_store_new();
     +	repo->remote_state = remote_state_new();
     +	repo->parsed_objects = parsed_object_pool_new();
     +	ALLOC_ARRAY(repo->index, 1);
     +	index_state_init(repo->index, repo);
    - 
    - 	/*
    - 	 * Unfortunately, we need to keep this hack around for the time being:
    - 	 *
    - 	 *   - Not setting up the hash algorithm for `the_repository` leads to
    - 	 *     crashes because `the_hash_algo` is a macro that expands to
    - 	 *     `the_repository->hash_algo`. So if Git commands try to access
    - 	 *     `the_hash_algo` without a Git directory we crash.
    - 	 *
    - 	 *   - Setting up the hash algorithm to be SHA1 by default breaks other
    - 	 *     commands when running with SHA256.
    - 	 *
    - 	 * This is another point in case why having global state is a bad idea.
    - 	 * Eventually, we should remove this hack and stop setting the hash
    - 	 * algorithm in this function altogether. Instead, it should only ever
    - 	 * be set via our repository setup procedures. But that requires more
    - 	 * work.
    - 	 */
    - 	if (repo == the_repository)
    - 		repo_set_hash_algo(repo, GIT_HASH_SHA1);
      }
      
      static void expand_base_dir(char **out, const char *in,

I'll maybe just revise the series and pull both of these topics in as a
dependency.

> * ps/ci-test-with-jgit (2024-04-12) 13 commits
>   (merged to 'next' on 2024-05-01 at 35e293e618)
>  + t0612: add tests to exercise Git/JGit reftable compatibility
>  + t0610: fix non-portable variable assignment
>  + t06xx: always execute backend-specific tests
>  + ci: install JGit dependency
>  + ci: make Perforce binaries executable for all users
>  + ci: merge scripts which install dependencies
>  + ci: fix setup of custom path for GitLab CI
>  + ci: merge custom PATH directories
>  + ci: convert "install-dependencies.sh" to use "/bin/sh"
>  + ci: drop duplicate package installation for "linux-gcc-default"
>  + ci: skip sudo when we are already root
>  + ci: expose distro name in dockerized GitHub jobs
>  + ci: rename "runs_on_pool" to "distro"
> 
>  Tests to ensure interoperability between reftable written by jgit
>  and our code have been added and enabled in CI.
> 
>  Will merge to 'master'.
>  source: <[email-masked]>

I have sent a follow-up here [2] that unbreaks the "ubuntu:latest" jobs.
The breakage is caused by the release of Ubuntu 24.04, where there is no
more Python 2 anymore.

[2]: https://lore.kernel.org/git/[email-masked]/T/#u

Patrick