Re: [PATCH v3] sha1_file: pass empty buffer to index empty file
Jeff King p...@peff.net writes: Not related to your patch, but I've often wondered if we can just get rid of hold_lock_file_for_append. There's exactly one caller, and I think it is doing the wrong thing. It is add_to_alternates_file(), but shouldn't it probably read the existing lines to make sure it is not adding a duplicate? IOW, I think hold_lock_file_for_append is a fundamentally bad interface, because almost nobody truly wants to _just_ append. Yeah, I tend to agree. Perhaps I should throw it into the list of low hanging fruits (aka lmgtfy:git blame leftover bits) and see if anybody bites ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] sha1_file: pass empty buffer to index empty file
Jeff King p...@peff.net writes: Your revised patch 2 looks good to me. I think you could test it more reliably by simply adding a larger file, like: test-genrandom foo $((128 * 1024 + 1)) big echo 'big filter=epipe' .gitattributes git config filter.epipe.clean true git add big The worst case if you get the size of the pipe buffer too small is that the test will erroneously pass, but that is OK. As long as one person has a reasonable-sized buffer, they will complain to the list eventually. :) Yeah, I like it. It was lazy of me not to add a new test. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] sha1_file: pass empty buffer to index empty file
On Wed, May 20, 2015 at 10:25:41AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: Not related to your patch, but I've often wondered if we can just get rid of hold_lock_file_for_append. There's exactly one caller, and I think it is doing the wrong thing. It is add_to_alternates_file(), but shouldn't it probably read the existing lines to make sure it is not adding a duplicate? IOW, I think hold_lock_file_for_append is a fundamentally bad interface, because almost nobody truly wants to _just_ append. Yeah, I tend to agree. Perhaps I should throw it into the list of low hanging fruits (aka lmgtfy:git blame leftover bits) and see if anybody bites ;-) Good thinking. I think it is the right urgency and difficulty for that list. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] sha1_file: pass empty buffer to index empty file
On Tue, May 19, 2015 at 12:48:21PM -0700, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Yuck; please discard the previous one. write_in_full() side is also writing into that process, so we should do the same. OK, without these two, and with the true filter that does not read anything reinstated in the test script, t0021 used to die i=0; while sh t0021-conversion.sh; do i=$(( $i + 1 )); done after 150 iteration or so for me. With these two, it seems to go on without breaking (I bored after 1000 iterations), so I'd declare it good enough ;-) Your revised patch 2 looks good to me. I think you could test it more reliably by simply adding a larger file, like: test-genrandom foo $((128 * 1024 + 1)) big echo 'big filter=epipe' .gitattributes git config filter.epipe.clean true git add big The worst case if you get the size of the pipe buffer too small is that the test will erroneously pass, but that is OK. As long as one person has a reasonable-sized buffer, they will complain to the list eventually. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] sha1_file: pass empty buffer to index empty file
Jim Hill gjth...@gmail.com writes: +test_expect_success filter: smudge empty file ' + git config filter.empty-in-repo.clean true But this one is correct but tricky ;-) If the contents to be cleaned is small enough (i.e. the one-liner file used in this test) to fit in the pipe buffer and we feed the pipe before 'true' exits, we won't see any problem. Otherwise we may get SIGPIPE when we attempt to write to the 'true' (non-)filter, but because we explicitly ignore SIGPIPE, 'true' still is a black hole filter. cat /dev/null may have been a more naive and straight-forward way to write this black hole filter, but what you did is fine. + git config filter.empty-in-repo.smudge echo smudged cat + + echo empty-in-repo filter=empty-in-repo .gitattributes + echo dead data walking empty-in-repo + git add empty-in-repo + + echo smudged expected + git checkout-index --prefix=filtered- empty-in-repo + test_cmp expected filtered-empty-in-repo This is also correct but tricky. rm -f empty-in-repo git checkout empty-in-repo may have been more straight-forward, but this exercises the same codepath and perfectly fine. Will queue and let's merge this to 'next' soonish. Thanks. +' + test_done -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
