tools/shellcheck: git log ${TRAVIS_COMMIT_RANGE/.../..}

[marc-hb]

$TRAVIS_COMMIT_RANGE contains triple dots,
e.g. "origin/master...HEAD". This is great for git diff but unsuitable
for git log. Convert any triple dots to double dots before passing to
git log (don't change the argument for git diff --name-only).

Also use a more verbose git log command and --no-pager instead of the
gross '|cat ': the sanity check was reporting issues but always
reporting success.

Fixes: faadb20 ("CI: travis: new shellcheck-gitrange.bash")

Signed-off-by: Marc Herbert marc.herbert@intel.com

[marc-hb]

it's not that simple, need more time.

[ranj063]

@marc-hb I dont understand why you need ... for git diff and .. for git log. I'd think that in both cases .. should suffice. The difference between the 2 as I understand is whether the starting commit SHA1 is included or not.

[marc-hb]

The difference between the 2 as I understand is whether the starting commit SHA1 is included or not.

Nope.

https://matthew-brett.github.io/pydagogue/pain_in_dots.html#pain-in-dots

The triple dots is needed for git diff because it makes G...C below start from E, which is great. We want F and G to be excluded and not pollute the output of git diff --name-only

        A---B---C pull request
       /
--D---E---F---G master

That's why $TRAVIS_COMMIT_RANGE contains a triple dot and why this PR doesn't change anything to the git diff --name-only command.

Could/should $TRAVIS_COMMIT_RANGE provide a more direct double-dot E..C instead? Maybe but more work for Travis when git's triple dot provides this "for free".

This PR removes the triple dot for git log because the triple dot has a totally different (and almost "opposite"?!) meaning for git log! Thank you git...

[ranj063]

The difference between the 2 as I understand is whether the starting commit SHA1 is included or not.

Nope.

https://matthew-brett.github.io/pydagogue/pain_in_dots.html#pain-in-dots

The triple dots is needed for git diff because it makes G...C below start from E, which is great. We want F and G to be excluded and not pollute the output of git diff --name-only

        A---B---C pull request
       /
--D---E---F---G master

That's why $TRAVIS_COMMIT_RANGE contains a triple dot and why this PR doesn't change anything to the git diff --name-only command.

Could/should $TRAVIS_COMMIT_RANGE provide a more direct double-dot E..C instead? Maybe but more work for Travis when git's triple dot provides this "for free".

This PR removes the triple dot for git log because the triple dot has a totally different (and almost "opposite"?!) meaning for git log! Thank you git...

@marc-hb I am probably missing the bigger picture of how this range gets used. What are we doing with the diff and log?

[marc-hb]

@marc-hb I am probably missing the bigger picture of how this range gets used. What are we doing with the diff and log?

Extracting and showing filenames and commits of the PR and only of the PR.

This PR removes the triple dot for git log because the triple dot has a totally different (and almost "opposite"?!) meaning for git log!

Opposite indeed. This is not just for TRAVIS_COMMIT_RANGE; a diff with double dots matches a log with triple dots and vice versa in pretty much every context:

          *---*---PR   (Pull Request)
         /
--*---base---*----upstream

     Double dots        |  Triple dots
       upstream..PR     |        upstream...PR
------------------------+--------------------------
diff   upstream  PR "V" |  base             PR "/" 
log    ^base     PR "/" | ^base  upstream   PR "V"

[ranj063]

Extracting and showing filenames and commits of the PR and only of the PR.

@marc-hb typically we do a rebase and merge for PR's isnt it? So, why not checkout the PR locally and rebase on top of master. Then a .. would suffice for both cases? Maybe Im oversimplifying

[marc-hb]

So, why not checkout the PR locally and rebase on top of master.

Ideally, CI would test every PR BOTH before and after rebase.

In practice testing only before rebase is much simpler and is what the SOF project does right now. This small logging change merely adjusts to this reality and to git's broken user interface.

[off topic]
upstream is a moving target so testing after rebase is more complex and can create confusion, especially when not "done right". For instance in a previous project I saw failures happening only on some specific hardware even though my PR was not hardware specific at all. It just happened that some other, totally unrelated and hardware specific change got merged right in the middle of my PR!

[ranj063]

Ideally, CI would test every PR BOTH before and after rebase.

In practice testing only before rebase is much simpler and is what the SOF project does right now. This small logging change merely adjusts to this reality and to git's broken user interface.

@marc-hb fair enough. Lets go ahead with the change.

[libinyang]

@marc-hb I'm not very clear on this part. We are using git log to check whether the argument $1 is set right or not? If so, why don't we use git diff directly here? This may be a little same with @ranj063's question.

[marc-hb]

This git log command shows which commits are being checked for logging purposes. It also aborts the script early if "$1" is not valid.

The git diff command below extracts all the names of the files (fname) modified by the PR

Those are two different operations that need different commands with a slightly different argument. I'm not sure I understand the question.

[kv2019i]

@libinyang I believe git log is still the correct command for doing the sanity check, so this seems correct.

[kv2019i]

Looks good to me!

[kv2019i]

@libinyang I believe git log is still the correct command for doing the sanity check, so this seems correct.

[marc-hb]

Thanks @kv2019i and @ranj063

This is merely tweaking the argument of a log command and I submitted this 2 weeks ago so I've taken the "risk" to merge this with only 1.5 approvals.