On Fri, Mar 25, 2016 at 2:07 PM, Mehul Jain <mehul.jain2029@xxxxxxxxx> wrote:
> On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html