Add --base option

[guludo]

Hi there!

This is particularly useful for when the local development branch has diverged from its remote counterpart due to a previous rebase. This was the main motivation for me to implement such a feature.

Best regards,
Gustavo Sousa

[keis]

Hey, thanks for the PR

I like the idea of this but I find a bit confusing to reason about what the revision range becomes. I think it should go something like this

• --base if specified
• @{upstream} if set
• detected base if autobase is enabled
• [full history]

Main difference being if there's an {upstream} that should take precedent, WDYT?

And either way lets try to get this logic grouped together because it's really about the same thing anyway.

[guludo]

Hello.

In my specific case I would like the opposite: the closest ancestor branch (i.e. autobase) to take precedence over @{upstream}. That said, I understand that it is not the most common case.

I'd like to suggest an alternative approach then. We get rid of --autobase and make --base be the only CLI option that defines the start of the revision range. As such, --base could be passed any revision accepted by git as well as a special string in the format %<i>, representing the HEAD's <i>-th closest ancestor branch (e.g.%1).

Then, the base of the revision range would decided in the following order:

1. The argument of --base if specified;
2. The value of the environment variable GITFIXUPBASE if present;
3. The value of the configuration key fixup.base if present;
4. @{upstream} if existing;
5. The root commit (i.e. full history).

What do you think?

[keis]

I like that, it is a priority order that's easy to understand and considering @{upstream} is a rev-parse'able string we can even just consider that to be the default of base unless anything else is specified by config/env/cli making the flow even simpler.

I don't quite follow how you think %n should work. %1 would be the same as what --autobase does in the PR now? When would you use %2 or higher? I'd like to avoid extended the already archaic rev-parse syntax if possible, perhaps it could just be spelled --base auto

[guludo]

%1 would be the same as what --autobase does in the PR now? When would you use %2 or higher?

Yes. We can extract <i> from such a pattern and use it in the command that gets the closest branch. I would use %2 in the case branchB -> .... -> branchA -> .... -> HEAD and I want branchB to be the base.

However, now that I think a bit more about it, it just seems to over-complicate things. I could just use the branch name for %2 or higher. So I like the idea of just using a symbol to refer to the closest branch, like you suggested. Although I started with the "auto" terminology, I don't quite like it because it seems a bit vague and open to interpretation hehe. What about --base closest?

[keis]

Alright, --base closest sounds good to me.

[guludo]

Hi there!

Sorry it took long. I just added a commit that changes the code to conform what we discussed. I think it might make sense to squash the two commits.

[keis]

LGTM, I'll merge with a squash

[mcepl]

I am meditating over this change and I wonder whether @{upstream} is the right one.

Let’s consider this situation:

flowchart TD
    current["staging"] --> A'@{ label: "A'" }
    A' --> B'@{ label: "B'" }
    B' --> C["C"]
    B["B"] --> C
    A["A (branch 'remote')"] --> B

    style A stroke:#00C853
    style B stroke:#00C853

Loading

git rev-parse '@{upstream}' gives remote or A. When considering commits, why should we consider any commit on the remote branch? In other words, shouldn’t closest be the default value of base option?

[keis]

The default should be something close to "everything on my branch" (So, 'C..staging' in your example)

The problem with closest is that it doesn't work well with stacked branches that gets updated by update-ref in the rebase. e.g if there was a local branch part of my patch set pointing to B' I would still like to get suggestions for fixups on those commits between B' and C

I see your point about using remote commits though. However, in practice (for my workflow), it ends up working because I am using stacked branches a lot and I'm always rebasing on origin/main

Perhaps it would be better to use the merge-base

[mcepl]

You are absolutely right about stacked branches. I will have to think about it, my point was just that @{upstream} is most likely not it. Perhaps aside from closest we need an equivalent of @{upstream}...HEAD?

[keis]

I'm not sure if git merge-base @{upstream} HEAD works right away but something to that effect should do as a better default