Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support explicit ref or commit (not just HEAD) #29

Closed
wants to merge 1 commit into from

Conversation

masklinn
Copy link
Contributor

Implicitly working with HEAD can be a chore when trying to use LC on a bare repo or "headless" (scripted).

Add support for an optional 2nd parameter, which defaults to HEAD but can be any ref or commit oid. If an oid, update-ref is skipped.

The skip heuristic is to show-ref the input, it'll fail on oids but should succeed on every resolveable ref (which we know ref_ is because we passed it to cat-file and it succeeded).

@not-an-aardvark
Copy link
Owner

I'm curious about the use case for running lucky_commit on a repo that doesn't have a HEAD ref. Could you elaborate a bit? I'm not entirely opposed, but I wonder if it would be better for esoteric use cases like this to use the Rust API rather than invoking the lucky_commit binary.

Copy link
Owner

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, but please fix the CI error (looks like it wants you to run cargo fmt)

@not-an-aardvark not-an-aardvark dismissed their stale review November 16, 2023 00:50

Accidentally left this comment on the wrong PR (meant to add it to #28)

@masklinn
Copy link
Contributor Author

I'm curious about the use case for running lucky_commit on a repo that doesn't have a HEAD ref. Could you elaborate a bit?

Basically for funsies I'd like to use lucky-commit in an integration tool, something like bors or github's merge queues. It does have local repositories but they're bare repos and it does all the work on raw oids, using git's plumbing.

Technically the repository does have a HEAD but that's just the upstream's default branch, it does not touch the HEAD because it doesn't have a working copy so that's not very useful for its job and moving the HEAD creates potential concurrency issues. Easier to work entirely with oids. Safely working with HEAD would require either having its own big lock around git operations, or using worktrees which is a lot of additional (and not really necessary) complexity.

I'm not entirely opposed, but I wonder if it would be better for esoteric use cases like this to use the Rust API rather than invoking the lucky_commit binary.

That is also an option, the integration tool is not in Rust but I could build a bespoke binary. However since I was already futzing in the source for #28 I figured I might as well see if you were interested in slightly extending the interface.

Implicitly working with `HEAD` can be a chore when trying to use LC on
a bare repo or "headless" (scripted).

Add support for an optional 2nd parameter, which defaults to `HEAD`
but can be any ref or commit oid. If an oid, `update-ref` is skipped.

The skip heuristic is to `show-ref` the input, it'll fail on oids but
should succeed on every resolveable ref (which we know `ref_` is
because we passed it to `cat-file` and it succeeded).
@not-an-aardvark
Copy link
Owner

Thanks for explaining.

Personally, I think it would be better to keep the CLI relatively simple, and I don't think most users of lucky-commit would benefit from this extra argument. So I'm going to close this PR.

Of course, you're welcome to use a fork that has the option, or to use the Rust API to generate the new commit data directly.

@masklinn masklinn deleted the non-head-support branch November 22, 2023 20:27
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