Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
dscho merged 2 commits intomaingitgitgadget/gitgitgadget:mainfrom
cc-current-git-gui-maintainergitgitgadget/gitgitgadget:cc-current-git-gui-maintainerCopy head branch name to clipboard
Aug 28, 2025
Merged

Cc: the current Git GUI maintainer on corresponding patches#1988
dscho merged 2 commits intomaingitgitgadget/gitgitgadget:mainfrom
cc-current-git-gui-maintainergitgitgadget/gitgitgadget:cc-current-git-gui-maintainerCopy head branch name to clipboard

Conversation

@dscho
Copy link
Member

@dscho dscho commented Aug 28, 2025

I just noticed that Pratyush was still Cc:ed by default. Then I looked and was puzzled because I thought that the project settings would be used instead of auto-magically figuring out whether the PR in question is a Git GUI one. But then I saw that the defaultConfig does not have a project attribute ;-)

dscho added 2 commits August 28, 2025 12:06
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Johannes Sixt gracefully accepted that job, and we need to Cc: them
instead of the previous maintainer.

I noticed this when Pratyush was Cc:ed in v2 of
gitgitgadget/git#358.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho requested review from mjcheetham and webstech August 28, 2025 11:02
@dscho dscho self-assigned this Aug 28, 2025
@webstech
Copy link
Contributor

This was probably mentioned somewhere as an outstanding issue. The cygwin, busybox and test suite can be handled by a config but I left them for compatibility until we got there. The handling of git-gui and upstream-branch is more problematic. Didn't really want to add functions to the config. Any thoughts on this? It may be a git specific problem.

@dscho
Copy link
Member Author

dscho commented Aug 28, 2025

We'll have to figure this out on the go, as we truly support more projects.

@dscho dscho merged commit 153badf into main Aug 28, 2025
6 checks passed
@dscho dscho deleted the cc-current-git-gui-maintainer branch August 28, 2025 18:37
github-actions bot pushed a commit that referenced this pull request Aug 28, 2025
…current-git-gui-maintainer, 2025-08-28))
github-actions bot pushed a commit to Bassamx93/gitgitgadget that referenced this pull request Aug 28, 2025
…itgadget/cc-current-git-gui-maintainer, 2025-08-28))
@dscho
Copy link
Member Author

dscho commented Sep 6, 2025

The handling of git-gui and upstream-branch is more problematic. Didn't really want to add functions to the config. Any thoughts on this? It may be a git specific problem.

Let's just leave this as-is, for now. If there is a project that wants to use GitGitGadget and that has "sub-lieutenants" that need to be Cc:ed on patches touching specific parts of the code, we can cross that bridge then.

And I wouldn't be surprised if it could be handled by a path pattern then, as opposed to the situation in Git which is hampered by its use of an ill-integrated subtree strategy (Junio merges Git GUI's changes, which are at the top-level tree, into git-gui/. But there are "filename overlaps" in those merge commits that Git should handle but does not: The Git project's staunch refusal to allow any sort of explicit way to help its often-wrong rename detection comes to full play here, where the git-gui pull commits pretend to completely rewrite, say, the top-level Makefile and to delete git-gui/Makefile when in reality it's more like the top-level Makefile gets replaced by a force-fully moved git-gui/Makefile. It is notoriously unintuitive to verify that those pull commits do not actually change git-gui's code.

In short, I could imagine that we will always have to special-case Git in that respect and it is probably not worth losing more brain cycles over this. My current thinking is to add the following to https://github.com/gitgitgadget-workflows/gitgitgadget-workflows/blob/config/gitgitgadget-config.json:

  "project": {
    "to": "--to=git@vger.kernel.org",
    "branch": "upstream/seen",
    "cc": [],
    "urlPrefix": "https://lore.kernel.org/git/"
  }

and make the project non-optional in IConfig and to modify ProjectOptions.get() to special-case Git-GUI like this, replacing all other project-specific code in that function:

let { branch: upstreamBranch, to, urlPrefix: midUrlPrefix } = config.project;

if ("gitgitgadget/git" === `${config.repo.owner}/${config.repo.name}`
    && (await revParse(`${baseCommit}:git-gui.sh`, workDir)) !== undefined) {
            // Git GUI
            cc.push("Johannes Sixt <j6t@kdbg.org>");
            upstreamBranch = "git-gui/master";
}

This will most likely need to live in the code eternally, even if we could come up with a more flexible way to configure this via the JSON, but... https://xkcd.com/1205/ is a real thing.

@webstech how does that plan sound to you?

@webstech
Copy link
Contributor

webstech commented Sep 8, 2025

@webstech how does that plan sound to you?

I mentioned in #1993 I thought you had introduced something for handling this but maybe it was in the sync remotes stuff. Sorry for any confusion.

Yes, making project required is a good thing.

Handling special cases is difficult. For git-gui the commit has a specific ancestor. Alternatively, it could be the path of any changed files are in a particular directory tree. These could be filters in the project.

Having said that, the special case will work for now and other projects may drive a generic solution.

Just to re-iterate, ProjectOptions is only used in PatchSeries so it may be the config can be used instead now (except for cc and upstreamBranch at the moment).

@dscho
Copy link
Member Author

dscho commented Sep 8, 2025

ProjectOptions is only used in PatchSeries so it may be the config can be used instead now (except for cc and upstreamBranch at the moment).

Right. And upstreamBranch is unlikely necessary in the Pull Request workflow (it was needed to emulate mail-patch-series.sh, back when I wanted to be able to use GitGitGadget also as a replacement for that script).

dscho added a commit to dscho/gitgitgadget that referenced this pull request Sep 9, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Sep 10, 2025

For the record, this is how I am intending to go forward with this: dscho@fb72cd6#diff-f15268abff24be0d82e67bb03d2d639789d27a2b734fdf93cf5e6c7fca61e44fL25-R31

@webstech
Copy link
Contributor

For the record, this is how I am intending to go forward with this: dscho@fb72cd6#diff-f15268abff24be0d82e67bb03d2d639789d27a2b734fdf93cf5e6c7fca61e44fL25-R31

Missing something?
cc.push("Johannes Sixt <j6t@kdbg.org>");

dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 3, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 6, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 7, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/gitgitgadget that referenced this pull request Oct 7, 2025
We're about to make the config required, and not implicit, therefore it
makes sense to start with the project options for `gitgitgadget/git`
itself.

So let's set `project` explicitly in `defaultConfig` and remove the
implicit configuration of the `gitgitgadget/git` project from
`ProjectOptions.get()`.

There is a slight quirk here, as the Git GUI special-casing is a bit
magic: based on the presence of a specific file at the top-level of
`HEAD`'s root tree, it is determined that this is a Git GUI PR rather
than a git/git PR.

Now, we _could_ introduce some new attribute of the `projectInfo` type
that would allow the same kind of trick for other projects. As per
gitgitgadget#1988 (comment)
though, no other project using GitGitGadget may ever need that, so we'll
still hard-code it for `gitgitgadget/git` (and `git/git`) specifically.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.