Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values
Phillip Woodwrites: > Keeping the permission bits makes sense (I'd not thought of them when > I created the patch) as we want to check that the file has the correct > permissions. As for the all-zero object name, is it really worth > leaving it in - if a file has been created or deleted then we'll still > have /dev/null as the file name for one side or the other and the diff > lines will show it as well. As these tests are just to check the state > of the index then I'm not sure the hash values add anything. How do > you feel about a filter like > > sed "/^ index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/" Something like that, with perhaps the single 'x' replaced with something more normal looking and the all-zero thing special cased, was what I had in mind. Special casing all-zero matters. I know that the current code uses all-zero on the missing side. The thing is, tests are about protecting the correctness we currently have, and we want to catch the next idiot that breaks the code to stop showing all-zero when talking about the missing side.
Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values
On 20/02/18 17:39, Junio C Hamano wrote: Phillip Woodwrites: From: Phillip Wood Purge the index lines from diffs so we're not hard coding sha1 hash values in the expected output. The motivation of this patch is clear, but all-zero object name for missing side of deletion or creation patch should not change when we transition to any hash function. Neither the permission bits shown in the output (and whether the index line has the bits are shown on it in the first place, i.e. the index line of a creation patch does not, whilethe one in a modification patch does). So I am a bit ambivalent about this change. Perhaps have a filter that redacts, instead of removes, selected pieces of information that are likely to change while hash transition, and use that consistently to filter both the expected output and the actual output before comparing? Keeping the permission bits makes sense (I'd not thought of them when I created the patch) as we want to check that the file has the correct permissions. As for the all-zero object name, is it really worth leaving it in - if a file has been created or deleted then we'll still have /dev/null as the file name for one side or the other and the diff lines will show it as well. As these tests are just to check the state of the index then I'm not sure the hash values add anything. How do you feel about a filter like sed "/^index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/" Best Wishes Phillip
Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values
Phillip Woodwrites: > From: Phillip Wood > > Purge the index lines from diffs so we're not hard coding sha1 hash > values in the expected output. The motivation of this patch is clear, but all-zero object name for missing side of deletion or creation patch should not change when we transition to any hash function. Neither the permission bits shown in the output (and whether the index line has the bits are shown on it in the first place, i.e. the index line of a creation patch does not, whilethe one in a modification patch does). So I am a bit ambivalent about this change. Perhaps have a filter that redacts, instead of removes, selected pieces of information that are likely to change while hash transition, and use that consistently to filter both the expected output and the actual output before comparing?
Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values
On Mon, Feb 19, 2018 at 6:29 AM, Phillip Woodwrote: > Purge the index lines from diffs so we're not hard coding sha1 hash > values in the expected output. Perhaps the commit message could provide a bit more explanation about why this is a good idea. For instance, briefly mention that doing so will ease Git's transition from SHA1 to some newer hash function. > Signed-off-by: Phillip Wood