git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Jun 3, 2020, 21:23 UTC
Message-ID
<20200603212336.GD253041@google.com>
In-Reply-To
<1253efb6-f1bc-0a16-68e3-c1bc07e1bc18@gmail.com>
Hi,
Derrick Stolee wrote:
> On 6/3/2020 1:16 AM, Taylor Blau wrote:
Show 8 quoted lines
>>   * Keep the shallow bit sticky, at least for fetch.writeCommitGraph
>>     (i.e., pretend as if fetch.writecommitgraph=0 in the case of
>>     '--unshallow').
>
> I'm in favor of this option, if possible. Anything that alters the
> commit history in-memory at any point in the Git process is unsafe to
> combine with a commit-graph read _or_ write. I'm sorry that the guards
> in commit_graph_compatible() are not enough here.

As described in [1], I agree --- this kind of "dirty bit" is a good option and seems like the right thing to do here.

And I'm glad you said read _or_ write here. I hadn't realized that this check already applies for reads, and I'm very happy it does.

[...]
Show 12 quoted lines
>>   * Dump the object cache upon un-shallowing, forcing us to re-discover
>>     the parents when they are no longer hidden behind a graft.
>>
>> The latter seems like the most complete feasible fix. The former should
>> work fine to address this case, but I wonder if there are other
>> call-sites that are affected by this behavior. My hunch is that this is
>> a unique case, since it requires going from shallow to unshallow in the
>> same process.
>
> The latter would solve issues that could arise outside of the commit-graph
> space. But it also presents an opportunity for another gap if someone edits
> the shallow logic without putting in the proper guards.
This, however, I don't agree with.

I'm a strong believer in having clear invariants --- without them, code can only be understood empirically, and https://ieeexplore.ieee.org/document/9068304 describes how painful of a world that can be.

So because shallow is specifically about changing the set of parents in objects used for traversal, I want to make sure that we as reviewers will push back on any potential other new meaning of shallow. *If* we had a safe way to invalidate the object cache, it would be sufficient and would be the right thing to do. As described in [1], we unfortunately don't have such a thing.

Okay, that's all an aside.  Now for the actual reason I'm replying:

I had been wondering why this wasn't caught at read time, but of course --unshallow clears away the shallows so there was no way for that to happen. So what I wonder is, why isn't this caught by fsck? Can "git fsck" run "git commit-graph verify" automatically and include its result as part of what it reports?

Thanks, Jonathan

[1] https://lore.kernel.org/git/20200603205151.GC253041@google.com/
Previous: Taylor BlauNext: Jonathan Nieder
Message 27 of 32 in “shallow.c: use 'reset_repository_shallow' when appropriate”
  1. shallow.c: use 'reset_repository_shallow' when appropriateTaylor Blau, Apr 21, 2020
  2. Junio C HamanoApr 21, 2020
  3. Taylor BlauApr 21, 2020
  4. Junio C HamanoApr 21, 2020
  5. Taylor BlauApr 21, 2020
  6. Junio C HamanoApr 21, 2020
  7. Jonathan TanApr 22, 2020
  8. Jonathan TanApr 22, 2020
  9. Junio C HamanoApr 22, 2020
  10. Taylor BlauApr 23, 2020
  11. 0/2 shallow.c: reset shallow-ness after updatingTaylor Blau, Apr 23, 2020
  12. 1/2 t5537: use test_write_lines, indented heredocs for readabilityTaylor Blau, Apr 23, 2020
  13. Jonathan NiederApr 23, 2020
  14. Taylor BlauApr 24, 2020
  15. Jonathan NiederApr 24, 2020
  16. Junio C HamanoApr 24, 2020
  17. 2/2 shallow.c: use '{commit,rollback}_shallow_file'Taylor Blau, Apr 23, 2020
  18. Jonathan NiederApr 23, 2020
  19. Jonathan TanApr 23, 2020
  20. Junio C HamanoApr 23, 2020
  21. Taylor BlauApr 24, 2020
  22. Jonathan NiederJun 3, 2020
  23. Taylor BlauJun 3, 2020
  24. Taylor BlauJun 3, 2020
  25. Derrick StoleeJun 3, 2020
  26. Taylor BlauJun 3, 2020
  27. Jonathan NiederJun 3, 2020
  28. Jonathan NiederJun 3, 2020
  29. Taylor BlauJun 3, 2020
  30. Jonathan NiederJun 3, 2020
  31. Taylor BlauJun 4, 2020
  32. Junio C HamanoApr 23, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.