Stefan Tauner posted comments on this change.
Patch set 11:
FWIW, these are my replies - mostly to Nico's excellent review - that also try to show rationales where my current patch diverges from what eventually landed in staging as of now. I still need to figure out how to integrate gerrit (and its hooks) into the scheme and will publish it then (with the intention to make it the first stable commit after 0.9.9).
(15 comments)
here are a few major differences:
- This uses coreboot's commit-msg hook which includes support for
generating and appending Change-Id.
- djgpp-dos target removal is moved to a
This seems way over-complicated with no visible use case. It also makes the
Of course it is over-complicated - it's what Carl-Daniel and I came up with when I convinced him to switch to git by fulfilling his requirements - namely having version strings with some kind of monotonicity that can easily be referred to by humans. Programmatically it is trivial to just use the git hash and derive some other string like that of git describe if that's needed by anybody. I don't see why this would be needed though?
The "visible use case" of the implemented scheme is simply... any log file posted that any developer needs to read?
Apart from the additional complexity of the get_revision script I don't really see any downsides. I put some substantial effort into making the high-quality transition to git possible, which includes this borderline-insane scheme to create somewhat monotonic version strings out of git without too much hassle for anybody. Of course I would like to keep it and there need to be better arguments then "meh this looks too complicated and I don't need it" to convince me otherwise.
Honestly, who cares? If somebody gets on the mailing list and presents this
It still gives a relatively good indication of what is contained in the code for those familiar with the commit history *without* checking a VCS.
This overwrites the default system-management heading (center of
Basically, because I think it is a good idea to give the user the version on the top instead of the rather useless manual section name. :)
And I would have never thought anybody would review this change in such detail so I simply slipped it in. If there is no objection I will simply note it in the commit message of the stable branch...
@ { $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX) >&2 && \
( echo "found."; echo "UTSNAME := yes" >> .features.tmp ) || \
( echo "not found."; echo "UTSNAME := no" >> .features.tmp ) } 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE)
@printf "Checking for clock_gettime support... " | tee -a
The makefile does not modify itself. It generates a modified version of itself in the export directory. :P But I see your point.
My implementation for your idea is slightly different to David's but essentially also creating and using a Makefile.version file.
Patch Set #1, Line 1377: t found."; echo "CLOCK
I think 'x;n;n;/^set$$/{b};x;p' would be more clear.
Is it? 'b' is documented in POSIX and sed's info pages as unconditional. In the case above this is kind of moot because there is a condition that guards the unconditional branch: the address using a regex to match the respective lines. Took me a while to grasp because I never really used addresses so far. When knowing this, the construct is nicer, true. I'll change it and also improve the comments.
One question: why the '{}' around 'b'?
...could be simply fixed by adding a dummy after the command like
Good catch! Thank you.
Why didn't I bikeshed this for being totally unrelated?
It should have been removed a very long time ago, and like other minor things I did not expect anybody to notice its removal in a review at all...
}
git_is_file_tracked() {
Do we break lines only at full stops now? This looks like a single
No, we break at the latest at a 112 character limit because that's what was agreed upon when the matter of line breaking was last discussed on the mailing list in 2012. In this case I broke it early but I cannot say why and this is indeed a bug. Possibly because breaking after every full stop is the style used when I write text documents (Latex etc).
Patch Set #1, Line 229: query_
So we never synchronize in the upstream-repo-registered case but
The idea behind this is simply to optimize performance for the common case. If the user uses upstream repos the code assumes that the user updates them in case some commits are merged upstream. It is rather improbable that a user continuously implements changes on top of a single (aging) upstream version (unless upstream is not updated for years but that's another story ;) without fetching any updates.
I have added this explanation to the source comment (the user output explains the updates of the upcache already and i dont want to complicate it ever further).
Nit, looks like `staging` would override `stable` here even if they
AFAICT the order is determined by that in 'upstream_branches' but I have not re-tested this now. Why do you think otherwise?
always true on this path?
No, there might be upstream branches that are neither stable nor staging (e.g. a release branch such as 0.9.9 for bug fixes). There is also an example in the commit message.
File util/git-hooks/install.sh:
Patch Set #1, Line 17: $(git
This assumption looks rather odd given the `git rev-parse` above.
No, IIRC, the idea is to make these relative links because else they get invalid when someone moves the checked out directory.
Patch Set #1, Line 1: #!/bin/sh
Since we probably want to avoid bashisms in general for git hooks, I got ri
What David(?) said...
Patch Set #1, Line 53: ommit $local_sha i
I reworded this to try and make it less confusing. LMK what you think.
Much clearer, thank you.
# Make _really_ sure we do not rewrite precious history
for lbranch in $precious_branches ; do
if [ "$remote_ref" = "refs/heads/$lbranch" ]; then
nonreachable=$(git rev-list $remote_sha ^$local_sha | head -1)
if [ -n "$nonreachable" ]; then
echo "Only fast-forward pushes are allowed on $lbranch." >&2
echo "$nonreachable is not included in $remote_sha while pushing to $remote_ref" >&2
exit 1
fi
fi
Isn't this configurable in the upstream repo?
The patch was written when I still hoped we could avoid Gerrit... and plain git (as used on flashrom.org/git) does not support that. I have not thought about how to deal with the two repositories in the hooks yet... just catching up with the changes to this patch atm.
To view, visit change 19206. To unsubscribe, visit settings.