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

Choose upstream revisions automatically #28

Closed
wants to merge 2 commits into from
Closed

Conversation

torbiak
Copy link
Owner

@torbiak torbiak commented Sep 8, 2023

I wrote tests for all the interesting or important cases I could think of:

  • the common case of just looking up @{upstream}
  • during interactive rebase
  • make sure we get fork-point when it's different from merge-base
  • ensure we can handle multiple upstreams, although I'd guess it's very rare that we'd need to. The only case I'm aware of is when there's a criss-cross merge and relevant reflog entries are expired.

I'm not confident that we fail nicely. One of my next tasks is to check the error-handling when running external commands.

Also, I'm not sure if my description in the --help would make sense to most git users: "If no revision is given and the current branch has a tracking branch, then C<@{upstream}> is used to find reasonable fork-points or merge-bases to use as upstream cutoffs."

It doesn't currently matter since all the tests set these env vars, but
I'd rather not leave HOME set to a dir that no longer exists after a
test is done.
If an upstream revision isn't given on the command-line, try to find
suitable ones automatically. This only works if the current branch has a
tracking branch.

Writing tests for this feature required refactoring the existing test
helpers significantly, and it made sense to put things in different
files and use packages other than main to make it obvious what's being
called. The stub module has to use the package that the module is
published as on CPAN, and we want to put the version in the script
(git-autofixup) as well and compare the two, so the script needs to be a
different package, which will only be used internally.

Closes #27
F<git-autofixup> parses hunks of changes in the working directory out of C<git diff> output and uses C<git blame> to assign those hunks to commits in C<E<lt>revisionE<gt>..HEAD>, which will typically represent a topic branch, and then creates fixup commits to be used with C<git rebase --interactive --autosquash>. It is assumed that hunks near changes that were previously committed to the topic branch are related.

C<@{upstream}> or C<@{u}> is likely a convenient value to use for C<E<lt>revisionE<gt>> if the current branch has a tracking branch. See C<git help revisions> for other ways to specify revisions.
F<git-autofixup> parses hunks of changes in the working directory out of C<git diff> output and uses C<git blame> to assign those hunks to commits in C<E<lt>revisionE<gt>..HEAD>, which will typically represent a topic branch, and then creates fixup commits to be used with C<git rebase --interactive --autosquash>. It is assumed that hunks near changes that were previously committed to the topic branch are related. If no revision is given and the current branch has a tracking branch, then C<@{upstream}> is used to find reasonable fork-points or merge-bases to use as upstream cutoffs. See C<git help revisions> for info about how to specify revisions.
Copy link
Collaborator

@krobelus krobelus Sep 9, 2023

Choose a reason for hiding this comment

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

If no revision is given and the current branch has a tracking branch, then C<@{upstream}> is used to find reasonable fork-points or merge-bases to use as upstream cutoffs. See C for info about how to specify revisions.

Sounds fine. It's unfortunate that Git has inconsistent terminology ("tracking branch" and "upstream"), not sure which one is better. Maybe we should use only upstream because it's in --set-upstream-to and @{upstream}.

Also we could rephrase as

If not specified, C<E<lt>revisionE<gt>> defaults to C<git merge-base
--forkpoint HEAD @{upstream} || git merge-base HEAD @{upstream}>. See C<git
help revisions> for info about how to specify revisions.

which is not entirely accurate but maybe easier to understand?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm, yeah, in the git-branch manpage they use "upstream branch" and "tracking branch" interchangably. Good point about upstream being more prevalent in the command-line interface, but previously I thought "tracking branch" was the canonical name for this concept in git and I'm not sure I would have known exactly what someone meant by "upstream branch". So now I'm thinking it might be good to use both, like "tracking/upstream branch".

Cool, I like describing the behaviour in terms of git commands. Very clear for people familiar with the commands, and for those that aren't it's obvious what to read to figure it out.

push @merge_bases, $_;
}
if (!@merge_bases) {
for (qx(git merge-base --all $upstream HEAD 2>/dev/null)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess here the ideal error output here would be something like

git merge-base: fatal: no upstream configured for branch 'my-branch'
Can't find tracking branch. Please specify a revision.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, for commands that are expected to fail it's probably worth prefixing their stderr lines with the command (as you showed) so we can pass errors through while making their provenance obvious.

@krobelus
Copy link
Collaborator

krobelus commented Sep 9, 2023

Thanks this is just great.

Related: I think in practice users often set a wrong upstream, for example when doing

$ git checkout default_rev
branch 'default_rev' set up to track 'origin/default_rev'.
Switched to a new branch 'default_rev'

If they want to rewrite history on this branch they should use git branch --set-upstream-to master.
I think many people don't know about this and work around it by passing explicit upstream arguments to git rebase -i etc.

There is branch.autoSetupMerge=inherit but I don't think it works for unique remote branches, because those don't have a tracking branch..

@torbiak
Copy link
Owner Author

torbiak commented Sep 14, 2023

Pushed using git directly, after making the discussed (and other) changes.

@torbiak torbiak closed this Sep 14, 2023
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