On Wed, Aug 10 2022, Jeff King wrote: > On Wed, Aug 10, 2022 at 04:04:17PM -0400, Jeff King wrote: > >> > Maybe downgrade to info or ignore by default then? It might still be >> > an issue for people who wilfully upgraded the diagnostic to error >> > hoping to catch the, but hopefully if they did that they'd rather get >> > the notice later than never? >> >> Yeah, that may be a sensible resolution. All things being equal I think >> "warning" is the right level, but out of caution and the historical >> precedent, maybe downgrading it to "info" is justified. >> >> It should be easy to work that into the patch I showed earlier. > > OK, so here are cleaned-up patches to do that. > > [1/3]: tree-walk: add a mechanism for getting non-canonicalized modes > [2/3]: fsck: actually detect bad file modes in trees > [3/3]: fsck: downgrade tree badFilemode to "info" This LGTM. I noticed/reported this issue more than a year ago, but the series I had for fixing it ended up being dropped, here's the report/analysis at the time: https://lore.kernel.org/git/20210308150650.18626-31-avarab@xxxxxxxxx/ Basically I was taking a much longer way around to avoid... > /* counts the number of bytes left in the `buffer`. */ > unsigned int size; > + > + /* option flags passed via init_tree_desc_gently() */ > + enum tree_desc_flags { > + TREE_DESC_RAW_MODES = (1 << 0), > + } flags; > }; ...this from 1/3 here, i.e. now we're paying the cost of an extra entry in every "struct tree_desc" user (which includes some hot codepaths), and just for this one fsck caller. I wonder if we couldn't introduce a init_tree_desc_gently_flags() for this instead. You note in 1/3 that you're (rightly) avoiding the churn of existing callers, but they could just use a "static inline" wrapper function that would set those flags to 0, we'd pass the flags down, and not put them into the "tree_desc" struct. Arguably it doesn't belong there at all, since this new thing is really an "opts" struct, but "tree_desc" is for "the state of the walk". It might/would make sense as a "raw_mode" in "struct name_entry" perhaps, but then we're gettin closer to the larger scope of my initial larger series, oh well ... :)
