Re: [PATCH v5] submodule merge: update conflict error message
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 19, 2022, 20:30 UTC
- Message-ID
- <xmqq8rooalr1.fsf@gitster.g>
- In-Reply-To
- <CAFySSZDoNX_5mxdKaT8JWkpYxBFxivQc_p2q_4bQk6YsT8QeZA@mail.gmail.com>
Calvin Wan <calvinwan@google.com> writes:
Show 17 quoted lines
>> These work on the result of calling find_first_merges(), but is it >> possible that we are asked to call this function more than once >> because we see conflicted submodule updates at two or more paths? > > This does get called multiple times if we see conflicted submodule > updates at two or more paths. > >> I may be misreading the code, but find_first_merges(), either the >> version we see in this file, or the one in merge-recursive.c, or its >> original introduced in 68d03e4a (Implement automatic fast-forward >> merge for submodules, 2010-07-07), look safe to be called twice. It >> runs the get_revision() machinery, smudging the object flags while >> walking the history, but I do not see any code that cleans up these >> flags for the second traversal. > > I don't quite understand which flags need to be cleaned up for the > second traversal.
UNINTERESTING, TREESAME, ADDED, SEEN, SHOWN are among the flags used by the object walk (if MyFirstObjectWalk does not talk about them, it probably should), and they need to be cleared before you prepare a new "struct rev_info" and throw it at setup_revisions(), prepare_revision_walk(), and start calling get_revision().
submodule.c::collect_changed_submodules() has its own revision walk, but it calls reset_revision_walk() to clear these flags from all objects in the superproject (i.e. the_repository).
I _think_ the reason why this never turned out to be a problem in practice is because we do not run this helper twice for the same submodule. Even though we may smudge many objects from a submodule with an object walk without clearing their flags, as long as we run the next object walk in a different submodule whose object flags are still unsmudged, it would be OK.