Re: ab/* topics
On Thu, Nov 01 2018, Duy Nguyen wrote: > On Thu, Nov 1, 2018 at 3:04 PM Junio C Hamano wrote: >> >> Ævar Arnfjörð Bjarmason writes: >> >> > Could you please pick up >> > https://public-inbox.org/git/20181024114725.3927-1-ava...@gmail.com/ ? >> > It seems to have fallen between the cracks and addressed the feedback on >> > v1, and looks good to me (and nobody's objected so far...). >> >> If this is the runtime-gettext-poison thing, this did not fall thru >> the cracks---I didn't get the impression that "no objection was a >> sign of being excellent"; rather I recall feeling that you were the >> only person who were excited about it, while everybody else was >> "Meh". > > I would be more excited if the scrambling patches go first (*) Sorry to be so unexciting :) I've sent a v3 in https://public-inbox.org/git/20181101193115.32681-1-ava...@gmail.com/T/#u which which hopefully addresses the concerns you & SZEDER had. > and then we start to make "make test" poisoned by default. Scrambled > output is still very readable and it will make people not forget about > grepping translated stuff the wrong way. Of course *i18n* functions in > the test suite need to be updated to to grep/compare for real, not > just exit early like they do now. I think now that this is a runtime option we'd instead just do stuff like: GIT_TEST_GETTEXT_POISON= git ... 2>err && grep str err Which has the advantages of: 1) You can grep for error messages you find in the code and find tests that check for them. 2) When you run the tests and something goes wrong, it's not some scrambled output, so you can quickly e.g. search for that error in the code / on Google without needing to hunt for some "how did I disable the scrambling again...?" knob. After all, the entire point of the facility is to catch us updating strings which have existing tests without "GIT_TEST_GETTEXT_POISON=" (or test_i18n...), but once we find those (with my patch) we can just selectively turn the whole thing off. > (*) The pseudolocalization idea is also good, but printing unicode by > default may be a bit of a stretch. Not everybody is running the test > suite with a unicode-capable terminal.
Re: ab/* topics
On Thu, Nov 1, 2018 at 3:04 PM Junio C Hamano wrote: > > Ævar Arnfjörð Bjarmason writes: > > > Could you please pick up > > https://public-inbox.org/git/20181024114725.3927-1-ava...@gmail.com/ ? > > It seems to have fallen between the cracks and addressed the feedback on > > v1, and looks good to me (and nobody's objected so far...). > > If this is the runtime-gettext-poison thing, this did not fall thru > the cracks---I didn't get the impression that "no objection was a > sign of being excellent"; rather I recall feeling that you were the > only person who were excited about it, while everybody else was > "Meh". I would be more excited if the scrambling patches go first (*) and then we start to make "make test" poisoned by default. Scrambled output is still very readable and it will make people not forget about grepping translated stuff the wrong way. Of course *i18n* functions in the test suite need to be updated to to grep/compare for real, not just exit early like they do now. (*) The pseudolocalization idea is also good, but printing unicode by default may be a bit of a stretch. Not everybody is running the test suite with a unicode-capable terminal. -- Duy
Re: ab/* topics (was: Re: What's cooking in git.git (Nov 2018, #01; Thu, 1))
On Thu, Nov 01, 2018 at 02:46:41PM +0100, Ævar Arnfjörð Bjarmason wrote: > > However, if you push that patch with 'sh-i18n--helper' as-is, then I > > do object now: parsing a boolean in shell is not at all that difficult > > to justify this new command. > > So instead of calling a helper (which is undocumented, and only used by > git itself internally) you'd instead like to have some shellscript > thingy like: > > if test $value = 'true' -o $value = '1' [] > then > exit 0 > elif test $value = 'false' -o $value = '0' [...] Yes, but more like: case "$GIT_TEST_GETTEXT_POISON" in yes|true|on|1) GIT_INTERNAL_GETTEXT_SH_SCHEME=poison ;; esac There is no need to check for 'false', 0, etc. Yes, I know that this is not as fully-fledged as git_env_bool(), e.g. it won't recognize 'TrUe' and '42' as true, but since this is "merely" a testing aid, I think it's sufficient. > Sure, if that's the consensus I can change that, but it seems like the > opposite of the direction we're moving with the general *.sh -> *.c > migration. I.e. implementing helpers whenever possible instead of adding > new shellscript-only logic. I doubt that we really want to implement helpers "whenever possible", i.e. even instead of such a rather trivial piece of shell code. In the discusson of v1 of that patch there were suggestions on how to turn an environment variable into a boolean in a more proper way, e.g. by extending 'git var', but I agree with Peff that "we should do nothing for now and see how often this comes up" [1]. In the meantime this builtin seems to be the worse direction of all. [1] https://public-inbox.org/git/20181025212358.ga23...@sigill.intra.peff.net/
Re: ab/* topics (was: Re: What's cooking in git.git (Nov 2018, #01; Thu, 1))
On Thu, Nov 01 2018, SZEDER Gábor wrote: > On Thu, Nov 01, 2018 at 12:02:01PM +0100, Ævar Arnfjörð Bjarmason wrote: >> Could you please pick up >> https://public-inbox.org/git/20181024114725.3927-1-ava...@gmail.com/ ? >> It seems to have fallen between the cracks and addressed the feedback on >> v1, and looks good to me (and nobody's objected so far...). > > I didn't object, because in > > https://public-inbox.org/git/87muqzllh0@evledraar.gmail.com/ > > you asked for "a more general review than just the problem of how > we turn an env variable into a boolean". > > However, if you push that patch with 'sh-i18n--helper' as-is, then I > do object now: parsing a boolean in shell is not at all that difficult > to justify this new command. So instead of calling a helper (which is undocumented, and only used by git itself internally) you'd instead like to have some shellscript thingy like: if test $value = 'true' -o $value = '1' [] then exit 0 elif test $value = 'false' -o $value = '0' [...] Sure, if that's the consensus I can change that, but it seems like the opposite of the direction we're moving with the general *.sh -> *.c migration. I.e. implementing helpers whenever possible instead of adding new shellscript-only logic.
Re: ab/* topics
Ævar Arnfjörð Bjarmason writes: > Could you please pick up > https://public-inbox.org/git/20181024114725.3927-1-ava...@gmail.com/ ? > It seems to have fallen between the cracks and addressed the feedback on > v1, and looks good to me (and nobody's objected so far...). If this is the runtime-gettext-poison thing, this did not fall thru the cracks---I didn't get the impression that "no objection was a sign of being excellent"; rather I recall feeling that you were the only person who were excited about it, while everybody else was "Meh". Thanks for pinging. It is very possible that I didn't read (or rememer) the thread correctly. Let me go back to the archive in the morning to double check.
Re: ab/* topics (was: Re: What's cooking in git.git (Nov 2018, #01; Thu, 1))
On Thu, Nov 01, 2018 at 12:02:01PM +0100, Ævar Arnfjörð Bjarmason wrote: > Could you please pick up > https://public-inbox.org/git/20181024114725.3927-1-ava...@gmail.com/ ? > It seems to have fallen between the cracks and addressed the feedback on > v1, and looks good to me (and nobody's objected so far...). I didn't object, because in https://public-inbox.org/git/87muqzllh0@evledraar.gmail.com/ you asked for "a more general review than just the problem of how we turn an env variable into a boolean". However, if you push that patch with 'sh-i18n--helper' as-is, then I do object now: parsing a boolean in shell is not at all that difficult to justify this new command.
ab/* topics (was: Re: What's cooking in git.git (Nov 2018, #01; Thu, 1))
On Thu, Nov 01 2018, Junio C Hamano wrote: > * ab/push-dwim-dst (2018-10-29) 9 commits > - SQUASH??? > - push doc: document the DWYM behavior pushing to unqualified > - push: add DWYM support for "git push refs/remotes/...:" > - push: test that doesn't DWYM if is unqualified > - push: add an advice on unqualified push > - push: move unqualified refname error into a function > - push: improve the error shown on unqualified push > - i18n: remote.c: mark error(...) messages for translation > - remote.c: add braces in anticipation of a follow-up change > > "git push $there $src:$dst" rejects when $dst is not a fully > qualified refname and not clear what the end user meant. The > codepath has been taught to give a clearer error message, and also > guess where the push should go by taking the type of the pushed > object into account (e.g. a tag object would want to go under > refs/tags/). > > The last few steps are questionable. > cf. <87in1lkw54@evledraar.gmail.com> Will send an update to this soon. > * ab/pack-tests-cleanup (2018-10-31) 3 commits > - index-pack tests: don't leave test repo dirty at end > - pack-objects tests: don't leave test .git corrupt at end > - pack-objects test: modernize style > > A couple of tests used to leave the repository in a state that is > deliberately corrupt, which have been corrected. > > Will merge to 'next'. Thanks! > * ab/reject-alias-loop (2018-10-19) 1 commit > (merged to 'next' on 2018-10-26 at bc213f1bef) > + alias: detect loops in mixed execution mode > > Two (or more) aliases that mutually refer to each other can form an > infinite loop; we now attempt to notice and stop. > > Discarded. > Reverted out of 'next'. > cf. <87sh0slvxm@evledraar.gmail.com> *nod* will try to find time to work on this soon, but treating it as non-urgent. Could you please pick up https://public-inbox.org/git/20181024114725.3927-1-ava...@gmail.com/ ? It seems to have fallen between the cracks and addressed the feedback on v1, and looks good to me (and nobody's objected so far...).
