David Hendricks posted comments on this change.
Patch set 6:
(9 comments)
Patch Set #6, Line 13: - Remove djgpp-dos target (it is not different to other x-compilations).
I was just wondering, why I didn't nag earlier, you can keep
Moved the change to another patch: https://review.coreboot.org/20293
Patch Set #6, Line 15: anything else from the code dump when we export the respective variable.
Nit, line is too long.
Done
Patch Set #6, Line 26: f5dd7ce1
This sha will probably get lost. Linking to the mailing list
It's actually the hash from the stable branch: https://review.coreboot.org/cgit/flashrom.git/commit/?id=f5dd7ce11b65ffd6ead214b4b4cbe90f3eb110dd
I guess the mailing list is the definitive source for this kind of stuff though, so I'll link to https://mail.coreboot.org/pipermail/flashrom/2016-November/014877.html instead.
For newer patches we can use flashrom-gerrit (https://mail.coreboot.org/pipermail/flashrom-gerrit/) to cite the source of such patches.
Patch Set #6, Line 28: pushing to the review server.
Maybe also mention, that you split out the complex version
Done
Patch Set #6, Line 1: #!/bin/sh
We should probably either make this #!/bin/bash or remove local variables. That would be for another patch, though.
Patch Set #6, Line 59: and git-svn branches/tags
removed the bit about git-svn
local svn_base=$(git log --grep=git-svn-id -1 --format='%h')
if [ "$svn_base" != "" ] ; then
local diff_to_svn=$(git rev-list --count ${svn_base}..${r})
if [ "$diff_to_svn" -gt 0 ] ; then
r="$r-$diff_to_svn"
fi
fi
Looks like we don't have git-svn-id any more. A simple `git describe
Done. I also took the liberty of reducing the indent level of this function by exiting earlier if the file isn't tracked.
Patch Set #6, Line 168: sed 's/.*@/r/;s/[[:blank:]].*//')
Same here, maybe just drop the --upstream command?
SGTM. It is a bit of a UI change, but AFAICT --upstream just returns "unknown" now so it's useless and isn't used by anything anyway.
else
range="$remote_sha..$local_sha"
Nit, actually I don't like the `else` here, it's not directly
Yep, good point. Done.
To view, visit change 19206. To unsubscribe, visit settings.