From: Junio C Hamano Date: Wed, 21 Mar 2012 19:22:44 GMT Subject: Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb Message-ID: <7vmx7921yz.fsf@alter.siamese.dyndns.org> In-Reply-To: <20120321173824.GA31490@odin.tremily.us> "W. Trevor King" writes: >> I think it would be better to add initial tests with refactoring, and >> snapshot specific tests with snapshot support, e.g.: >> >> 1/2: gitweb: Refactor If-Modified-Since handling and add tests >> 2/2: gitweb: Add If-Modified-Since support for snapshots > > But the new tests would be for the new functionality (i.e. snapshot > support), so they wouldn't belong in the general refactoring commit. Then you are planning to split it in a wrong way. As I said, I do not think it matters that much for a small patch like this, but if the plan is to make the part to create i-m-s helper function as a standalone "refactoring" patch, then what Jakub outlined is the right way to go about it. In the first patch, you create i-m-s helper and update the existing code that can use the helper, without changing anything else. Do not touch snapshot code in this patch, if the current code does not support i-m-s in snapshot. And in the same patch, add tests for codepaths that use i-m-s to make sure your refactoring did not break them. In other words, if you remove the change to gitweb/ from the first patch and apply only the changes to the tests, the resulting new tests should pass with the current code that has i-m-s support inline without the i-m-s helper. And if you add back your change to gitweb/ for the refactoring, the test should still pass. And then in the second patch, you update the snapshot code and whatever else that can use i-m-s helper to support i-m-s. You can add tests to protect the new feature from future breakages in this patch.