cristibalan / braid

Simple tool to help track vendor branches in a Git repository.
http://cristibalan.github.io/braid
MIT License
457 stars 54 forks source link

Ability to specify a commit message for a non-interactive `braid push` #112

Open udeved opened 2 years ago

udeved commented 2 years ago

We have recently migrated to braid from git-subrepo, works really great. Perfect for our use case, we got 5000+ package repos all added to one controlling braid repo. https://gitea.artixlinux.org/pkgbuild/main

Anyhow, we slightly patched the braid, so that the commit message is passed from the local head to the push of a given added repo. The commit editor is thus not popping up. I am not sure you want this patch upstream, but I could open a PR any time if there is interest. https://github.com/artix-linux/braid/tree/no-editor

mattmccutchen commented 2 years ago

I'm glad you've found Braid useful! We don't have a good way right now to collect data about who is using Braid and how, so these bits of information in issue reports are nice.

I don't want to change the behavior of braid push unconditionally. While I'm not sure of the rationale for the current behavior, users may be used to it by now and may be surprised by a change. I'm also not convinced that your branch's current behavior of copying the commit message from the most recent downstream commit would be useful to enough users that we'd want to add a dedicated flag to Braid for it. (The most recent downstream commit may not have touched the mirror at all. A better default might be the concatenation of the messages of all commits that changed the mirror since the last time it had no diff from upstream, though that might be expensive to compute.) An approach we would probably accept is to have braid push accept arbitrary extra flags to pass through to git commit, like braid diff does for its underlying git diff. (With this kind of design, there's always a risk that passing through certain flags interacts poorly with other things Braid is doing. My experience suggests that users can deal with this and it isn't important for us to proactively restrict the flags, though of course, we can block a specific flag later if we learn that it commonly trips users up.) Then you could write a wrapper script to generate the commit message you want and pass it via the -m or -F option.

Another issue: I don't think we want to remove the "Braid: " from the commit messages automatically generated by commands such as braid add, so that behavior would have to be made conditional in the git.commit method.

I leave it to you whether you're interested in doing the work to upstream the feature now. Note that it would also require a simple test case. Or we can wait and see if there's evidence of demand from more users.

udeved commented 2 years ago

You are absolutely right, a better approach would be to do this with a flag, or even a dedicated braid commit command? There are some flaws with the patch as a general use case, we have the braid repo rebase on pull, and our wrapper script that actually commits and pushes already generates the message. If no rebase is set, the patch would likely produce wrong commit messages. The patch just passes that message to have sort of automated pushes without constantly entering the commit message. That "braid:" removal from the message is really tailored to our use case.

Another general flaw, not realted to the patch is that what gets pushed in the subrepo is kind of like a sqash commit, it "forgets" previous commits. That's ok for our purposes though, but more generally, there should probably another flag to pass multiple commits?

Btw, there is also a patch to apply to braid's ruby-main dependency, which complains about the finalizers. We think we fixed it here. https://gitea.artixlinux.org/aldum/ruby-main/commit/c2c5a3d54897e949aeb4ad8391a03fea5e1bf09b

Not sure I got the time to work on a more general solution to commit messages in braid. But if I do, I'll let you know.

mattmccutchen commented 2 years ago

Another general flaw, not realted to the patch is that what gets pushed in the subrepo is kind of like a sqash commit, it "forgets" previous commits. That's ok for our purposes though, but more generally, there should probably another flag to pass multiple commits?

Yes, that would probably be nice to have, but it's a bigger project to figure out the right semantics and a reasonable implementation. Feel free to file a separate issue and we can see if there's enough demand to justify the work. In the meantime, users for whom this is a major problem might be better served by Git submodules (or for that matter, by maintaining a fork repo of the subproject as they would when using a Git submodule, but then bringing that fork repo into the superproject using Braid instead of a Git submodule).

Btw, there is also a patch to apply to braid's ruby-main dependency, which complains about the finalizers. We think we fixed it here. https://gitea.artixlinux.org/aldum/ruby-main/commit/c2c5a3d54897e949aeb4ad8391a03fea5e1bf09b

Yes, main seems undermaintained and we have other issues with it too, such as the lack of an option to treat braid FOO help as a normal command invocation with the single argument help rather than showing the help (see #89). We probably want to evaluate whether it's more feasible to switch to a fork with all of these issues patched or switch to a different command line parser, but that's another bigger project. This would also merit a separate issue; for now, #89 will at least remind me of it. In the meantime, we're living with the warning about the finalizer.