Re: pre-v2.34.0-rc0 regressions: 'git log' has a noisy iconv() warning (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

From: Jeff King <peff@peff.net>
Date: Oct 27, 2021, 11:14 UTC
Message-ID: <YXk0hAgaSJbLUgeB@coredump.intra.peff.net>
In-Reply-To: <211026.86y26gyqui.gmgdl@evledraar.gmail.com>

On Tue, Oct 26, 2021 at 01:15:00PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> 
> On Mon, Oct 25 2021, Junio C Hamano wrote:
> 
> > The fifteenth batch of topics are in 'master'.  I expect that this
> > is more-or-less what we can expect in the -rc0, unless there is a
> > hotfix to what's already merged.
> 
> I suggested a way forward for these iconv warnings that will be new in
> 2.34.0 at [1], poked'd a few days ago at [2].

Sorry to let this go for so long.

I do agree with your line of reasoning that it would be nice to sanity-check the incoming encoding once, but I think the details of doing so make things to tricky (to the point that it isn't worth doing; see my response just now in that thread).

Show 12 quoted lines

> Jeff: What do you think? Per [1] I think it's best to drop it entirely
> for now, or split out just the "completely unknown encoding" problem
> from "can't decode this particular thing".
> 
> You also had a patch in [3] that wasn't picked up, which would warn
> about this once.
> 
> If we *are* going to warn that seems like the worst of both in some
> sense, i.e. we'll no longer give users anything like "in this huge
> commit stream, we couldn't search in commit XYZ", instead we just warn
> on whatever happens to be the first commit, and you'll have no idea if
> subsequent matches were completed or not.

Yeah. I pretty much hate all of the options here. ;)

The firehose of warnings for "git log --encoding=nonsense" was known and discussed in fd680bc558 (logmsg_reencode(): warn when iconv() fails, 2021-08-27). It's ugly for sure, but I'm still OK with it for the reasoning there: your next step is to fix the --encoding argument you gave. Whether you saw one line of warning or many is not that important, IMHO. Giving a single more sensible warning ("your encoding 'nonsense' isn't valid") would be better, but I think it's hard to do without creating other problems.

But the most compelling argument against warning at all is the case you gave earlier: that there may be historical garbage commits, and you can't get rid of them, so being warned constantly that we're not going to show or grep them correctly is just annoying. And that is true whether the user sees one warning or a hundred.

So while I do hate to have Git just silently ignore errors, probably the original behavior is the least-bad thing, and we should just revert fd680bc558 (logmsg_reencode(): warn when iconv() fails, 2021-08-27). We probably want to salvage the documentation change (minus the "along with a warning") part.

Do you want to do the honors?

-Peff