On Tue, May 5, 2020 at 11:47 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Sibi Siddharthan <sibisiddharthan.github@xxxxxxxxx> writes: > > > I have made the following changes as suggested: > > 1) The CMake script works from contrib/cmake now. > > 2) The CMake script parses the Makefile for > > SCRIPT_SH > > SCRIPT_PERL > > TEST_BUILTINS_OBJS > > LIB_OBJS > > BUILTIN_OBJS > > XDIFF_OBJS > > VCSSVN_OBJS > > 3) Philip suggested to change the error message if sh/bash was not > > found on windows. > > 4) CMake now tests for ICONV_OMITS_BOM, NO_ST_BLOCKS_IN_STRUCT_STAT. > > > > Regarding the commits, since 1,2,4 are additions to the script, is it > > acceptable for those changes to be staged in the next commits? > > I am not sure what you exactly mean by "to be staged in the next > commits". But as a new topic (from the point of view of the general > public), please avoid adding a known-broken one in patch 1 and then > following up with a series of "oops, that was wrong and here is to > fix the breakage" patches. > > On the other hand, if the final contents added by the resulting > topic is easier to explain and review if built incrementally in > logical steps, please do so. In other words, a series of follow up > "now we have basics there, teach the machinery to do this, too" > patches is perfectly fine, as opposed to "oops, that was wrong and > here is to fix". > 2 and 4 are additions to the capability of the script, which means they have to be added as separate commits (between 7/8 and 8/8). 3 is clearly an edit (fixup 6/8) I am confused for 1 though. I think it would be better to add it as a new commit (between 7/8 and 8/8). Is this acceptable? Thank You, Sibi Siddharthan > > Regarding the workflow file(main.yml), I modified the vs-build and > > test steps, should I drop the commit or should I keep the changes(a > > modification is further needed if CMake is going to be used for > > vs-build) > > It sounds like an integral part of the end result we would want to > get out of this series, so if that is the case, I do not see a > reason to exclude it. > > Thanks.
