Page MenuHomeFreeBSD

newvers.sh: fix git false positive -dirty tag
ClosedPublic

Authored by emaste on Jun 22 2018, 4:36 PM.
Tags
None
Referenced Files
F103209525: D15968.id44311.diff
Fri, Nov 22, 6:35 AM
F103209508: D15968.id44314.diff
Fri, Nov 22, 6:35 AM
F103209485: D15968.id49959.diff
Fri, Nov 22, 6:35 AM
F103209480: D15968.id.diff
Fri, Nov 22, 6:34 AM
F103209478: D15968.id49957.diff
Fri, Nov 22, 6:34 AM
F103207680: D15968.diff
Fri, Nov 22, 6:16 AM
Unknown Object (File)
Wed, Nov 13, 10:17 PM
Unknown Object (File)
Oct 13 2024, 1:08 PM
Subscribers

Details

Summary

Assuming that any output from git diff-index --name-only implies changes in the working tree results in false positives: files with metadata, but not content, changes are also listed.

Check that content differences exist before adding the -dirty tag to the git hash.

PR: 229230

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste added a subscriber: jilles.
sys/conf/newvers.sh
93 ↗(On Diff #44311)

This will be $git_cmd not git.

Hrm, this only works if git diff is run at the top level dir; needs further investigation.

cd to the VCS topdir before invoking git diff

Can you use --shortstat instead of parsing sha's ?

In D15968#376629, @kib wrote:

Can you use --shortstat instead of parsing sha's ?

No, the user-facing commands have the false positive problem identified in the comment, and trying to use plain git diff has performance concerns.

sys/conf/newvers.sh
80 ↗(On Diff #44314)

Declare locals?

83 ↗(On Diff #44314)

"latter"

94 ↗(On Diff #44314)

IMO it would be a bit nicer to cache foo=$(realpath "$VCSTOP") (to ensure that it's an absolute path), and then git diff "${foo}/${file}". Then you don't need the subshell.

markj added inline comments.
sys/conf/newvers.sh
80 ↗(On Diff #44314)

There's also smode, dmode, ssha, dsha, status, file.

This revision is now accepted and ready to land.Nov 2 2018, 9:10 PM
emaste added inline comments.
sys/conf/newvers.sh
80 ↗(On Diff #44314)

The while loop has an implicit subshell and the loop variables themselves aren't visible outside so I think they're not needed.

This revision was automatically updated to reflect the committed changes.