Check returns empty commit message in version 0.3.13

[erica-edholm]

Describe the bug
The command convco check started return empty commit message in version 0.3.13. When running the same command with version 0.3.12 the tool finds the commit, and validates it properly. I haven't found any documentation about any behavior change with the check command, hence this bug report.

To Reproduce
Steps to reproduce the behavior:

1. Clone convco repo: https://github.com/convco/convco.git
2. Ensure local convco version 0.3.13 with convco --version
3. Run convco check
4. See error empty commit message

Expected behavior
Expect output to be the same as with version 0.3.12, i.e:

FAIL  38be02d  wrong type: doc  doc: clarify cmake is needed to build
FAIL  8c4dbb6  wrong type: doc  doc: improve gitlab section in `README.m...
FAIL  ffcf34a  scope does not match regex: changelog|check|commit|version  ci(docker): Add a dockerfile
FAIL  724bb2b  first line doesn't match `<type>[optional scope]: <description>`  release v0.3.0
FAIL  155e652  scope does not match regex: changelog|check|commit|version  build(deps): bump versions

5/167 failed

System (please complete the following information):

• OS: ubuntu
• Version: 20.04

Additional context
Not sure if this is a bug or an actual change of behavior, couldn't find any changes in the documentation though, so I assume that it indeed a bug. If this behavior change is as intended, it would be good to update the documentation and then you can just close this bug.

[hdevalke]

There is a change in behavior. When stdin is not a terminal, it will read from stdin. see f53b02c.
I am on ubuntu 22.10, but can't reproduce your issue:

$ convco --version && convco check
convco 0.3.13
FAIL  38be02d  wrong type: doc  doc: clarify cmake is needed to build
FAIL  8c4dbb6  wrong type: doc  doc: improve gitlab section in `README.m...
FAIL  ffcf34a  scope does not match regex: changelog|check|commit|version  ci(docker): Add a dockerfile
FAIL  724bb2b  first line doesn't match `<type>[optional scope]: <description>`  release v0.3.0
FAIL  155e652  scope does not match regex: changelog|check|commit|version  build(deps): bump versions

5/167 failed

[hdevalke]

When running in docker there is indeed a change in behaviour. You have to add the flag -t or --tty

❯ docker run -t -u "$UID" -v "$PWD:/tmp" --workdir /tmp  convco/convco:0.3.13 check
FAIL  38be02d  wrong type: doc  doc: clarify cmake is needed to build
FAIL  8c4dbb6  wrong type: doc  doc: improve gitlab section in `README.m...
FAIL  ffcf34a  scope does not match regex: changelog|check|commit|version  ci(docker): Add a dockerfile
FAIL  724bb2b  first line doesn't match `<type>[optional scope]: <description>`  release v0.3.0
FAIL  155e652  scope does not match regex: changelog|check|commit|version  build(deps): bump versions

5/167 failed

❯ docker run -u "$UID" -v "$PWD:/tmp" --workdir /tmp  convco/convco:0.3.13 checkempty commit message

[hdevalke]

I see this issue is happening in github actions too as it seems not to open a tty.

https://github.com/convco/convco/actions/runs/3914150434/jobs/6690889112

[hdevalke]

One solution is to use shell: 'script -q -e -c "bash {0}"' in your github action:

      - name: Validate commit messages
        shell: 'script -q -e -c "bash {0}"'
        run: |
          git show-ref
          curl -sSfL https://github.com/convco/convco/releases/latest/download/convco-ubuntu.zip | zcat > convco
          chmod +x convco
          ./convco check ${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }}
          rm convco

I also added a check to validate if some rev is specified to check. if it is, it will not read from stdin.

[hdevalke]

@erica-edholm v0.3.14 has been released. Can you confirm the issue is solved. If you run convco version without arguments you might still see the issue. You could pass in HEAD to check all commits.

[ite-klass]

We are running convco check in Jenkins as bat steps. This regression broke our checks. It is an issue with v0.3.14 too.

---

I saw the point in the changelog, but did not expect my workflow/use to change and break.

The commit message describes it as

This is usefull to use convco in a commit-msg git hook.

I do not expect behavior change for existing use from that. The release notes did not warn about potential breakage either.

The commit message does not make it clear to me that there are differences, the intention, or alternatives are; it only broadly describes a new use case (piping into convco check).

---

Is there a description of how it is supposed to work now? Only stdin? Supposed to auto detect one or the other use, but that check is faulty/broken?

[hdevalke]

@ite-klass

As this is a pre-major release anything can change. I try to do my best to not introduce breaking changes, but this one was unexpected. I propose to pin to v0.3.12 for the time being.

https://semver.org/#spec-item-4
Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

As you run it with Jenkins, I guess no tty is available. There is still a bug I need to fix.
I should have taken more time to test this, but my spare time is sparse.

diff --git a/src/cmd/check.rs b/src/cmd/check.rs
index 53be1f3..73cc6d6 100644
--- a/src/cmd/check.rs
+++ b/src/cmd/check.rs
@@ -64,7 +64,7 @@ impl Command for CheckCommand {
 
         let Config { merges, .. } = config;
 
-        if self.rev.is_some() && !stdin().is_terminal() {
+        if self.rev.is_none() && !stdin().is_terminal() {
             let mut stdin = stdin().lock();
             let mut commit_msg = String::new();
             stdin.read_to_string(&mut commit_msg)?;

update: with this change you will have to set convco check HEAD if convco runs not in a tty.

[ite-klass]

Ok, we will stay on v0.3.12 for now.

Thank you for your efforts. I hope my comment did not come off too harshly. You've always been very responsive and worked through issues in a timely manner. You're right it's a 0 version, so I guess I should not expect consistent stability.

[hdevalke]

I hope my comment did not come off too harshly.

No it is fine. I understand breaking a workflow is not nice.
Thank you for always being quick to report issues.

[hdevalke]

I released v0.3.15 which should contain the fix to this issue.

[ite-klass]

v0.3.15

I would expect convco check --merges --max-count 30 to check the last 30 commits and to work by default.

If I have a commit range v1.2.3.. it works. Passing HEAD works too.

But without passing a rev, I still get the 'empty commit message' failure in Jenkins.

[hdevalke]

This is documented:

Arguments:
[REV] Start of the revwalk, can also be a commit range. Can be in the form <commit>..<commit>. If not provided and a tty it will check from HEAD. If not provided and not a tty it will check a single commit message from stdin

[ite-klass]

ok

I still find the behavior depending on the environment a questionable decision, and the resulting behavior confusing.

Writing down the default behavior without arguments

• changelog - unlimited
• check tty - HEAD / unlimited
• check not tty - single commit

I don't get why check would be wanted to behave different when not in a tty. If I were deciding on this I'd need very strong reasons for introducing this inconsistent behavior. With a reasonable alternative of a parameter for git hooks (#100 suggested --stdin) I would not design it like this.

But I can be explicit with the argument in my use to get consistent behavior across environments.

related note: tty may be terminology not familiar to users (I think it depends on users being familiar in the cli linux and server administration specifically?)

[dpc]

--stdin would work with me

[hdevalke]

commitlint has the same behaviour:

commitlint
[input] reads from stdin if --edit, --env, --from and --to are omitted

[dpc]

I think the tty vs notty is the only undesirable (fragile) part. I have not played with it myself, so please excuse if I misunderstand the current behavior.

I authored slog the logging library for Rust and people were routinely complaining that tty vs no tty removed colors from the stderr formatting (people don't know what tty is). But that was just minor cosmetic issue. Here it's about the core behavior, so I would not make it depend on tty.

[ite-klass]

commitlint has the same behaviour:

commitlint
[input] reads from stdin if --edit, --env, --from and --to are omitted

that's only half "the same behavior"; it's not the same condition, it does not document anything about runtime environment (tty or not) as condition; only parameters

to my understanding, it specifically does not follow the behavior I am criticizing.

[erica-edholm]

@hdevalke sorry for being MIA for three weeks, I hadn't checked my github notifications 🙈

Thanks for the explanation. It was indeed inside GHA that we first encountered the issue, but also locally for some reason.
Unfortunately, I have gone on maternity leave so I no longer have access to the computer where I had that issue. On my personal computer (which runs arch) I can't reproduce the bug either 🤷‍♀️
I have asked a colleague to verify version 0.3.15 in the place where we encountered the issue, but with a short test of my own in GHA it looks like it still returns empty commit message with version 0.3.15 if you don't provide the HEAD parameter. However, it's easily resolved by adding the HEAD parameter as you suggested, and we will use that going forward as it's also more clear what the intention is :)

[erica-edholm]

Actually, after talking to my colleague I realized that the command that failed was convco check origin/main..HEAD, and not just convco check. Either way, I have verified that version 0.3.15 works in GHA with the following little PoC:

name: Pull Request Checks
on: pull_request
jobs:
  check:
    name: Pull Request Checks
    runs-on: ubuntu-latest
    steps:
      - name: Checkout code
        uses: actions/checkout@v3
        with:
          fetch-depth: 0
          ref: ${{ github.event.pull_request.head.sha }}
      - name: Download and install convco
        run: |
          curl -OL https://github.com/convco/convco/releases/download/v0.3.15/convco-deb.zip
          unzip convco-deb.zip
          sudo dpkg -i convco*.deb
      - name: Check conventional commits
        run: convco check origin/main..HEAD

Thank you again for the quick response (even if I was so slow getting back to you, sorry about that!)

[hdevalke]

No problem, thank you for reporting the issue. #116 will introduce the --stdin option instead of the detection of tty or not.