Fix: a bug that causes an error when pushing without setting git remote

[matscube]

Description

If git remote is not set, oco needs to skip the git push process regardless the OCO_GITPUSH setting.

Fix

• When git remote does not exist, return early.
• Add E2E tests related to git remote (each pattern: no remote / only one remote / two remotes)

Error capture

[di-sukharev]

hi, @matscube are you sure there is a bug? I've never seen it. When there is no remotes.length it runs git push without asking to select a remote

[matscube]

hi, @di-sukharev
I'm not sure if this can be called a bug, but I thought there was an unnatural behavior.
In a git repository where no remotes are set, users cannot do git push itself, and if users try, the error message fatal: No configured push destination. is displayed.

Even if the remote is not set, the oco command tries to execute git push and an error occurs.
Is it necessary to intentionally cause a git error when the remote is not set?

The error can be reproduced by the following operations.

• OCO_GITPUSH is not set in the ~/.opencommit file
• $ mkdir git-test && cd git-test && git init
• $ touch file && oco

[di-sukharev]

but if you set the remote, then this lines work well, no?

[matscube]

@di-sukharev

but if you set the remote, then this lines work well, no?

No, I don't think so. (Sorry if I was totally wrong)

When the remote is set, remotes.length is 1 (or >= 2 ), so this lines will not be executed.
The subsequent process will execute git push.

When the remote is not set, remotes.length is 0, so this lines are executed, but an error occurs when trying to git push without a remote.

/* dev branch code */

if (!remotes.length) {
  const { stdout } = await execa('git', ['push']);
  if (stdout) outro(stdout);
  process.exit(0);
}

if (remotes.length === 1 && config?.OCO_GITPUSH !== true) {
  // ...
  // execute git push
  // ...
} else {
  // ...
  // execute git push
  // ...
}

[di-sukharev]

sorry, i was referring to this lines:

if (!remotes.length) {
  const { stdout } = await execa('git', ['push']);
  if (stdout) outro(stdout);
  process.exit(0);
}

i see. let's merge and thank you very much 🙇

[di-sukharev]

please pull the latest dev, fix conflicts, i merge and it will be deployed with #397

[di-sukharev]

i now get what you mean, but i would keep this lines, especially the if (stdout) outro(stdout); log, because otherwise user won't see why push is not working bc of the silent return on line 100

[matscube]

@di-sukharev
If it's an intentional behavior to log an error when no remotes are set, I understand it.
I'll leave it as it is without fixing it.

I don't think the E2E tests related to git push will be wasted, so I'm thinking of converting this PR to a PR for E2E tests only. Is that okay?

[di-sukharev]

awesome idea, lets have the tests 100%

[di-sukharev]

please let me know when you are ready with this, no rush, have a good rest of the weekends ❤️

[matscube]

@di-sukharev

I have a question about OCO_GITPUSH.
I understand that when OCO_GITPUSH is set to false, oco command will exit without executing git push.

But when I checked the behavior with the latest version in the following 6 patterns of checking the number of git remote settings and the settings of OCO_GITPUSH (false/true), it behaves as follows.

• 1. Pattern where git remote is not set
  • 1.1 OCO_GITPUSH: false => an error is output when trying to execute git push (Might be a bug?)
  • 1.2 OCO_GITPUSH: true => an error is output when trying to execute git push (as previously confirmed, this is the expected behavior)
• 2. Pattern where only one git remote is set
  • 2.1 OCO_GITPUSH: false => Confirm whether to git push (yes/no) => (Select yes) => Execute git push (Might be a bug?)
  • 2.2 OCO_GITPUSH: true => Confirm which remote to push (but there is only one remote, so you have no choice but to select that remote) => (Select) => Execute git push
• 3. Pattern where two or more git remotes are set
  • 3.1 OCO_GITPUSH: false => Confirm which remote to push => (Select one) => Execute git push (Might be a bug?)
  • 3.2 OCO_GITPUSH: true => Confirm which remote to push => (Select one) => Execute git push

In the previous version, it returned early when OCO_GITPUSH was false, but it seems that the line was deleted in the recent refactoring commit.

8fe8e61#diff-ae7d70033d3bac66c3fb6f289e5de37ff2336967e2a9831ad9654214c12122b6L98-L102

Is this an intentional fix?
If it is a bug, I will fix it to return early as it was before, is that okay?

[di-sukharev]

i think you are 100% correct assuming 1.1, 2.1 and 3.1 are bugs, they are.. if you want to fix them — this PR is a good fit for it :)

[matscube]

@di-sukharev
I rewrote the E2E test including bug fixes.
Please review :)

Fixes

• Fixed typo in README
• Fixed a bug in OCO_GITPUSH
• Added E2E tests related to git push flow