On Fri, Mar 25, 2016 at 2:07 PM, Mehul Jain <[email protected]> wrote: > On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <[email protected]> > wrote: >> Nit: Some of the test titles spell this as "rebase.autostash" while >> others use "rebase.autoStash". >> [...] >> The title says that this is testing with rebase.autoStash unset, >> however, the test itself doesn't take any action to ensure that it is >> indeed unset. As with the two above tests which explicitly set >> rebase.autoStash, this test should explicitly unset rebase.autoStash >> to ensure consistent results even if some future change somehow >> pollutes the configuration globally. Therefore: >> [...] >> With the addition of these three new tests, aside from the >> introductory 'test_{un}config', this exact sequence of commands is now >> repeated four times in the script. Such repetition suggests that the >> common code should be moved to a function. For instance: > > I agree with all of these comments. I will introduce two new function to > reduce the code and the above mention loop. Also the work on Matthieu's > comment. > > I feel that most of your comments are necessary and should be there in > the next patch. But I have a doubt regarding the next patch. As Junio has > merged v10 of current series in next branch (as noticed from his mail), > sending a new patch should be based on the current patch (i.e. on next > branch) or master branch (i.e. continuing with this series)?
I hadn't noticed that v10 was already in 'next'. In this case, the suggested changes should be a new patch series which makes incremental changes to what is already in 'next'. Be sure to mention in the cover letter that the new series should be applied atop mj/pull-rebase-autostash. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
