Change in flashrom[staging]: Convert flashrom to git

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/19206 )

Change subject: Convert flashrom to git
......................................................................


Patch Set 4:

(9 comments)

PS3 attempts to address some of your comments for the Makefile.

PS4 addresses another one of the comments in getrevision.sh, though there is still one
left unanswered. Perhaps it would be easiest for you to submit a patch if you have a
solution in mind already.

In any case, hopefully we're almost to the point where we can merge this into the
staging branch.

https://review.coreboot.org/#/c/19206/1/Makefile
File Makefile:

PS1, Line 1357: p || mv .featu
  This overwrites the default system-management heading
(center of
Not sure, perhaps that was unintentional. I reverted the format, but used
$(MAN_DATE) in place of the call to util/getrevision.sh.


PS1, Line 1369: 	mkdir -p $(DESTDIR)$(MANDIR)/man8
              : 	$(INSTALL) -m 0755 $(PROGRAM)$(EXEC_SUFFIX) $(DESTDIR)$(PREFIX)/sbin
              : 	$(INSTALL) -m 0644 $(PROGRAM).8 $(DESTDIR)$(MANDIR)/man8
              : 
  I'd rather not review a self-modifying Makefile...
Good thinking - done.


Line 1374: 	@rm -rf "$(EXPORTDIR)/flashrom-$(RELEASENAME)"
  Hmmm, fancy. One nit and one problem here:
Indeed, and I'm not even convinced this is a good idea since it conflates two
different concepts.

It seems this is done to mimic the behavior of "svn export".


PS1, Line 1377: akefile.version since 
  I think 'x;n;n;/^set$$/{b};x;p' would be more
clear.
Done


PS1, Line 1378: .
  This will ignore $0 which is the first argument after
the command for
Nice catch!


Line 1381: #	Restore modification date of all tracked files not marked
  ...could be simply fixed by adding a dummy after the
command like
Done


https://review.coreboot.org/#/c/19206/1/util/getrevision.sh
File util/getrevision.sh:

PS1, Line 257: 		update_upcache || { echo "offline" ; return ; }
             : 		up_refs=$upcache_refs
             : 	fi
             : 
             : 	# Find nearest commit contained in this branch that is also in any of the
up_refs, i.e. the branch point
             : 	# of the current branch. This might be the latest commit if it's in
any of the upstream branches.
             : 	loca
  Nit, looks like `staging` would override `stable` here
even if they
TBH, I'm not sure what to do about that here :-/


PS1, Line 268: 
  probably uninitialized
Done


PS1, Line 269: 
  always true on this path?
Yep, now it is!


-- 
To view, visit https://review.coreboot.org/19206
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2
Gerrit-PatchSet: 4
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com>
Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: david.hendricks(a)gmail.com
Gerrit-HasComments: Yes