> On Oct. 17, 2014, 12:54 a.m., Vinod Kone wrote: > > support/mesos_split.py, line 27 > > <https://reviews.apache.org/r/26825/diff/1/?file=723339#file723339line27> > > > > It's weird that this script talks about commits but takes a list of > > files. Why not have it take it a commit sha and let it convert to a list of > > files? > > Cody Maloney wrote: > The code is a lot simpler. > > To go from sha-1 back I would need to import subprocess, have it call git > then parse the output. Having bash + xargs just take care of that is a lot > simpler (And also matches the pre-commit style hook).
I agree about the simplicity. But the weird part for me is that it is violating abstractions. Somehow, the script knows that a list of files given to it belongs to a commit and thats what its error message says. Maybe the error meessage need not mention commit? But we still need to say it is the commit that is in violation so that we give a better error message to ppl using it. Hmm. Alternatively, do you think this script would be used for any reason other than to examine a commit? If not, I think it should take a commit. > On Oct. 17, 2014, 12:54 a.m., Vinod Kone wrote: > > support/mesos_split.py, line 59 > > <https://reviews.apache.org/r/26825/diff/1/?file=723339#file723339line59> > > > > No need to pull this out into a constant. Just print the string here. > > Cody Maloney wrote: > Theh indentation gets particularly odd because the lines in the error > message shouldn't be indented, so in the middle of a python block you end up > with text full unindented which is rather odd. Making a multi-line string > constant is the standard python way. i see. ok. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26825/#review57081 ----------------------------------------------------------- On Oct. 21, 2014, 9:20 p.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26825/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 9:20 p.m.) > > > Review request for mesos, Adam B and Vinod Kone. > > > Bugs: MESOS-1712 and MESOS-1932 > https://issues.apache.org/jira/browse/MESOS-1712 > https://issues.apache.org/jira/browse/MESOS-1932 > > > Repository: mesos-git > > > Description > ------- > > Adds a new script mesos_split.py which is run as a git precommit hook. It > will error if a commit is made which spans across multiple of the mesos > projects (mesos, stout, libprocess) > > Sample output: > ``` > ERROR: Commit spanning multiple projects > > Mesos rules state that a commit should only touch one mesos project (stout, > libprocess, mesos). > > Paths grouped by project: > mesos: > baz > libprocess: > 3rdparty/libprocess/asdf > stout: > 3rdparty/libprocess/3rdparty/stout/foobar > ``` > > > Diffs > ----- > > support/hooks/pre-commit f6910f852a51d64a3441f9c23e70cafc6f7de741 > support/mesos_split.py PRE-CREATION > > Diff: https://reviews.apache.org/r/26825/diff/ > > > Testing > ------- > > Git added files in various groupings of the subprojects. Tried committing, > resulting in the sample error message above. > > > Thanks, > > Cody Maloney > >
