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 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
Jeff King 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 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 Tue, May 19, 2015 at 12:48:21PM -0700, Junio C Hamano wrote: > Junio C Hamano 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 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
