Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2012, 19:22 UTC
Message-ID: <7vmx7921yz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120321173824.GA31490@odin.tremily.us>

"W. Trevor King" <wking@drexel.edu> writes:

Show 8 quoted lines

>> 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.