Skip to content

Uses correct counts in rebase command condition/messaging #3780

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Uses correct counts in rebase command condition/messaging #3780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72ee61d
Uses correct counts in rebase condition/messaging
axosoft-ramint Nov 19, 2024
991e248
Updates placeholder wording
axosoft-ramint Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 30 additions & 29 deletions src/commands/git/rebase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ interface Context {
repos: Repository[];
associatedView: ViewsWithRepositoryFolders;
cache: Map<string, Promise<GitLog | undefined>>;
destination: GitBranch;
branch: GitBranch;
pickCommit: boolean;
pickCommitForItem: boolean;
selectedBranchOrTag: GitReference | undefined;
Expand All @@ -40,7 +40,7 @@ type Flags = '--interactive';

interface State {
repo: string | Repository;
reference: GitReference;
destination: GitReference;
flags: Flags[];
}

Expand All @@ -63,7 +63,7 @@ export class RebaseGitCommand extends QuickCommand<State> {
counter++;
}

if (args?.state?.reference != null) {
if (args?.state?.destination != null) {
counter++;
}

Expand All @@ -87,15 +87,15 @@ export class RebaseGitCommand extends QuickCommand<State> {
configs = ['-c', `"sequence.editor=${editor}"`];
}

state.repo.rebase(configs, ...state.flags, state.reference.ref);
state.repo.rebase(configs, ...state.flags, state.destination.ref);
}

protected async *steps(state: PartialStepState<State>): StepGenerator {
const context: Context = {
repos: this.container.git.openRepositories,
associatedView: this.container.views.commits,
cache: new Map<string, Promise<GitLog | undefined>>(),
destination: undefined!,
branch: undefined!,
pickCommit: false,
pickCommitForItem: false,
selectedBranchOrTag: undefined,
Expand Down Expand Up @@ -130,29 +130,29 @@ export class RebaseGitCommand extends QuickCommand<State> {
}
}

if (context.destination == null) {
if (context.branch == null) {
const branch = await state.repo.git.getBranch();
if (branch == null) break;

context.destination = branch;
context.branch = branch;
}

context.title = `${this.title} ${getReferenceLabel(context.destination, {
context.title = `${this.title} ${getReferenceLabel(context.branch, {
icon: false,
label: false,
})} onto`;
context.pickCommitForItem = false;

if (state.counter < 2 || state.reference == null) {
if (state.counter < 2 || state.destination == null) {
const pickCommitToggle = new PickCommitToggleQuickInputButton(context.pickCommit, context, () => {
context.pickCommit = !context.pickCommit;
pickCommitToggle.on = context.pickCommit;
});

const result: StepResult<GitReference> = yield* pickBranchOrTagStep(state as RebaseStepState, context, {
placeholder: context => `Choose a branch${context.showTags ? ' or tag' : ''} to rebase`,
placeholder: context => `Choose a branch${context.showTags ? ' or tag' : ''} to rebase onto`,
picked: context.selectedBranchOrTag?.ref,
value: context.selectedBranchOrTag == null ? state.reference?.ref : undefined,
value: context.selectedBranchOrTag == null ? state.destination?.ref : undefined,
additionalButtons: [pickCommitToggle],
});
if (result === StepResultBreak) {
Expand All @@ -164,18 +164,18 @@ export class RebaseGitCommand extends QuickCommand<State> {
continue;
}

state.reference = result;
state.destination = result;
context.selectedBranchOrTag = undefined;
}

if (!isRevisionReference(state.reference)) {
context.selectedBranchOrTag = state.reference;
if (!isRevisionReference(state.destination)) {
context.selectedBranchOrTag = state.destination;
}

if (
state.counter < 3 &&
context.selectedBranchOrTag != null &&
(context.pickCommit || context.pickCommitForItem || state.reference.ref === context.destination.ref)
(context.pickCommit || context.pickCommitForItem || state.destination.ref === context.branch.ref)
) {
const ref = context.selectedBranchOrTag.ref;

Expand All @@ -194,14 +194,14 @@ export class RebaseGitCommand extends QuickCommand<State> {
? `No commits found on ${getReferenceLabel(context.selectedBranchOrTag, {
icon: false,
})}`
: `Choose a commit to rebase ${getReferenceLabel(context.destination, {
: `Choose a commit to rebase ${getReferenceLabel(context.branch, {
icon: false,
})} onto`,
picked: state.reference?.ref,
picked: state.destination?.ref,
});
if (result === StepResultBreak) continue;

state.reference = result;
state.destination = result;
}

const result = yield* this.confirmStep(state as RebaseStepState, context);
Expand All @@ -219,24 +219,25 @@ export class RebaseGitCommand extends QuickCommand<State> {
private async *confirmStep(state: RebaseStepState, context: Context): AsyncStepResultGenerator<Flags[]> {
const counts = await this.container.git.getLeftRightCommitCount(
state.repo.path,
createRevisionRange(context.destination.ref, state.reference.ref, '...'),
createRevisionRange(state.destination.ref, context.branch.ref, '...'),
{ excludeMerges: true },
);

const title = `${context.title} ${getReferenceLabel(state.reference, { icon: false, label: false })}`;
const count = counts != null ? counts.left : 0;
if (count === 0) {
const title = `${context.title} ${getReferenceLabel(state.destination, { icon: false, label: false })}`;
const ahead = counts != null ? counts.right : 0;
const behind = counts != null ? counts.left : 0;
Comment on lines +222 to +228
Copy link
Contributor Author

@axosoft-ramint axosoft-ramint Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The core of the bug lies in these lines. The ref that is passed into the rebase command via state is the destination of the rebase, not the source. The rebase command is rebasing the current branch (previously housed in context.destination, renamed now to context.branch onto the chosen branch (previously housed in state.reference, renamed here to state.destination).

When we get the counts in container.git.getLeftRightCommitCount, we consistently put the "destination" on the left of the git revision range. In this case, because of the mislabeling, we were putting the source (current branch) on the left side and calling it the "destination", and the actual destination (chosen branch) is on the right side. So counts.left was giving us "how many commits ahead the current branch is" when we really wanted to know "how many commits behind the current branch is". In the messaging further down though, we say "will rebase by applying X commits on top of". Here, we want to know "how many commits ahead the current branch is".

So I split the counts into clearly labeled ahead and behind variables, put the "destination" and "source" into the correct slots of the revision range, and renamed everywhere so that it's clear that the destination is the argument provided to the command in the state/initial state, and the source is the current branch that we load into the context.

if (behind === 0) {
const step: QuickPickStep<DirectiveQuickPickItem> = this.createConfirmStep(
appendReposToTitle(title, state, context),
[],
createDirectiveQuickPickItem(Directive.Cancel, true, {
label: 'OK',
detail: `${getReferenceLabel(context.destination, {
detail: `${getReferenceLabel(context.branch, {
capitalize: true,
})} is already up to date with ${getReferenceLabel(state.reference, { label: false })}`,
})} is already up to date with ${getReferenceLabel(state.destination, { label: false })}`,
}),
{
placeholder: `Nothing to rebase; ${getReferenceLabel(context.destination, {
placeholder: `Nothing to rebase; ${getReferenceLabel(context.branch, {
label: false,
icon: false,
})} is already up to date`,
Expand All @@ -252,18 +253,18 @@ export class RebaseGitCommand extends QuickCommand<State> {
[
createFlagsQuickPickItem<Flags>(state.flags, [], {
label: this.title,
detail: `Will update ${getReferenceLabel(context.destination, {
detail: `Will update ${getReferenceLabel(context.branch, {
label: false,
})} by applying ${pluralize('commit', count)} on top of ${getReferenceLabel(state.reference, {
})} by applying ${pluralize('commit', ahead)} on top of ${getReferenceLabel(state.destination, {
label: false,
})}`,
}),
createFlagsQuickPickItem<Flags>(state.flags, ['--interactive'], {
label: `Interactive ${this.title}`,
description: '--interactive',
detail: `Will interactively update ${getReferenceLabel(context.destination, {
detail: `Will interactively update ${getReferenceLabel(context.branch, {
label: false,
})} by applying ${pluralize('commit', count)} on top of ${getReferenceLabel(state.reference, {
})} by applying ${pluralize('commit', ahead)} on top of ${getReferenceLabel(state.destination, {
label: false,
})}`,
}),
Expand Down
2 changes: 1 addition & 1 deletion src/commands/quickCommand.steps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2148,7 +2148,7 @@ async function getShowCommitOrStashStepItems<
command: 'rebase',
state: {
repo: state.repo,
reference: state.reference,
destination: state.reference,
},
}),
new GitWizardQuickPickItem('Switch to Commit...', {
Expand Down
2 changes: 1 addition & 1 deletion src/git/actions/repository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export function push(repos?: string | string[] | Repository | Repository[], forc
export function rebase(repo?: string | Repository, ref?: GitReference, interactive: boolean = true) {
return executeGitCommand({
command: 'rebase',
state: { repo: repo, reference: ref, flags: interactive ? ['--interactive'] : [] },
state: { repo: repo, destination: ref, flags: interactive ? ['--interactive'] : [] },
});
}

Expand Down
Loading