Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > First of all, let me echo what Glen said [1], that this series is > overall well laid out and makes sense. > > Other reviewers have commented on style issues, but I'll hold off on > making my comments on those and also possible improvements on commit > messages until I can say "besides style and commit messages, I think > that this series is good to merge in". > > [1] https://lore.kernel.org/git/kl6lr0yuqlk0.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > "Alphadelta14 via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: >> + // This current codepath isn't executed by any existing callbacks >> + // so it wouldn't show up as an issue at this time. > > I was a bit confused by this comment, so I looked at the surrounding > code. I think it could be better rephrased as: > > All existing callbacks at the time of writing cause this part of the > code to be skipped when S_ISGITLINK(entry.mode) is true, so this > wrong behavior does not call any issues. > As I already said, I do not think this is "wrong behaviour" to begin with. The current code requires that you'd use add_submodule_odb() to make the objects in them accessible and if your program fails to do so, as a very natural consequence, you'd not see objects pointed by the gitlink. Changing that assumption is OK as long as existing callers that depend on the current semantics are not broken by such a change, but I do not think "wrong behaviour does not call any issues" is a correct analysis of the problem.
