Always pass the `config` as an explicit constructor parameter by dscho · Pull Request #1993 · gitgitgadget/gitgitgadget

dscho · Sep 1, 2025

This is necessary so that the project config is reliably passed through, and the defaultConfig is not used by mistake.

This PR is stacked on #1994 and needs that to be merged first.

Implicit configuration is a recipe for confusion when trying to allow for overriding said configuration. Let's require the config to be passed down explicitly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

webstech · Sep 4, 2025

lib/project-options.ts

        baseCommit?: string,
    ): Promise<ProjectOptions> {
-        const config: IConfig = getConfig();
+        const config: IConfig = defaultConfig;


Should this be passed as a parameter?

Yes, of course!

(And sorry for making such a mistake, in my defense, I thought I was still in similar-looking test code, where we do want to use the defaultConfig for reproducible testing.)

No problem. Will this be going away?

Yes. Sorry, I forgot to force-push.

Will this be going away?

Sorry for not being clear. I was wondering if ProjectOptions might go away in the future. It was making GitGitGadget specific decisions but you have added config features that should handle the specific oddities of ggg. For a generic user, the ProjectOptions contains passed values and values from the config project section.

Oh yes, absolutely. The config handling should all be unified. I mentioned this elsewhere. (I forgot where. There are so many PRs by now. I'm so sorry!)

Definitely lots of opportunities for cleaning up after all that stuff is done, especially to remove the remnants of a CLI interface (GitGitGadget started out as my personal Bash script and for some time I tried to keep them in sync and then I tried to make GitGitGadget fulfill the role of the Bash script by having a compatible CLI interface. But that was a mistake and only lead to needlessly complex code.)

Implicit configuration is a recipe for confusion when trying to allow for overriding said configuration. Let's require the config to be passed down explicitly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho · Sep 5, 2025

Force-pushed.

Range-diff

1: a0244a6 ! 1: 39c86ed ProjectOptions: do not allow the config to be passed implicitly

@@ Commit message
 
     Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
 
+ ## lib/patch-series.ts ##
+@@ lib/patch-series.ts: export class PatchSeries {
+ 
+         const publishToRemote: string | undefined = undefined;
+ 
+-        const project = await ProjectOptions.get(workDir, headCommit, cc, basedOn, publishToRemote, baseCommit);
++        const project = await ProjectOptions.get(config, workDir, headCommit, cc, basedOn, publishToRemote, baseCommit);
+         if (rangeDiff) {
+             options.rangeDiff = rangeDiff;
+         }
+
  ## lib/project-options.ts ##
 @@
  import { commitExists, git, revParse } from "./git.js";
 -import { IConfig, getConfig, projectInfo } from "./project-config.js";
-+import defaultConfig from "./gitgitgadget-config.js";
 +import { IConfig, projectInfo } from "./project-config.js";
  
  // For now, only the Git, Cygwin and BusyBox projects are supported
  export class ProjectOptions {
+     public static async get(
++        config: IConfig,
+         workDir: string,
+         branchName: string,
+         cc: string[],
 @@ lib/project-options.ts: export class ProjectOptions {
          publishToRemote?: string,
          baseCommit?: string,
      ): Promise<ProjectOptions> {
 -        const config: IConfig = getConfig();
-+        const config: IConfig = defaultConfig;
          let upstreamBranch: string;
          let to: string;
          let midUrlPrefix = " Message-ID: ";
+
+ ## tests/project-options.test.ts ##
+@@ tests/project-options.test.ts: test("project options", async () => {
+     expect(await repo.commit("C")).not.toEqual("");
+ 
+     const options2 = await ProjectOptions.get(
++        defaultConfig,
+         repo.workDir,
+         "test-project-options",
+         ["Nguyễn Thái Ngọc Duy <pclouds@gmail.com>"],

…licit-config, 2025-09-08))

…itgadget/explicit-config, 2025-09-08))

GitGitGadget's GitHub Actions all implicitly use the [`defaultConfig`](https://github.com/gitgitgadget/gitgitgadget/blob/153badfc78153bb25d10e0e0d40c2cb3924d7f1b/lib/gitgitgadget-config.ts#L3-L45) that hard-codes the configuration of the OG GitGitGadget that supports the Git project (and the Git project _only_). However, for a long time there have been feature requests and musings towards using GitGitGadget also for other projects. GitGitGadget already _does_ have some beginnings of a support for other projects, e.g. the `IConfig` interface. With these changes, that interface gets extended a little, a validator is added that can verify whether or not an arbitrary object conforms to that interface, and then new inputs are introduced, accepting this (JSON-encoded) configuration as a user-provided string. The idea is that a project-specific fork of https://github.com/gitgitgadget/gitgitgadget-workflows/ contains this configuration in the `gitgitgadget-config.json` file in a dedicated `config` branch, from where it is synchronized via a GitHub workflow to the repository variable called `CONFIG`. This somewhat non-trivial setup allows the config to be conveniently tracked via Git, updated via Pull Requests, validated via GitHub workflows, and still to be used in a trivial manner in the main workflows via `${{ vars.CONFIG }}` (as opposed to having to play games with multi-branch checkouts or `curl`'ing a file from a different branch). This PR is step number 2 in that direction. Step number 1 was to register the [`cygwingitgadget` GitHub org](https://github.com/cygwingitgadget/) for experimenting with GitGitGadget support for a different mailing-list-based project than Git: Cygwin. That org is where I experimented with this change as well as with all the others leading up to gitgitgadget/gitgitgadget-github-app#7. This PR is stacked on top of #1993 and #1994

dscho added 4 commits September 2, 2025 01:17

GitGitGadget: do not allow the config to be passed implicitly

b291ab7

Implicit configuration is a recipe for confusion when trying to allow for overriding said configuration. Let's require the config to be passed down explicitly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

PatchSeries: do not allow the config to be passed implicitly

e8bb14c

Implicit configuration is a recipe for confusion when trying to allow for overriding said configuration. Let's require the config to be passed down explicitly. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho requested a review from webstech September 1, 2025 23:51

dscho self-assigned this Sep 1, 2025

This was referenced Sep 1, 2025

Actions: start accepting a config as input #1991

Merged

Require a project-specific config as repository variable #2002

Open

dscho marked this pull request as draft September 2, 2025 13:22

dscho changed the base branch from main to v1 September 3, 2025 18:10

dscho changed the base branch from v1 to main September 3, 2025 18:10

dscho marked this pull request as ready for review September 3, 2025 18:10

webstech reviewed Sep 4, 2025

View reviewed changes

dscho force-pushed the explicit-config branch from a0244a6 to 39c86ed Compare September 5, 2025 08:03

dscho enabled auto-merge September 5, 2025 08:06

webstech approved these changes Sep 8, 2025

View reviewed changes

dscho merged commit 272e1d1 into main Sep 8, 2025
6 checks passed

dscho deleted the explicit-config branch September 8, 2025 05:33

github-actions bot pushed a commit that referenced this pull request Sep 8, 2025

Sync v1 with (272e1d1 (Merge pull request #1993 from gitgitgadget/exp…

561e8f6

…licit-config, 2025-09-08))

webstech mentioned this pull request Sep 8, 2025

Cc: the current Git GUI maintainer on corresponding patches #1988

Merged

github-actions bot pushed a commit to Bassamx93/gitgitgadget that referenced this pull request Sep 8, 2025

Sync v1 with (272e1d1 (Merge pull request gitgitgadget#1993 from gitg…

6be8ee5

…itgadget/explicit-config, 2025-09-08))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always pass the `config` as an explicit constructor parameter#1993

Always pass the `config` as an explicit constructor parameter#1993
dscho merged 5 commits intomaingitgitgadget/gitgitgadget:mainfrom
explicit-configgitgitgadget/gitgitgadget:explicit-configCopy head branch name to clipboard

dscho commented Sep 1, 2025 •

edited

Loading

Uh oh!

webstech Sep 4, 2025

Uh oh!

dscho Sep 4, 2025

Uh oh!

dscho Sep 4, 2025

Uh oh!

webstech Sep 4, 2025

Uh oh!

dscho Sep 5, 2025

Uh oh!

webstech Sep 8, 2025

Uh oh!

dscho Sep 8, 2025

Uh oh!

dscho commented Sep 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Search code, repositories, users, issues, pull requests...

Conversation

dscho commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webstech Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

dscho Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

dscho Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

webstech Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

dscho Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

webstech Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

dscho Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

dscho commented Sep 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dscho commented Sep 1, 2025 •

edited

Loading