* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 0:52 What's cooking in git.git (Sep 2021, #08; Mon, 27) Junio C Hamano
@ 2021-09-28 1:57 ` Ævar Arnfjörð Bjarmason
2021-09-28 20:52 ` Junio C Hamano
2021-09-28 6:46 ` Elijah Newren
` (5 subsequent siblings)
6 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-28 1:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, René Scharfe, Jeff King,
Carlo Marcelo Arenas Belón, SZEDER Gábor, Taylor Blau
On Mon, Sep 27 2021, Junio C Hamano wrote:
Updates on my topics, CC'd the people I mentioned.
> * ab/config-based-hooks-1 (2021-09-27) 8 commits
> - hook-list.h: add a generated list of hooks, like config-list.h
> - hook.c users: use "hook_exists()" instead of "find_hook()"
> - hook.c: add a hook_exists() wrapper and use it in bugreport.c
> - hook.[ch]: move find_hook() from run-command.c to hook.c
> - Makefile: remove an out-of-date comment
> - Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)
> - Makefile: stop hardcoding {command,config}-list.h
> - Makefile: mark "check" target as .PHONY
Thanks for picking this up. Looks like my split-up plan is working in
that this has received a lot of careful review.
I think per René's the v2 should be good-to-go, barring any new
undiscovered issues:
https://lore.kernel.org/git/0e07cee4-079a-af12-880f-d4a99300df28@web.de/
> * ab/http-pinned-public-key-mismatch (2021-09-27) 1 commit
> - http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors
>
> HTTPS error handling updates.
>
> Will merge to 'next'?
I'd like it merged. I think a fair summary of Jeff King's
https://lore.kernel.org/git/YU5CJC9XJvQITfr8@coredump.intra.peff.net/ is
"meh" though :)
I think the translation improvement + mentioning the specific git config
variable in play is a pretty minor but still worthwhile change.
> * ab/make-compdb-fix (2021-09-27) 1 commit
> - Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes
>
> Build update.
>
> Will merge to 'next'.
Thanks, FWIW I thought Carlos's cb/make-compdb-fix was better, but see
from this E-Mail & other context that you'd looked at both and preferred
the consistency with my COMPUTE_HEADER_DEPENDENCIES fix.
In any case both fix the immediate "pedantic" fallout.
> * ab/pack-objects-stdin (2021-07-09) 5 commits
> - pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
> - pack-objects.c: do stdin parsing via revision.c's API
> - revision.[ch]: add a "handle_stdin_line" API
> - revision.h: refactor "disable_stdin" and "read_from_stdin"
> - upload-pack: run is_repository_shallow() before setup_revisions()
>
> Introduce handle_stdin_line callback to revision API and uses it.
>
> Expecting a reroll.
I said I'd re-roll this into the larger "git bundle --stdin" topic I had
in mind, but when doing so found one memory leak, which I really wanted
the SANITIZE=leak interface to fix.
I think it's best to eject/discard this given the amount of other
outstanding stuff I've got. I don't really *need* this for anything
else, I'll loop back to it sometime after the SANITIZE=leak testing has
landed.
> * ab/fsck-unexpected-type (2021-09-22) 17 commits
> [...]
> "git fsck" has been taught to report mismatch between expected and
> actual types of an object better.
>
> Needs review.
The "needs review" note here pre-dates v6, which got a thorough review,
and Taylor seemed happy with the v7, aside from a minor issue I just
re-rolled v8 with a fix for:
https://lore.kernel.org/git/cover-v8-00.17-00000000000-20210928T021616Z-avarab@gmail.com/
> * ab/make-clean-depend-dirs (2021-09-22) 1 commit
> [...]
> Will merge to 'next'.
Thanks!
> * ab/auto-depend-with-pedantic (2021-09-22) 1 commit
> - Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic
>
> Improve build procedure for developers.
>
> Will merge to 'next'.
Thanks!
> * ab/make-sparse-for-real (2021-09-22) 1 commit
> - Makefile: make the "sparse" target non-.PHONY
>
> Prevent "make sparse" from running for the source files that
> haven't been modified.
>
> Will merge to 'next'?
I just submitted a hopefully final v4, please ignore the v3 :)
https://lore.kernel.org/git/patch-v4-1.1-f31fa3e8282-20210928T014023Z-avarab@gmail.com/T/#u
That leaves "sparse" as the bug-for-bug compatible old target, with a
new "sparse-incr" for those (like me) who want the incremental
version. The feedback on v2 was mainly about the change in behavior
breaking existing workflows, so v4 should get around that nicely.
> * ab/repo-settings-cleanup (2021-09-22) 5 commits
> [...]
> Will merge to 'next'.
Thanks!
> * ab/align-parse-options-help (2021-09-22) 4 commits
> - parse-options: properly align continued usage output
> - git rev-parse --parseopt tests: add more usagestr tests
> - send-pack: properly use parse_options() API for usage string
> - parse-options API users: align usage output in C-strings
>
> When "git cmd -h" shows more than one line of usage text (e.g.
> the cmd subcommand may take sub-sub-command), parse-options API
> learned to align these lines, even across i18n/l10n.
>
> Will merge to 'next'?
I think it's ready for that.
> * ab/help-config-vars (2021-09-23) 9 commits
> - help: move column config discovery to help.c library
> - help / completion: make "git help" do the hard work
> - help tests: test --config-for-completion option & output
> - help: simplify by moving to OPT_CMDMODE()
> - help: correct logic error in combining --all and --guides
> - help: correct logic error in combining --all and --config
> - help tests: add test for --config output
> - help: correct usage & behavior of "git help --guides"
> - help: correct the usage string in -h and documentation
>
> Teach "git help -c" into helping the command line completion of
> configuration variables.
>
> Will merge to 'next'?
I think so too, you had some feedback on the v3, I think partially
addressed by the "simplify by moving to OPT_CMDMODE()" step.
For further improving the usage info in the rare case of combining
e.g. --all and --config I'd prefer to just leave it for a future
improvement.
> * ab/sanitize-leak-ci (2021-09-23) 2 commits
> - tests: add a test mode for SANITIZE=leak, run it in CI
> - Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS
>
> CI learns to run the leak sanitizer builds.
>
> Will merge to 'next'?
Yes please! As noted in some other threads I've got more memory leak
fixes waiting on this, and since the v8 to avoid testing the split index
it doesn't seem to be causing any trouble:
https://lore.kernel.org/git/cover-v8-0.2-00000000000-20210923T091819Z-avarab@gmail.com/
> * ab/refs-errno-cleanup (2021-08-25) 4 commits
> - refs: make errno output explicit for refs_resolve_ref_unsafe
> - refs: explicitly return failure_errno from parse_loose_ref_contents
> - branch tests: test for errno propagating on failing read
> - refs: add failure_errno to refs_read_raw_ref() signature
> (this branch uses ab/refs-files-cleanup and hn/refs-errno-cleanup.)
>
> The "remainder" of hn/refs-errno-cleanup topic.
I think this should be ready too, but if you'd prefer to wait a bit
after ab/refs-files-cleanup and ab/refs-files-cleanup land I understand,
it's a lot of ref code changes...
> * ab/lib-subtest (2021-09-22) 9 commits
> - test-lib tests: get rid of copy/pasted mock test code
> - test-lib tests: assert 1 exit code, not non-zero
> - test-lib tests: refactor common part of check_sub_test_lib_test*()
> - test-lib tests: avoid subshell for "test_cmp" for readability
> - test-lib tests: don't provide a description for the sub-tests
> - test-lib tests: split up "write and run" into two functions
> - test-lib tests: move "run_sub_test" to a new lib-subtest.sh
> - Merge branch 'ps/t0000-output-directory-fix' into ab/lib-subtest
> - Merge branch 'jk/t0000-subtests-fix' into ab/lib-subtest
>
> Updates to the tests in t0000 to test the test framework.
>
> Will merge to 'next'?
Yes please, it was waiting on review for a long time. Carlo had some
feedback on it that I re-rolled v4 addressing, resolving what I think
was the main sticking point all along. I.e. conflating a test assertion
mode with this t0000 cleanup/split-up:
https://lore.kernel.org/git/cover-v4-0.7-00000000000-20210922T111734Z-avarab@gmail.com/
> * ab/only-single-progress-at-once (2021-09-22) 8 commits
> - progress.c: add & assert a "global_progress" variable
> - pack-bitmap-write.c: add a missing stop_progress()
> - progress.c: add temporary variable from progress struct
> - progress.c: stop eagerly fflush(stderr) when not a terminal
> - progress.c: call progress_interval() from progress_test_force_update()
> - progress.c: move signal handler functions lower
> - progress.c tests: test some invalid usage
> - progress.c tests: make start/stop verbs on stdin
>
> Further tweaks on progress API.
I think per
https://lore.kernel.org/git/cover-v2-0.8-00000000000-20210920T225701Z-avarab@gmail.com/
this should be ready to progress.
I.e. it was held off due to comment from SZEDER that once clarified I
understand to be a general concern that we should test the 8/8 step. As
noted in the re-rolled 8/8 I've tested this thoroughly in a way that
would break if that BUG() was going to bite us.
> * ab/refs-files-cleanup (2021-08-25) 13 commits
> [...]
>
> Continued work on top of the hn/refs-errno-cleanup topic.
>
> Will merge to 'master'.
[...]
> * hn/refs-errno-cleanup (2021-08-25) 4 commits
> [...]
>
> Will merge to 'master'.
Yay, thanks!
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 1:57 ` Ævar Arnfjörð Bjarmason
@ 2021-09-28 20:52 ` Junio C Hamano
0 siblings, 0 replies; 40+ messages in thread
From: Junio C Hamano @ 2021-09-28 20:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, René Scharfe, Jeff King,
Carlo Marcelo Arenas Belón, SZEDER Gábor, Taylor Blau
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> * ab/http-pinned-public-key-mismatch (2021-09-27) 1 commit
>> - http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors
>>
>> HTTPS error handling updates.
>>
>> Will merge to 'next'?
>
> I'd like it merged. I think a fair summary of Jeff King's
> https://lore.kernel.org/git/YU5CJC9XJvQITfr8@coredump.intra.peff.net/ is
> "meh" though :)
Mine is also "I do not mind seeing it in or out."
>> * ab/pack-objects-stdin (2021-07-09) 5 commits
>> - pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
>> - pack-objects.c: do stdin parsing via revision.c's API
>> - revision.[ch]: add a "handle_stdin_line" API
>> - revision.h: refactor "disable_stdin" and "read_from_stdin"
>> - upload-pack: run is_repository_shallow() before setup_revisions()
>>
>> Introduce handle_stdin_line callback to revision API and uses it.
>>
>> Expecting a reroll.
>
> I said I'd re-roll this into the larger "git bundle --stdin" topic I had
> in mind, but when doing so found one memory leak, which I really wanted
> the SANITIZE=leak interface to fix.
>
> I think it's best to eject/discard this given the amount of other
> outstanding stuff I've got. I don't really *need* this for anything
> else, I'll loop back to it sometime after the SANITIZE=leak testing has
> landed.
OK, will mark as "Retracted for now".
>> * ab/fsck-unexpected-type (2021-09-22) 17 commits
>> [...]
>> "git fsck" has been taught to report mismatch between expected and
>> actual types of an object better.
>>
>> Needs review.
>
> The "needs review" note here pre-dates v6, which got a thorough review,
> and Taylor seemed happy with the v7, aside from a minor issue I just
> re-rolled v8 with a fix for:
>
> https://lore.kernel.org/git/cover-v8-00.17-00000000000-20210928T021616Z-avarab@gmail.com/
Thanks.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 0:52 What's cooking in git.git (Sep 2021, #08; Mon, 27) Junio C Hamano
2021-09-28 1:57 ` Ævar Arnfjörð Bjarmason
@ 2021-09-28 6:46 ` Elijah Newren
2021-09-28 7:45 ` Ævar Arnfjörð Bjarmason
` (4 more replies)
2021-09-28 8:22 ` da/difftool (was: Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)) Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
6 siblings, 5 replies; 40+ messages in thread
From: Elijah Newren @ 2021-09-28 6:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King, Jonathan Nieder
[Did some slight re-ordering of topics]
On Mon, Sep 27, 2021 at 5:53 PM Junio C Hamano <gitster@pobox.com> wrote:
> * ds/add-rm-with-sparse-index (2021-09-24) 13 commits
> - advice: update message to suggest '--sparse'
> - mv: refuse to move sparse paths
> - rm: skip sparse paths with missing SKIP_WORKTREE
> - rm: add --sparse option
> - add: update --renormalize to skip sparse paths
> - add: update --chmod to skip sparse paths
> - add: implement the --sparse option
> - add: skip tracked paths outside sparse-checkout cone
> - add: fail when adding an untracked sparse file
> - dir: fix pattern matching on dirs
> - dir: select directories correctly
> - t1092: behavior for adding sparse files
> - t3705: test that 'sparse_entry' is unstaged
>
> "git add", "git mv", and "git rm" have been adjusted to avoid
> updating paths outside of the sparse-checkout definition unless
> the user specifies a "--sparse" option.
>
> Will merge to 'next'?
It would be nice to see the --diff-filter=u change, which you also
seemed to like[1]; but after that, yeah this is ready to merge down.
[1] https://lore.kernel.org/git/xmqq35pppwsm.fsf@gitster.g/
> * js/scalar (2021-09-14) 15 commits
> - scalar: accept -C and -c options before the subcommand
> - scalar: implement the `version` command
> - scalar: implement the `delete` command
> - scalar: teach 'reconfigure' to optionally handle all registered enlistments
> - scalar: allow reconfiguring an existing enlistment
> - scalar: implement the `run` command
> - scalar: teach 'clone' to support the --single-branch option
> - scalar: implement the `clone` subcommand
> - scalar: implement 'scalar list'
> - scalar: let 'unregister' handle a deleted enlistment directory gracefully
> - scalar: 'unregister' stops background maintenance
> - scalar: 'register' sets recommended config and starts maintenance
> - scalar: create test infrastructure
> - scalar: start documenting the command
> - scalar: create a rudimentary executable
>
> Add pieces from "scalar" to contrib/.
>
> Will merge to 'next'?
I feel bad for taking so long to take a look, but I finally responded
with a few comments. Mostly, I'm glad it's a contrib thing; a lot of
the config makes sense to me (even if some of it is 'meh' for my
repository sizes or setup), but two of the config settings would be
very objectionable if they were a core git command. Since it's
contrib, though, it's probably fine to be very opinionated...and
perhaps even excessively so. ;-)
However, since Johannes has been away for a couple weeks, maybe give
him a chance to return and respond to myself and others (and perhaps
push any updates that occurred to him while on vacation) before
merging down?
> * en/remerge-diff (2021-08-31) 7 commits
> - doc/diff-options: explain the new --remerge-diff option
> - show, log: provide a --remerge-diff capability
> - tmp-objdir: new API for creating and removing primary object dirs
> - merge-ort: capture and print ll-merge warnings in our preferred fashion
> - ll-merge: add API for capturing warnings in a strbuf instead of stderr
> - merge-ort: add ability to record conflict messages in a file
> - merge-ort: mark a few more conflict messages as omittable
>
> A new presentation for two-parent merge "--remerge-diff" can be
> used to show the difference between mechanical (and possibly
> conflicted) merge results and the recorded resolution.
>
> Will merge to 'next'?
It has been a month that it's been cooking with no issues brought up,
and it's been in production for nearly a year...
But just this morning I pinged peff and jrnieder if they might have
time to respectively look at the tmp-objdir stuff (patch 5, plus its
integration into log-tree.c in patch 7) and the ll-merge.[ch] changes
(patch 3). I don't know if either will have time to do it, but
perhaps wait half a week or so to see if they'll mention they have
time? Otherwise, yeah, it's probably time to merge this down.
> * en/removing-untracked-fixes (2021-09-27) 12 commits
> - Documentation: call out commands that nuke untracked files/directories
> - Comment important codepaths regarding nuking untracked files/dirs
> - unpack-trees: avoid nuking untracked dir in way of locally deleted file
> - unpack-trees: avoid nuking untracked dir in way of unmerged file
> - Change unpack_trees' 'reset' flag into an enum
> - Remove ignored files by default when they are in the way
> - unpack-trees: make dir an internal-only struct
> - unpack-trees: introduce preserve_ignored to unpack_trees_options
> - read-tree, merge-recursive: overwrite ignored files by default
> - checkout, read-tree: fix leak of unpack_trees_options.dir
> - t2500: add various tests for nuking untracked files
> - Merge branch 'en/am-abort-fix' into en/removing-untracked-fixes
>
> Various fixes in code paths that move untracked files away to make room.
>
> Will merge to 'next'?
I just sent out v3 this morning with five new patches (included in
your list above). While I think my patches are good, and I'd like to
see them merged down to next so I can send my
current-working-directory-deletion fixes that build on top of it, I'm
a little surprised you're proposing to merge this series down this
quickly instead of waiting a little longer for review of the new
patches. I'm not complaining...but was that intentional?
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 6:46 ` Elijah Newren
@ 2021-09-28 7:45 ` Ævar Arnfjörð Bjarmason
2021-09-28 17:25 ` Junio C Hamano
2021-09-28 8:07 ` Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
4 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-28 7:45 UTC (permalink / raw)
To: Elijah Newren
Cc: Junio C Hamano, Git Mailing List, Jeff King, Jonathan Nieder,
Neeraj Singh
On Mon, Sep 27 2021, Elijah Newren wrote:
> [Did some slight re-ordering of topics]
>
> On Mon, Sep 27, 2021 at 5:53 PM Junio C Hamano <gitster@pobox.com> wrote:
> [...]
>> * en/remerge-diff (2021-08-31) 7 commits
>> - doc/diff-options: explain the new --remerge-diff option
>> - show, log: provide a --remerge-diff capability
>> - tmp-objdir: new API for creating and removing primary object dirs
>> - merge-ort: capture and print ll-merge warnings in our preferred fashion
>> - ll-merge: add API for capturing warnings in a strbuf instead of stderr
>> - merge-ort: add ability to record conflict messages in a file
>> - merge-ort: mark a few more conflict messages as omittable
>>
>> A new presentation for two-parent merge "--remerge-diff" can be
>> used to show the difference between mechanical (and possibly
>> conflicted) merge results and the recorded resolution.
>>
>> Will merge to 'next'?
>
> It has been a month that it's been cooking with no issues brought up,
> and it's been in production for nearly a year...
>
> But just this morning I pinged peff and jrnieder if they might have
> time to respectively look at the tmp-objdir stuff (patch 5, plus its
> integration into log-tree.c in patch 7) and the ll-merge.[ch] changes
> (patch 3). I don't know if either will have time to do it, but
> perhaps wait half a week or so to see if they'll mention they have
> time? Otherwise, yeah, it's probably time to merge this down.
I haven't poked at it much, but haven't you and Neeraj Singh (CC'd)
independently come up with two slightly different changes in
tmp-objdir.c to do the same thing? See the tmp-objdir.c part of:
http://lore.kernel.org/git/543ea3569342165363c1602ce36683a54dce7a0b.1632527609.git.gitgitgadget@gmail.com
And your:
http://lore.kernel.org/git/67d3b2b09f9ddda616cdd0d1b12ab7afc73670ed.1630376800.git.gitgitgadget@gmail.com
I.e. yours has the object database managed outside, his has it added to
"struct tmp_objdir", but it's the same objdir dance isn't it?
I started reading the combined code in "seen" and found it quite
confusing until I saw what was going on.
For both, if you can agree on some common API: I'd prefer if the
"%s/incoming-XXXXXX" part of the "tmp_objdir_create()" was passed as
some template, perhaps just the string "incoming" as a prefix?
I.e. this was for receive-pack.c originally, now it's for bulk-checkin.c
and log.c, if either of those segfault or is long-running it's quite
confusing to have something called "incoming" if you're manually
inspecting it. Perhaps "core-fsyncObjectFiles-batch" (or even
"core.fsyncObjectFiles=batch") and "log-remerge-diff" as prefixes for
the two, and "incoming" for the one existing caller in "master"?
>> * en/removing-untracked-fixes (2021-09-27) 12 commits
>> - Documentation: call out commands that nuke untracked files/directories
>> - Comment important codepaths regarding nuking untracked files/dirs
>> - unpack-trees: avoid nuking untracked dir in way of locally deleted file
>> - unpack-trees: avoid nuking untracked dir in way of unmerged file
>> - Change unpack_trees' 'reset' flag into an enum
>> - Remove ignored files by default when they are in the way
>> - unpack-trees: make dir an internal-only struct
>> - unpack-trees: introduce preserve_ignored to unpack_trees_options
>> - read-tree, merge-recursive: overwrite ignored files by default
>> - checkout, read-tree: fix leak of unpack_trees_options.dir
>> - t2500: add various tests for nuking untracked files
>> - Merge branch 'en/am-abort-fix' into en/removing-untracked-fixes
>>
>> Various fixes in code paths that move untracked files away to make room.
>>
>> Will merge to 'next'?
>
> I just sent out v3 this morning with five new patches (included in
> your list above). While I think my patches are good, and I'd like to
> see them merged down to next so I can send my
> current-working-directory-deletion fixes that build on top of it, I'm
> a little surprised you're proposing to merge this series down this
> quickly instead of waiting a little longer for review of the new
> patches. I'm not complaining...but was that intentional?
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 7:45 ` Ævar Arnfjörð Bjarmason
@ 2021-09-28 17:25 ` Junio C Hamano
2021-09-28 21:00 ` Neeraj Singh
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-09-28 17:25 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Elijah Newren, Git Mailing List, Jeff King, Jonathan Nieder,
Neeraj Singh
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> I haven't poked at it much, but haven't you and Neeraj Singh (CC'd)
> independently come up with two slightly different changes in
> tmp-objdir.c to do the same thing? See the tmp-objdir.c part of:
>
> http://lore.kernel.org/git/543ea3569342165363c1602ce36683a54dce7a0b.1632527609.git.gitgitgadget@gmail.com
>
> And your:
>
> http://lore.kernel.org/git/67d3b2b09f9ddda616cdd0d1b12ab7afc73670ed.1630376800.git.gitgitgadget@gmail.com
>
> I.e. yours has the object database managed outside, his has it added to
> "struct tmp_objdir", but it's the same objdir dance isn't it?
They touch the same tmp-objdir, but unlike the original use in the
receive-pack (i.e. responding to "git push") with the intention to
add the objects collected in them back to the primary, remerge-diff
wants to discard what was added there at the end. I do not think it
would directly help the bulk-fsync stuff (but I didn't quite see why
bulk-fsync stuff needed to _add_ new functions to tmp-objdir API,
instead of just being a customer of tmp-objdir API), where it wants
to do the same _migrate() dance in the end.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 17:25 ` Junio C Hamano
@ 2021-09-28 21:00 ` Neeraj Singh
2021-09-28 23:34 ` Junio C Hamano
0 siblings, 1 reply; 40+ messages in thread
From: Neeraj Singh @ 2021-09-28 21:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Elijah Newren,
Git Mailing List, Jeff King, Jonathan Nieder, Neeraj Singh
On Tue, Sep 28, 2021 at 10:25:08AM -0700, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > I haven't poked at it much, but haven't you and Neeraj Singh (CC'd)
> > independently come up with two slightly different changes in
> > tmp-objdir.c to do the same thing? See the tmp-objdir.c part of:
> >
> > http://lore.kernel.org/git/543ea3569342165363c1602ce36683a54dce7a0b.1632527609.git.gitgitgadget@gmail.com
> >
> > And your:
> >
> > http://lore.kernel.org/git/67d3b2b09f9ddda616cdd0d1b12ab7afc73670ed.1630376800.git.gitgitgadget@gmail.com
> >
> > I.e. yours has the object database managed outside, his has it added to
> > "struct tmp_objdir", but it's the same objdir dance isn't it?
>
> They touch the same tmp-objdir, but unlike the original use in the
> receive-pack (i.e. responding to "git push") with the intention to
> add the objects collected in them back to the primary, remerge-diff
> wants to discard what was added there at the end. I do not think it
> would directly help the bulk-fsync stuff (but I didn't quite see why
> bulk-fsync stuff needed to _add_ new functions to tmp-objdir API,
> instead of just being a customer of tmp-objdir API), where it wants
> to do the same _migrate() dance in the end.
Both Elijah and I needed the same functionality of having a writable
ODB in the current process, which wasn't previously provided by tmp-objdir.
I'm making a new patch which is an amalgamation of Elijah's version and mine.
I'll rebase and resend my patch series with that version and I'll also make
a modified version of Elijah's branch available on github.
Thanks,
Neeraj
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 21:00 ` Neeraj Singh
@ 2021-09-28 23:34 ` Junio C Hamano
2021-09-28 23:53 ` Neeraj Singh
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-09-28 23:34 UTC (permalink / raw)
To: Neeraj Singh
Cc: Ævar Arnfjörð Bjarmason, Elijah Newren,
Git Mailing List, Jeff King, Jonathan Nieder, Neeraj Singh
Neeraj Singh <nksingh85@gmail.com> writes:
> Both Elijah and I needed the same functionality of having a writable
> ODB in the current process, which wasn't previously provided by tmp-objdir.
>
> I'm making a new patch which is an amalgamation of Elijah's version and mine.
> I'll rebase and resend my patch series with that version and I'll also make
> a modified version of Elijah's branch available on github.
Sounds good. Thanks.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 23:34 ` Junio C Hamano
@ 2021-09-28 23:53 ` Neeraj Singh
2021-10-07 22:01 ` Junio C Hamano
0 siblings, 1 reply; 40+ messages in thread
From: Neeraj Singh @ 2021-09-28 23:53 UTC (permalink / raw)
To: Junio C Hamano, Elijah Newren
Cc: Ævar Arnfjörð Bjarmason, Git Mailing List,
Jeff King, Jonathan Nieder, Neeraj Singh
On Tue, Sep 28, 2021 at 4:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Neeraj Singh <nksingh85@gmail.com> writes:
>
> > Both Elijah and I needed the same functionality of having a writable
> > ODB in the current process, which wasn't previously provided by tmp-objdir.
> >
> > I'm making a new patch which is an amalgamation of Elijah's version and mine.
> > I'll rebase and resend my patch series with that version and I'll also make
> > a modified version of Elijah's branch available on github.
>
> Sounds good. Thanks.
I just posted the new series to the list.
Elijah,
Here's a branch of your changes based on the amalgamated tmp-objdir code:
https://github.com/neerajsi-msft/git/commits/neerajsi/remerge-diff
This commit adapts your code to use the amalgamated API:
https://github.com/neerajsi-msft/git/commit/725328fe1d8be8326d2ddef78e164ca21450b100
Thanks,
Neeraj
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 23:53 ` Neeraj Singh
@ 2021-10-07 22:01 ` Junio C Hamano
2021-10-08 6:51 ` Elijah Newren
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-10-07 22:01 UTC (permalink / raw)
To: Neeraj Singh, Elijah Newren
Cc: Ævar Arnfjörð Bjarmason, Git Mailing List,
Jeff King, Jonathan Nieder, Neeraj Singh
Neeraj Singh <nksingh85@gmail.com> writes:
> Elijah,
>
> Here's a branch of your changes based on the amalgamated tmp-objdir code:
> https://github.com/neerajsi-msft/git/commits/neerajsi/remerge-diff
>
> This commit adapts your code to use the amalgamated API:
> https://github.com/neerajsi-msft/git/commit/725328fe1d8be8326d2ddef78e164ca21450b100
It seems that the discussion petered out at this point.
Right now I have a version of ns/remerge-diff before this adjustment
in 'seen', and Neeraj's latest version is kept out of 'seen' as they
do not play well together without an adjustment like that.
What's the good way forward? I do not deeply care which one goes
first, but I have a feeling that the need by remerge-diff that wants
to discard temporary objects would involve more work to make it safe
than the need by batched fsync where newly created objects will not
be discarded but merely moved to the primary store before the end of
the operation, so from that point of view, it seems simpler and
safer to queue ns/batched-fsync topic first (especially given that
it is a no-op until the end-user opts into the experiment), and have
a remerge-diff that uses the infrastructure from Neeraj's topic.
What's your take on the rebase Neeraj made, Elijah (at the URL
above)?
Thanks.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-07 22:01 ` Junio C Hamano
@ 2021-10-08 6:51 ` Elijah Newren
2021-10-08 22:30 ` Neeraj Singh
2021-10-08 23:01 ` Junio C Hamano
0 siblings, 2 replies; 40+ messages in thread
From: Elijah Newren @ 2021-10-08 6:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Neeraj Singh, Ævar Arnfjörð Bjarmason,
Git Mailing List, Jeff King, Jonathan Nieder, Neeraj Singh
On Thu, Oct 7, 2021 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Neeraj Singh <nksingh85@gmail.com> writes:
>
> > Elijah,
> >
> > Here's a branch of your changes based on the amalgamated tmp-objdir code:
> > https://github.com/neerajsi-msft/git/commits/neerajsi/remerge-diff
> >
> > This commit adapts your code to use the amalgamated API:
> > https://github.com/neerajsi-msft/git/commit/725328fe1d8be8326d2ddef78e164ca21450b100
>
> It seems that the discussion petered out at this point.
>
> Right now I have a version of ns/remerge-diff before this adjustment
> in 'seen', and Neeraj's latest version is kept out of 'seen' as they
> do not play well together without an adjustment like that.
>
> What's the good way forward? I do not deeply care which one goes
> first, but I have a feeling that the need by remerge-diff that wants
> to discard temporary objects would involve more work to make it safe
> than the need by batched fsync where newly created objects will not
> be discarded but merely moved to the primary store before the end of
> the operation, so from that point of view, it seems simpler and
> safer to queue ns/batched-fsync topic first (especially given that
> it is a no-op until the end-user opts into the experiment), and have
> a remerge-diff that uses the infrastructure from Neeraj's topic.
>
> What's your take on the rebase Neeraj made, Elijah (at the URL
> above)?
I meant to dig further, but nearly all my git time in the last week
and a half was attempting to keep up with other patch reviews. My git
time is fast disappearing in the near term, and it's not clear how
much, if any, time I'll have to work on patches (or even continued
reviewing) before, say, mid-November. I most likely won't be able to
do any discussion-prep work in advance of the Git Contributor's
Summit, and might not even be able to attend anymore.
I had looked over Neeraj's patches and they looked reasonable. I
thought there might be some tweaks that I could try out, but at this
point, just take what he has and keep my topic as expecting an update.
I'll circle back eventually.
Sorry for the delay.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-08 6:51 ` Elijah Newren
@ 2021-10-08 22:30 ` Neeraj Singh
2021-10-08 23:01 ` Junio C Hamano
1 sibling, 0 replies; 40+ messages in thread
From: Neeraj Singh @ 2021-10-08 22:30 UTC (permalink / raw)
To: Elijah Newren
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Git Mailing List, Jeff King, Jonathan Nieder, Neeraj Singh
Thanks Junio. I'd be happy to have my change go in first. I'm hoping
that we'll be able to get a lot of mileage with batch mode in Git For
Windows early on.
Thanks,
Neeraj
On Thu, Oct 7, 2021 at 11:52 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Thu, Oct 7, 2021 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Neeraj Singh <nksingh85@gmail.com> writes:
> >
> > > Elijah,
> > >
> > > Here's a branch of your changes based on the amalgamated tmp-objdir code:
> > > https://github.com/neerajsi-msft/git/commits/neerajsi/remerge-diff
> > >
> > > This commit adapts your code to use the amalgamated API:
> > > https://github.com/neerajsi-msft/git/commit/725328fe1d8be8326d2ddef78e164ca21450b100
> >
> > It seems that the discussion petered out at this point.
> >
> > Right now I have a version of ns/remerge-diff before this adjustment
> > in 'seen', and Neeraj's latest version is kept out of 'seen' as they
> > do not play well together without an adjustment like that.
> >
> > What's the good way forward? I do not deeply care which one goes
> > first, but I have a feeling that the need by remerge-diff that wants
> > to discard temporary objects would involve more work to make it safe
> > than the need by batched fsync where newly created objects will not
> > be discarded but merely moved to the primary store before the end of
> > the operation, so from that point of view, it seems simpler and
> > safer to queue ns/batched-fsync topic first (especially given that
> > it is a no-op until the end-user opts into the experiment), and have
> > a remerge-diff that uses the infrastructure from Neeraj's topic.
> >
> > What's your take on the rebase Neeraj made, Elijah (at the URL
> > above)?
>
> I meant to dig further, but nearly all my git time in the last week
> and a half was attempting to keep up with other patch reviews. My git
> time is fast disappearing in the near term, and it's not clear how
> much, if any, time I'll have to work on patches (or even continued
> reviewing) before, say, mid-November. I most likely won't be able to
> do any discussion-prep work in advance of the Git Contributor's
> Summit, and might not even be able to attend anymore.
>
> I had looked over Neeraj's patches and they looked reasonable. I
> thought there might be some tweaks that I could try out, but at this
> point, just take what he has and keep my topic as expecting an update.
> I'll circle back eventually.
>
> Sorry for the delay.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-08 6:51 ` Elijah Newren
2021-10-08 22:30 ` Neeraj Singh
@ 2021-10-08 23:01 ` Junio C Hamano
1 sibling, 0 replies; 40+ messages in thread
From: Junio C Hamano @ 2021-10-08 23:01 UTC (permalink / raw)
To: Elijah Newren
Cc: Neeraj Singh, Ævar Arnfjörð Bjarmason,
Git Mailing List, Jeff King, Jonathan Nieder, Neeraj Singh
Elijah Newren <newren@gmail.com> writes:
> I had looked over Neeraj's patches and they looked reasonable. I
> thought there might be some tweaks that I could try out, but at this
> point, just take what he has and keep my topic as expecting an update.
> I'll circle back eventually.
Thanks.
I've fetched Neeraj's remerge-diff rebase, and structured three
topics:
. 'ns/tmp-objdir' topic is to hold the bottommost two tmp-objdir
patches.
. 'ns/batched-fsync' topic forks from 'master', merges
'ns/tmp-objdir', and then applies its own patches.
. 'ns/remerge-diff' topic forks from 'master', merges
'ns/tmp-objdir', and then applies its own patches.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 6:46 ` Elijah Newren
2021-09-28 7:45 ` Ævar Arnfjörð Bjarmason
@ 2021-09-28 8:07 ` Ævar Arnfjörð Bjarmason
2021-09-28 17:27 ` Junio C Hamano
2021-09-28 13:31 ` Derrick Stolee
` (2 subsequent siblings)
4 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-28 8:07 UTC (permalink / raw)
To: Elijah Newren
Cc: Junio C Hamano, Git Mailing List, Jeff King, Jonathan Nieder
On Mon, Sep 27 2021, Elijah Newren wrote:
[Aside: I quite liked the method of replying to What's Cooking per-topic
in Johannes's replies to
https://lore.kernel.org/git/xmqqsfyf5b74.fsf@gitster.g/; I'm never quite
sure if I should have one E-Mail with all of my comments on other
in-flight work, use existing commentary as a springboard etc. Doing the
latter here]
>> * js/scalar (2021-09-14) 15 commits
>> - scalar: accept -C and -c options before the subcommand
>> - scalar: implement the `version` command
>> - scalar: implement the `delete` command
>> - scalar: teach 'reconfigure' to optionally handle all registered enlistments
>> - scalar: allow reconfiguring an existing enlistment
>> - scalar: implement the `run` command
>> - scalar: teach 'clone' to support the --single-branch option
>> - scalar: implement the `clone` subcommand
>> - scalar: implement 'scalar list'
>> - scalar: let 'unregister' handle a deleted enlistment directory gracefully
>> - scalar: 'unregister' stops background maintenance
>> - scalar: 'register' sets recommended config and starts maintenance
>> - scalar: create test infrastructure
>> - scalar: start documenting the command
>> - scalar: create a rudimentary executable
>>
>> Add pieces from "scalar" to contrib/.
>>
>> Will merge to 'next'?
>
> I feel bad for taking so long to take a look, but I finally responded
> with a few comments. Mostly, I'm glad it's a contrib thing; a lot of
> the config makes sense to me (even if some of it is 'meh' for my
> repository sizes or setup), but two of the config settings would be
> very objectionable if they were a core git command. Since it's
> contrib, though, it's probably fine to be very opinionated...and
> perhaps even excessively so. ;-)
>
> However, since Johannes has been away for a couple weeks, maybe give
> him a chance to return and respond to myself and others (and perhaps
> push any updates that occurred to him while on vacation) before
> merging down?
Since the thread at
https://lore.kernel.org/git/87ilz44kdk.fsf@evledraar.gmail.com/ I've
been running with an altered version of Johannes's patches that does the
Makefile integration differently, per my
https://lore.kernel.org/git/87ilz44kdk.fsf@evledraar.gmail.com/.
I've been trickling in some of the Makefile changes that make that
easier (still quite possible on top of "master", I just had some
cleanups). It builds[1], tests[2], installs[3] and participates in any
auxiliary targets like "check-docs", "sparse"[4] (and now my
"sparse-incr") etc.
AFAIKT Johannes's version is so far just doing [1] and [2], and just
optionally if you "make" in the contrib subdirectory.
Junio seemed positive on that direction in [1][2]. Re your comments in
[3]: Right now "scalar" doesn't define any of its own config, but one
way out of some of your comments seems to be to do that.
If it's using its own build system in contrib/scalar that'll happen how
exactly? We keep the main manpage in contrib/scalar/scalar.txt but have
a Documentation/config/scalar.txt? Have "git help --config" grow some
optional mode for scalar-specific config? Add a "scalar help-config"?
In case my opinion on it isn't painfully obvious at this point I think
the answer for both current & future integration reasons should be to
just have its assets & build logic intergrated with existing locations
and logic.
1. https://lore.kernel.org/git/xmqqilz32hhr.fsf@gitster.g/
2. https://lore.kernel.org/git/xmqq1r5qzv35.fsf@gitster.g/
3. https://lore.kernel.org/git/CABPp-BG_wupp1o5bBSYOJSvF3eJjf=TbX0RBHqqKuD+3F8s6hw@mail.gmail.com/
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 8:07 ` Ævar Arnfjörð Bjarmason
@ 2021-09-28 17:27 ` Junio C Hamano
0 siblings, 0 replies; 40+ messages in thread
From: Junio C Hamano @ 2021-09-28 17:27 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Elijah Newren, Git Mailing List, Jeff King, Jonathan Nieder
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Mon, Sep 27 2021, Elijah Newren wrote:
>
> [Aside: I quite liked the method of replying to What's Cooking per-topic
> in Johannes's replies to
> https://lore.kernel.org/git/xmqqsfyf5b74.fsf@gitster.g/; I'm never quite
> sure if I should have one E-Mail with all of my comments on other
> in-flight work, use existing commentary as a springboard etc. Doing the
> latter here]
Yup. Splitting them out may be more work to produce comments, but
it lets you Cc relevant folks and would be easier for those who
participate in the discussion.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 6:46 ` Elijah Newren
2021-09-28 7:45 ` Ævar Arnfjörð Bjarmason
2021-09-28 8:07 ` Ævar Arnfjörð Bjarmason
@ 2021-09-28 13:31 ` Derrick Stolee
2021-09-28 17:33 ` Junio C Hamano
2021-09-28 17:16 ` Junio C Hamano
2021-09-28 23:40 ` Jeff King
4 siblings, 1 reply; 40+ messages in thread
From: Derrick Stolee @ 2021-09-28 13:31 UTC (permalink / raw)
To: Elijah Newren, Junio C Hamano
Cc: Git Mailing List, Jeff King, Jonathan Nieder
On 9/28/2021 2:46 AM, Elijah Newren wrote:
> [Did some slight re-ordering of topics]
>
> On Mon, Sep 27, 2021 at 5:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>
>> * ds/add-rm-with-sparse-index (2021-09-24) 13 commits
>> - advice: update message to suggest '--sparse'
>> - mv: refuse to move sparse paths
>> - rm: skip sparse paths with missing SKIP_WORKTREE
>> - rm: add --sparse option
>> - add: update --renormalize to skip sparse paths
>> - add: update --chmod to skip sparse paths
>> - add: implement the --sparse option
>> - add: skip tracked paths outside sparse-checkout cone
>> - add: fail when adding an untracked sparse file
>> - dir: fix pattern matching on dirs
>> - dir: select directories correctly
>> - t1092: behavior for adding sparse files
>> - t3705: test that 'sparse_entry' is unstaged
>>
>> "git add", "git mv", and "git rm" have been adjusted to avoid
>> updating paths outside of the sparse-checkout definition unless
>> the user specifies a "--sparse" option.
>>
>> Will merge to 'next'?
>
> It would be nice to see the --diff-filter=u change, which you also
> seemed to like[1]; but after that, yeah this is ready to merge down.
>
> [1] https://lore.kernel.org/git/xmqq35pppwsm.fsf@gitster.g/
Yes, I agree. I was waiting to see if more comments trickled in, but
it seems stable now. Do you want me to re-roll the whole series, or
do you want to apply the fixup below?
Thanks,
-Stolee
---- >8 ----
From d279bd580ad3b66187f9a8c0370acc5bca7cc5b6 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@microsoft.com>
Date: Tue, 28 Sep 2021 09:30:10 -0400
Subject: [PATCH] fixup! t1092: behavior for adding sparse files
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
t/t1092-sparse-checkout-compatibility.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index a00e42fa233..ca91c6a67f8 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -192,7 +192,7 @@ test_sparse_unstaged () {
for repo in sparse-checkout sparse-index
do
# Skip "unmerged" paths
- git -C $repo diff --staged --diff-filter=ACDMRTXB -- "$file" >diff &&
+ git -C $repo diff --staged --diff-filter=u -- "$file" >diff &&
test_must_be_empty diff || return 1
done
}
--
2.33.0.vfs.0.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 13:31 ` Derrick Stolee
@ 2021-09-28 17:33 ` Junio C Hamano
2021-09-28 20:16 ` Derrick Stolee
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-09-28 17:33 UTC (permalink / raw)
To: Derrick Stolee
Cc: Elijah Newren, Git Mailing List, Jeff King, Jonathan Nieder
Derrick Stolee <stolee@gmail.com> writes:
> Yes, I agree. I was waiting to see if more comments trickled in, but
> it seems stable now. Do you want me to re-roll the whole series, or
> do you want to apply the fixup below?
Let us wait for a bit more trickling in, but in the meantime, here
is what I'd keep. Thanks.
[ds/add-rm-with-sparse-index] $ git range-diff @{1}...
1: f1309dd7be ! 1: edd2cd345f t1092: behavior for adding sparse files
@@ t/t1092-sparse-checkout-compatibility.sh: test_sparse_match () {
+ for repo in sparse-checkout sparse-index
+ do
+ # Skip "unmerged" paths
-+ git -C $repo diff --staged --diff-filter=ACDMRTXB -- "$file" >diff &&
++ git -C $repo diff --staged --diff-filter=u -- "$file" >diff &&
+ test_must_be_empty diff || return 1
+ done
+}
2: 9a8c340809 = 2: f6526728f9 dir: select directories correctly
3: 2a4e495cd4 = 3: ed4958477b dir: fix pattern matching on dirs
...
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 17:33 ` Junio C Hamano
@ 2021-09-28 20:16 ` Derrick Stolee
0 siblings, 0 replies; 40+ messages in thread
From: Derrick Stolee @ 2021-09-28 20:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Git Mailing List, Jeff King, Jonathan Nieder
On 9/28/2021 1:33 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
>> Yes, I agree. I was waiting to see if more comments trickled in, but
>> it seems stable now. Do you want me to re-roll the whole series, or
>> do you want to apply the fixup below?
>
> Let us wait for a bit more trickling in, but in the meantime, here
> is what I'd keep. Thanks.
>
> [ds/add-rm-with-sparse-index] $ git range-diff @{1}...
> 1: f1309dd7be ! 1: edd2cd345f t1092: behavior for adding sparse files
> @@ t/t1092-sparse-checkout-compatibility.sh: test_sparse_match () {
> + for repo in sparse-checkout sparse-index
> + do
> + # Skip "unmerged" paths
> -+ git -C $repo diff --staged --diff-filter=ACDMRTXB -- "$file" >diff &&
> ++ git -C $repo diff --staged --diff-filter=u -- "$file" >diff &&
> + test_must_be_empty diff || return 1
> + done
> +}
> 2: 9a8c340809 = 2: f6526728f9 dir: select directories correctly
> 3: 2a4e495cd4 = 3: ed4958477b dir: fix pattern matching on dirs
That's exactly the diff I was hoping for. I'll keep an eye for more
feedback.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 6:46 ` Elijah Newren
` (2 preceding siblings ...)
2021-09-28 13:31 ` Derrick Stolee
@ 2021-09-28 17:16 ` Junio C Hamano
2021-09-29 6:42 ` Elijah Newren
2021-09-28 23:40 ` Jeff King
4 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-09-28 17:16 UTC (permalink / raw)
To: Elijah Newren; +Cc: Git Mailing List, Jeff King, Jonathan Nieder
Elijah Newren <newren@gmail.com> writes:
>> * js/scalar (2021-09-14) 15 commits
> ...
> However, since Johannes has been away for a couple weeks, maybe give
> him a chance to return and respond to myself and others (and perhaps
> push any updates that occurred to him while on vacation) before
> merging down?
Fair enough.
>> * en/remerge-diff (2021-08-31) 7 commits
>> - doc/diff-options: explain the new --remerge-diff option
>> - show, log: provide a --remerge-diff capability
>> - tmp-objdir: new API for creating and removing primary object dirs
>> - merge-ort: capture and print ll-merge warnings in our preferred fashion
>> - ll-merge: add API for capturing warnings in a strbuf instead of stderr
>> - merge-ort: add ability to record conflict messages in a file
>> - merge-ort: mark a few more conflict messages as omittable
>>
>> A new presentation for two-parent merge "--remerge-diff" can be
>> used to show the difference between mechanical (and possibly
>> conflicted) merge results and the recorded resolution.
>>
>> Will merge to 'next'?
>
> It has been a month that it's been cooking with no issues brought up,
> and it's been in production for nearly a year...
Please do not read that much for being in "seen". Until a topic
hits 'next', where some orgs package and ship to their internal
audience, I am not sure if it can be called "cooking".
But your using it on your folks in the production (how big is your
audience, I don't know) does count ;-)
> But just this morning I pinged peff and jrnieder if they might have
> time to respectively look at the tmp-objdir stuff (patch 5, plus its
> integration into log-tree.c in patch 7) and the ll-merge.[ch] changes
> (patch 3). I don't know if either will have time to do it, but
> perhaps wait half a week or so to see if they'll mention they have
> time?
Sure.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 17:16 ` Junio C Hamano
@ 2021-09-29 6:42 ` Elijah Newren
0 siblings, 0 replies; 40+ messages in thread
From: Elijah Newren @ 2021-09-29 6:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King, Jonathan Nieder
On Tue, Sep 28, 2021 at 10:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> * js/scalar (2021-09-14) 15 commits
> > ...
> > However, since Johannes has been away for a couple weeks, maybe give
> > him a chance to return and respond to myself and others (and perhaps
> > push any updates that occurred to him while on vacation) before
> > merging down?
>
> Fair enough.
>
> >> * en/remerge-diff (2021-08-31) 7 commits
> >> - doc/diff-options: explain the new --remerge-diff option
> >> - show, log: provide a --remerge-diff capability
> >> - tmp-objdir: new API for creating and removing primary object dirs
> >> - merge-ort: capture and print ll-merge warnings in our preferred fashion
> >> - ll-merge: add API for capturing warnings in a strbuf instead of stderr
> >> - merge-ort: add ability to record conflict messages in a file
> >> - merge-ort: mark a few more conflict messages as omittable
> >>
> >> A new presentation for two-parent merge "--remerge-diff" can be
> >> used to show the difference between mechanical (and possibly
> >> conflicted) merge results and the recorded resolution.
> >>
> >> Will merge to 'next'?
> >
> > It has been a month that it's been cooking with no issues brought up,
> > and it's been in production for nearly a year...
>
> Please do not read that much for being in "seen". Until a topic
> hits 'next', where some orgs package and ship to their internal
> audience, I am not sure if it can be called "cooking".
As a heads up, I found this topic in an email titled "What's
Cooking...", and in particular it was listed in a section of that
email that was titled "[Cooking]". ;-)
Sorry, I couldn't resist.
But yeah, I know "seen" doesn't mean much. My comment was reflecting
that while leaving a topic in seen for a while sometimes results in
more information coming forth to inform the decision about whether to
merge the series to next, or drop it, or ask the submitter for
changes, there comes a point where further waiting is unlikely to
provide any additional information that would help inform that
decision. I was starting to wonder whether we'd get any (more)
feedback. But I'm glad we waited a little longer. :-)
> But your using it on your folks in the production (how big is your
> audience, I don't know) does count ;-)
~100 opt-in users (apparently shot up in the past few months; used to
be ~50). I have a hard time gauging how heavily this particular
feature was/is used by them, however:
* The feature was explicitly announced, but I did it because I
thought it was cool and a nice demonstration of merge-ort features,
not because there were explicit requests for it
* I did make "-p" imply "--remerge-diff" for git-log in our internal
opt-in distribution, so that should increase usage significantly.
* A bug in an internal tool for a while was accidentally running
`git log -p --name-status $COMMIT --not
--remotes=ACCIDENTALLY_INVALID_REMOTE_NAME` for a while, including in
big repos; in combination with previous point, this increased usage
and scope of remerge-diff usage a fair amount as that command would
run on all commits in history.
* Most merges here are done by either GitHub (Enterprise) or Gerrit,
both of which require the user to rebase and re-push if there are any
conflicts. This makes remerge-diff boring for most merges. There are
some notable exceptions where merges are done differently, they're
just much less common. (--remerge-diff-only for cherry-picks and
reverts is a bit more interesting, but that's not part of this
series...and I also haven't gotten much feedback on it.)
* I have only received feedback from two users about the
remerge-diff feature, one only indirectly, and both reports actually
ended up being about underlying bugs in early versions of merge-ort[+]
[+] The bugs were (1) my QSORT in write_tree() was not originally
using tree_entry_order() and thus was writing trees with the wrong
ordering of elements; this was a bug that was fixed before
write_tree() was ever submitted upstream, and (2) and the bug fixed by
72b3091040f8 (merge-ort: use STABLE_QSORT instead of QSORT where
required, 2021-03-20)
> > But just this morning I pinged peff and jrnieder if they might have
> > time to respectively look at the tmp-objdir stuff (patch 5, plus its
> > integration into log-tree.c in patch 7) and the ll-merge.[ch] changes
> > (patch 3). I don't know if either will have time to do it, but
> > perhaps wait half a week or so to see if they'll mention they have
> > time?
>
> Sure.
Apparently that was a good idea; Peff and Ævar have both chimed in
with feedback. :-)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 6:46 ` Elijah Newren
` (3 preceding siblings ...)
2021-09-28 17:16 ` Junio C Hamano
@ 2021-09-28 23:40 ` Jeff King
2021-09-28 23:49 ` Jeff King
` (2 more replies)
4 siblings, 3 replies; 40+ messages in thread
From: Jeff King @ 2021-09-28 23:40 UTC (permalink / raw)
To: Elijah Newren; +Cc: Junio C Hamano, Git Mailing List, Jonathan Nieder
On Mon, Sep 27, 2021 at 11:46:40PM -0700, Elijah Newren wrote:
> > * en/remerge-diff (2021-08-31) 7 commits
> > - doc/diff-options: explain the new --remerge-diff option
> > - show, log: provide a --remerge-diff capability
> > - tmp-objdir: new API for creating and removing primary object dirs
> > - merge-ort: capture and print ll-merge warnings in our preferred fashion
> > - ll-merge: add API for capturing warnings in a strbuf instead of stderr
> > - merge-ort: add ability to record conflict messages in a file
> > - merge-ort: mark a few more conflict messages as omittable
> >
> > A new presentation for two-parent merge "--remerge-diff" can be
> > used to show the difference between mechanical (and possibly
> > conflicted) merge results and the recorded resolution.
> >
> > Will merge to 'next'?
>
> It has been a month that it's been cooking with no issues brought up,
> and it's been in production for nearly a year...
>
> But just this morning I pinged peff and jrnieder if they might have
> time to respectively look at the tmp-objdir stuff (patch 5, plus its
> integration into log-tree.c in patch 7) and the ll-merge.[ch] changes
> (patch 3). I don't know if either will have time to do it, but
> perhaps wait half a week or so to see if they'll mention they have
> time? Otherwise, yeah, it's probably time to merge this down.
Sorry to take so long. I think this is a very exciting topic, and I
appreciate being called into to look at tmp-objdir stuff, because it can
be quite subtle.
I just left a rather long-ish mail in the thread, but the gist of it is
that I'm worried that there's some possibility of corruption there if
the diff code writes objects. I didn't do a proof-of-concept there, but
I worked one up just now. Try this:
# make a repo
git init repo
cd repo
echo base >file
git add file
git commit -m base
# two diverging branches
echo main >file
git commit -am main
git checkout -b side HEAD^
echo side >file
git commit -am side
# we get a conflict, and we resolve
git merge main
echo resolved >file
git commit -am merged
# now imagine we had a file with a diff driver. I stuffed it
# in here after the fact, but it could have been here all along,
# or come as part of the merge, etc.
echo whatever >unrelated
echo "unrelated diff=foo" >.gitattributes
git add .
git commit --amend --no-edit
# set up the diff driver not just to do a textconv, but to cache the
# result. This will require writing out new objects for the cache
# as part of the diff operation.
git config diff.foo.textconv "$PWD/upcase"
git config diff.foo.cachetextconv true
cat >upcase <<\EOF &&
#!/bin/sh
tr a-z A-Z <$1
EOF
chmod +x upcase
# now show the diff
git log -1 --remerge-diff
# and make sure the repo is still OK
git fsck
The remerge diff will correctly show the textconv'd content (because
it's not in either parent, and hence an evil merge):
diff --git a/unrelated b/unrelated
new file mode 100644
index 0000000..982793c
--- /dev/null
+++ b/unrelated
@@ -0,0 +1 @@
+WHATEVER
but then fsck shows that our cache is corrupted:
Checking object directories: 100% (256/256), done.
error: refs/notes/textconv/foo: invalid sha1 pointer 1e9b3ecffca73411001043fe914a7ec0e7291043
error: refs/notes/textconv/foo: invalid reflog entry 1e9b3ecffca73411001043fe914a7ec0e7291043
dangling tree 569b6e414203d2f50bdaafc1585eae11e28afc37
Now I'll admit the textconv-cache is a pretty seldom-used feature. And
that there _probably_ aren't a lot of other ways that the diff code
would try to write objects or references. But I think it speaks to the
kind of subtle interactions we should worry about (and certainly
corrupting the repository is a pretty bad outcome, though at least in
this case it's "just" a cache and we could blow away that ref manually).
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 23:40 ` Jeff King
@ 2021-09-28 23:49 ` Jeff King
2021-09-29 18:43 ` Neeraj Singh
2021-10-04 13:45 ` Elijah Newren
2 siblings, 0 replies; 40+ messages in thread
From: Jeff King @ 2021-09-28 23:49 UTC (permalink / raw)
To: Elijah Newren
Cc: Neeraj Singh, Junio C Hamano, Git Mailing List, Jonathan Nieder
On Tue, Sep 28, 2021 at 07:40:14PM -0400, Jeff King wrote:
> but then fsck shows that our cache is corrupted:
>
> Checking object directories: 100% (256/256), done.
> error: refs/notes/textconv/foo: invalid sha1 pointer 1e9b3ecffca73411001043fe914a7ec0e7291043
> error: refs/notes/textconv/foo: invalid reflog entry 1e9b3ecffca73411001043fe914a7ec0e7291043
> dangling tree 569b6e414203d2f50bdaafc1585eae11e28afc37
>
> Now I'll admit the textconv-cache is a pretty seldom-used feature. And
> that there _probably_ aren't a lot of other ways that the diff code
> would try to write objects or references. But I think it speaks to the
> kind of subtle interactions we should worry about (and certainly
> corrupting the repository is a pretty bad outcome, though at least in
> this case it's "just" a cache and we could blow away that ref manually).
I haven't looked at the way the bulk-fsync code uses tmp_objdir at all
yet, but this is the same kind of thing to be watching out for. I've
added Neeraj to the cc. The full reproduction showing the problem with
Elijah's patch is in the replied-to mail:
https://lore.kernel.org/git/YVOn3hDsb5pnxR53@coredump.intra.peff.net/
The batch-fsync stuff might be OK in that the goal there is presumably
not to throw away the result, but to eventually migrate it into the odb.
So the unintended interaction to look out for there might be more like:
- some code foo() wants to write a bunch of objects; it opens up a
tmp_objdir to batch them
- some other code bar() deep in the call-stack wants to write an
object for whatever reason (maybe it's a caching textconv). Its
object ends up in the tmp_objdir, too.
- bar() writes a ref pointing to the object's oid, thinking it has
fully written the object.
- foo() encounters an error and aborts, rolling back the tmp_objdir,
including the object written by bar(). Now our ref is corrupt.
As I said, I haven't looked at how the bulk-fsync stuff uses
tmp_objdir. But if it's doing the same "globally all object writes go to
the tmpdir" then it's at risk of this sort of thing.
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 23:40 ` Jeff King
2021-09-28 23:49 ` Jeff King
@ 2021-09-29 18:43 ` Neeraj Singh
2021-09-30 8:16 ` Jeff King
2021-10-04 13:45 ` Elijah Newren
2 siblings, 1 reply; 40+ messages in thread
From: Neeraj Singh @ 2021-09-29 18:43 UTC (permalink / raw)
To: Jeff King
Cc: Elijah Newren, Junio C Hamano, Git Mailing List, Jonathan Nieder
On Tue, Sep 28, 2021 at 07:40:14PM -0400, Jeff King wrote:
> On Mon, Sep 27, 2021 at 11:46:40PM -0700, Elijah Newren wrote:
>
> > > * en/remerge-diff (2021-08-31) 7 commits
> > > - doc/diff-options: explain the new --remerge-diff option
> > > - show, log: provide a --remerge-diff capability
> > > - tmp-objdir: new API for creating and removing primary object dirs
> > > - merge-ort: capture and print ll-merge warnings in our preferred fashion
> > > - ll-merge: add API for capturing warnings in a strbuf instead of stderr
> > > - merge-ort: add ability to record conflict messages in a file
> > > - merge-ort: mark a few more conflict messages as omittable
> > >
> > > A new presentation for two-parent merge "--remerge-diff" can be
> > > used to show the difference between mechanical (and possibly
> > > conflicted) merge results and the recorded resolution.
> > >
> > > Will merge to 'next'?
> >
> > It has been a month that it's been cooking with no issues brought up,
> > and it's been in production for nearly a year...
> >
> > But just this morning I pinged peff and jrnieder if they might have
> > time to respectively look at the tmp-objdir stuff (patch 5, plus its
> > integration into log-tree.c in patch 7) and the ll-merge.[ch] changes
> > (patch 3). I don't know if either will have time to do it, but
> > perhaps wait half a week or so to see if they'll mention they have
> > time? Otherwise, yeah, it's probably time to merge this down.
>
> Sorry to take so long. I think this is a very exciting topic, and I
> appreciate being called into to look at tmp-objdir stuff, because it can
> be quite subtle.
>
> I just left a rather long-ish mail in the thread, but the gist of it is
> that I'm worried that there's some possibility of corruption there if
> the diff code writes objects. I didn't do a proof-of-concept there, but
> I worked one up just now. Try this:
>
> # make a repo
> git init repo
> cd repo
> echo base >file
> git add file
> git commit -m base
>
> # two diverging branches
> echo main >file
> git commit -am main
> git checkout -b side HEAD^
> echo side >file
> git commit -am side
>
> # we get a conflict, and we resolve
> git merge main
> echo resolved >file
> git commit -am merged
>
> # now imagine we had a file with a diff driver. I stuffed it
> # in here after the fact, but it could have been here all along,
> # or come as part of the merge, etc.
> echo whatever >unrelated
> echo "unrelated diff=foo" >.gitattributes
> git add .
> git commit --amend --no-edit
>
> # set up the diff driver not just to do a textconv, but to cache the
> # result. This will require writing out new objects for the cache
> # as part of the diff operation.
> git config diff.foo.textconv "$PWD/upcase"
> git config diff.foo.cachetextconv true
> cat >upcase <<\EOF &&
> #!/bin/sh
> tr a-z A-Z <$1
> EOF
> chmod +x upcase
>
> # now show the diff
> git log -1 --remerge-diff
>
> # and make sure the repo is still OK
> git fsck
>
> The remerge diff will correctly show the textconv'd content (because
> it's not in either parent, and hence an evil merge):
>
> diff --git a/unrelated b/unrelated
> new file mode 100644
> index 0000000..982793c
> --- /dev/null
> +++ b/unrelated
> @@ -0,0 +1 @@
> +WHATEVER
>
> but then fsck shows that our cache is corrupted:
>
> Checking object directories: 100% (256/256), done.
> error: refs/notes/textconv/foo: invalid sha1 pointer 1e9b3ecffca73411001043fe914a7ec0e7291043
> error: refs/notes/textconv/foo: invalid reflog entry 1e9b3ecffca73411001043fe914a7ec0e7291043
> dangling tree 569b6e414203d2f50bdaafc1585eae11e28afc37
>
> Now I'll admit the textconv-cache is a pretty seldom-used feature. And
> that there _probably_ aren't a lot of other ways that the diff code
> would try to write objects or references. But I think it speaks to the
> kind of subtle interactions we should worry about (and certainly
> corrupting the repository is a pretty bad outcome, though at least in
> this case it's "just" a cache and we could blow away that ref manually).
>
> -Peff
It seems to me that one problem is that the new "primary" objdir code doesn't
disable ref updates since the GIT_QUARANTINE_ENVIRONMENT setting isn't set.
If we fix that we should be forbidding ref updates.
I've included a path that fixes my test case. This is on top of:
https://lore.kernel.org/git/CANQDOddqwVtWfC4eEP3fJB4sUiszGX8bLqoEVLcMf=v+jzx19g@mail.gmail.com/
From 38be7f6d31da8df3f205b35d25dd0a505aa75a8a Mon Sep 17 00:00:00 2001
From: Neeraj Singh <neerajsi@microsoft.com>
Date: Wed, 29 Sep 2021 11:24:05 -0700
Subject: [PATCH] tmp-objdir: disable ref updates when replacing the primary
odb
When creating a subprocess with a temporary ODB, we set the
GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not
to update refs, since the tmp-objdir may go away.
Introduce a similar mechanism for in-process temporary ODBs when
we call tmp_objdir_replace_primary_odb.
Peff's test case was invoking ref updates via the cachetextconv
setting. That particular code silently does nothing when a ref
update is forbidden
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
---
object-file.c | 7 +++++++
refs.c | 22 +++++++++++++++++++++-
refs.h | 5 +++++
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/object-file.c b/object-file.c
index 990381abee..e3e5cec3c8 100644
--- a/object-file.c
+++ b/object-file.c
@@ -770,6 +770,12 @@ struct object_directory *set_temporary_primary_odb(const char *dir, int will_des
new_odb->will_destroy = will_destroy;
new_odb->next = the_repository->objects->odb;
the_repository->objects->odb = new_odb;
+
+ /*
+ * Disable ref updates while a temporary odb is active, since
+ * the objects in the database may roll back.
+ */
+ refs_disable_updates();
return new_odb->next;
}
@@ -786,6 +792,7 @@ void restore_primary_odb(struct object_directory *restore_odb, const char *old_p
the_repository->objects->odb = restore_odb;
free_object_directory(cur_odb);
+ refs_enable_updates();
}
/*
diff --git a/refs.c b/refs.c
index 8b9f7c3a80..98026b7341 100644
--- a/refs.c
+++ b/refs.c
@@ -19,6 +19,8 @@
#include "repository.h"
#include "sigchain.h"
+static int ref_update_disabled_count;
+
/*
* List of all available backends
*/
@@ -1045,6 +1047,24 @@ void ref_transaction_free(struct ref_transaction *transaction)
free(transaction);
}
+void refs_disable_updates(void)
+{
+ ++ref_update_disabled_count;
+}
+
+void refs_enable_updates(void)
+{
+ if (!ref_update_disabled_count)
+ BUG("ref updates are not disabled");
+
+ --ref_update_disabled_count;
+}
+
+int ref_updates_are_enabled(void)
+{
+ return !ref_update_disabled_count;
+}
+
struct ref_update *ref_transaction_add_update(
struct ref_transaction *transaction,
const char *refname, unsigned int flags,
@@ -2126,7 +2146,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
break;
}
- if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+ if (getenv(GIT_QUARANTINE_ENVIRONMENT) || !ref_updates_are_enabled()) {
strbuf_addstr(err,
_("ref updates forbidden inside quarantine environment"));
return -1;
diff --git a/refs.h b/refs.h
index 48970dfc7e..acd7e275c5 100644
--- a/refs.h
+++ b/refs.h
@@ -840,6 +840,11 @@ int ref_storage_backend_exists(const char *name);
struct ref_store *get_main_ref_store(struct repository *r);
+/* Helpers to mark updates to the refs as forbidden */
+void refs_disable_updates(void);
+void refs_enable_updates(void);
+int ref_updates_are_enabled(void);
+
/**
* Submodules
* ----------
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-29 18:43 ` Neeraj Singh
@ 2021-09-30 8:16 ` Jeff King
2021-10-01 7:50 ` Elijah Newren
0 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2021-09-30 8:16 UTC (permalink / raw)
To: Neeraj Singh
Cc: Elijah Newren, Junio C Hamano, Git Mailing List, Jonathan Nieder
On Wed, Sep 29, 2021 at 11:43:39AM -0700, Neeraj Singh wrote:
> It seems to me that one problem is that the new "primary" objdir code doesn't
> disable ref updates since the GIT_QUARANTINE_ENVIRONMENT setting isn't set.
> If we fix that we should be forbidding ref updates.
>
> I've included a path that fixes my test case. This is on top of:
> https://lore.kernel.org/git/CANQDOddqwVtWfC4eEP3fJB4sUiszGX8bLqoEVLcMf=v+jzx19g@mail.gmail.com/
Ah, right, I forgot we had that "forbid ref updates in quarantine" magic
(despite being the one who added it).
I do think this improves the safety, but things are still a bit more
dangerous because we're handling it all in a single process, which sees
both the quarantine and non-quarantine states. I wrote more details in
this reply a few minutes ago:
https://lore.kernel.org/git/YVVmssXlaFM6yD5W@coredump.intra.peff.net/
(sorry, it's long; search for the paragraph starting with "Whereas doing
it in-process").
> When creating a subprocess with a temporary ODB, we set the
> GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not
> to update refs, since the tmp-objdir may go away.
>
> Introduce a similar mechanism for in-process temporary ODBs when
> we call tmp_objdir_replace_primary_odb.
>
> Peff's test case was invoking ref updates via the cachetextconv
> setting. That particular code silently does nothing when a ref
> update is forbidden
Oh good. I was worried that it would generate ugly errors, rather than
silently skipping the update.
> @@ -2126,7 +2146,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
> break;
> }
>
> - if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
> + if (getenv(GIT_QUARANTINE_ENVIRONMENT) || !ref_updates_are_enabled()) {
> strbuf_addstr(err,
> _("ref updates forbidden inside quarantine environment"));
> return -1;
I think the overall goal of this patch makes sense, but this
conditional, along with tmp-objdir reaching out to the refs code, feels
funny. Should we perhaps have a single:
int tmp_objdir_is_primary(struct repository *r)
{
if (the_tmp_objdir &&
!strcmp(the_tmp_objdir->path.buf, r->objects->odb->path))
return 1; /* our temporary is the primary */
if (getenv(GIT_QUARANTINE_PATH))
return 1; /* our caller put us in quarantine */
return 0;
}
Then it's all nicely abstracted and the ref code does not have to know
the details of GIT_QUARANTINE_PATH in the first place.
If you do got that route, the strcmp() might need to be a little more
careful about whether r->objects can be NULL (I didn't check).
Alternatively, I kind of wonder if "struct object_directory" should just
have a flag that says "is_temporary".
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-30 8:16 ` Jeff King
@ 2021-10-01 7:50 ` Elijah Newren
2021-10-01 17:02 ` Junio C Hamano
0 siblings, 1 reply; 40+ messages in thread
From: Elijah Newren @ 2021-10-01 7:50 UTC (permalink / raw)
To: Jeff King; +Cc: Neeraj Singh, Junio C Hamano, Git Mailing List, Jonathan Nieder
On Thu, Sep 30, 2021 at 1:16 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Sep 29, 2021 at 11:43:39AM -0700, Neeraj Singh wrote:
>
> > It seems to me that one problem is that the new "primary" objdir code doesn't
> > disable ref updates since the GIT_QUARANTINE_ENVIRONMENT setting isn't set.
> > If we fix that we should be forbidding ref updates.
> >
> > I've included a path that fixes my test case. This is on top of:
> > https://lore.kernel.org/git/CANQDOddqwVtWfC4eEP3fJB4sUiszGX8bLqoEVLcMf=v+jzx19g@mail.gmail.com/
>
> Ah, right, I forgot we had that "forbid ref updates in quarantine" magic
> (despite being the one who added it).
>
> I do think this improves the safety, but things are still a bit more
> dangerous because we're handling it all in a single process, which sees
> both the quarantine and non-quarantine states. I wrote more details in
> this reply a few minutes ago:
>
> https://lore.kernel.org/git/YVVmssXlaFM6yD5W@coredump.intra.peff.net/
>
> (sorry, it's long; search for the paragraph starting with "Whereas doing
> it in-process").
>
> > When creating a subprocess with a temporary ODB, we set the
> > GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not
> > to update refs, since the tmp-objdir may go away.
> >
> > Introduce a similar mechanism for in-process temporary ODBs when
> > we call tmp_objdir_replace_primary_odb.
> >
> > Peff's test case was invoking ref updates via the cachetextconv
> > setting. That particular code silently does nothing when a ref
> > update is forbidden
>
> Oh good. I was worried that it would generate ugly errors, rather than
> silently skipping the update.
>
> > @@ -2126,7 +2146,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
> > break;
> > }
> >
> > - if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
> > + if (getenv(GIT_QUARANTINE_ENVIRONMENT) || !ref_updates_are_enabled()) {
> > strbuf_addstr(err,
> > _("ref updates forbidden inside quarantine environment"));
> > return -1;
>
> I think the overall goal of this patch makes sense, but this
> conditional, along with tmp-objdir reaching out to the refs code, feels
> funny. Should we perhaps have a single:
>
> int tmp_objdir_is_primary(struct repository *r)
> {
> if (the_tmp_objdir &&
> !strcmp(the_tmp_objdir->path.buf, r->objects->odb->path))
> return 1; /* our temporary is the primary */
>
> if (getenv(GIT_QUARANTINE_PATH))
> return 1; /* our caller put us in quarantine */
>
> return 0;
> }
>
> Then it's all nicely abstracted and the ref code does not have to know
> the details of GIT_QUARANTINE_PATH in the first place.
>
> If you do got that route, the strcmp() might need to be a little more
> careful about whether r->objects can be NULL (I didn't check).
> Alternatively, I kind of wonder if "struct object_directory" should just
> have a flag that says "is_temporary".
Actually, wouldn't this be the safest approach, for my particular
usecase? By having the quarantine in place, no refs will be updated,
which removes the risk of new refs mentioning objects that will go
away. The only issue that could arise would be from new objects
referencing objects that will go away. But the new objects are also
written to the temporary object store when it remains the primary, and
thus those new objects will also be deleted when the temporary object
store is.
In contrast, moving the temporary object store to the end of the
alternates list would allow new objects to be written that reference
the objects that will go away. Using pretend_object_file() will also
allow new objects to be written that reference the objects that will
go away.
Said another way, I don't think anything should be writing a critical
file that needs to be durable when we're in the middle of a
"read-only" process like git-log. The only things written should be
temporary stuff, like the automatic remerge calculation from
merge-ort, the textconv cache optimization stuff, or perhaps future
gitattributes transformation caching. All that stuff can safely be
blown away at the completion of each merge.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-01 7:50 ` Elijah Newren
@ 2021-10-01 17:02 ` Junio C Hamano
2021-10-01 17:39 ` Neeraj Singh
2021-10-01 18:12 ` Elijah Newren
0 siblings, 2 replies; 40+ messages in thread
From: Junio C Hamano @ 2021-10-01 17:02 UTC (permalink / raw)
To: Elijah Newren; +Cc: Jeff King, Neeraj Singh, Git Mailing List, Jonathan Nieder
Elijah Newren <newren@gmail.com> writes:
> Said another way, I don't think anything should be writing a critical
> file that needs to be durable when we're in the middle of a
> "read-only" process like git-log. The only things written should be
> temporary stuff, like the automatic remerge calculation from
> merge-ort, the textconv cache optimization stuff, or perhaps future
> gitattributes transformation caching. All that stuff can safely be
> blown away at the completion of each merge.
The textconv cache can be populated/written during "git log -p" into
the object store to persist. With or without "--remerge-diff", we
can make design decision to either
- use temporary object store to discard everything we create at the
end in one-go, or
- write them into the object store to let later gc to handle the
crufts.
The former will disable persistent write access to the cache. It
still allows accesses the cached data during the same process,
though. We so far deemed that textconv cache, when the user enables
it, is valuable enough to make persistent. Perhaps remerge-diff's
tentative merge result may fall into the same category? Some folks
may want to cache, others may not.
If we were to use the same notes-cache mechanism, we record the tree
object (perhaps the object name) as the cached value for the merge
commit in question. Hopefully most of the merges are clean merges,
and "caching" the results of the recreation of the merge would cost
almost nothing. We need objects to record the fact that "this merge
has cached result" in the notes-cache, but the tree object that
represents the cached result is already in the history the merge
belongs to.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-01 17:02 ` Junio C Hamano
@ 2021-10-01 17:39 ` Neeraj Singh
2021-10-01 18:15 ` Elijah Newren
2021-10-01 18:12 ` Elijah Newren
1 sibling, 1 reply; 40+ messages in thread
From: Neeraj Singh @ 2021-10-01 17:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Jeff King, Git Mailing List, Jonathan Nieder
On Fri, Oct 1, 2021 at 10:02 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Said another way, I don't think anything should be writing a critical
> > file that needs to be durable when we're in the middle of a
> > "read-only" process like git-log. The only things written should be
> > temporary stuff, like the automatic remerge calculation from
> > merge-ort, the textconv cache optimization stuff, or perhaps future
> > gitattributes transformation caching. All that stuff can safely be
> > blown away at the completion of each merge.
>
> The textconv cache can be populated/written during "git log -p" into
> the object store to persist. With or without "--remerge-diff", we
> can make design decision to either
>
> - use temporary object store to discard everything we create at the
> end in one-go, or
>
> - write them into the object store to let later gc to handle the
> crufts.
>
> The former will disable persistent write access to the cache. It
> still allows accesses the cached data during the same process,
> though. We so far deemed that textconv cache, when the user enables
> it, is valuable enough to make persistent. Perhaps remerge-diff's
> tentative merge result may fall into the same category? Some folks
> may want to cache, others may not.
>
> If we were to use the same notes-cache mechanism, we record the tree
> object (perhaps the object name) as the cached value for the merge
> commit in question. Hopefully most of the merges are clean merges,
> and "caching" the results of the recreation of the merge would cost
> almost nothing. We need objects to record the fact that "this merge
> has cached result" in the notes-cache, but the tree object that
> represents the cached result is already in the history the merge
> belongs to.
>
Elijah,
To Junio's point, I'm also curious about why it's so important to
aggressively purge the
mechanical merge cache that you want to do it after each diff
operation rather than once
at the end.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-01 17:39 ` Neeraj Singh
@ 2021-10-01 18:15 ` Elijah Newren
0 siblings, 0 replies; 40+ messages in thread
From: Elijah Newren @ 2021-10-01 18:15 UTC (permalink / raw)
To: Neeraj Singh; +Cc: Junio C Hamano, Jeff King, Git Mailing List, Jonathan Nieder
On Fri, Oct 1, 2021 at 10:39 AM Neeraj Singh <nksingh85@gmail.com> wrote:
>
> On Fri, Oct 1, 2021 at 10:02 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Elijah Newren <newren@gmail.com> writes:
> >
> > > Said another way, I don't think anything should be writing a critical
> > > file that needs to be durable when we're in the middle of a
> > > "read-only" process like git-log. The only things written should be
> > > temporary stuff, like the automatic remerge calculation from
> > > merge-ort, the textconv cache optimization stuff, or perhaps future
> > > gitattributes transformation caching. All that stuff can safely be
> > > blown away at the completion of each merge.
> >
> > The textconv cache can be populated/written during "git log -p" into
> > the object store to persist. With or without "--remerge-diff", we
> > can make design decision to either
> >
> > - use temporary object store to discard everything we create at the
> > end in one-go, or
> >
> > - write them into the object store to let later gc to handle the
> > crufts.
> >
> > The former will disable persistent write access to the cache. It
> > still allows accesses the cached data during the same process,
> > though. We so far deemed that textconv cache, when the user enables
> > it, is valuable enough to make persistent. Perhaps remerge-diff's
> > tentative merge result may fall into the same category? Some folks
> > may want to cache, others may not.
> >
> > If we were to use the same notes-cache mechanism, we record the tree
> > object (perhaps the object name) as the cached value for the merge
> > commit in question. Hopefully most of the merges are clean merges,
> > and "caching" the results of the recreation of the merge would cost
> > almost nothing. We need objects to record the fact that "this merge
> > has cached result" in the notes-cache, but the tree object that
> > represents the cached result is already in the history the merge
> > belongs to.
> >
>
> Elijah,
> To Junio's point, I'm also curious about why it's so important to
> aggressively purge the
> mechanical merge cache that you want to do it after each diff
> operation rather than once
> at the end.
Performance; I don't want to accumulate hundreds of thousands of loose
objects and pay the penalty from them sitting around, especially when
I know they're cruft. (My original implementation from a year ago did
just clean out once at the end.)
For more details, see
https://lore.kernel.org/git/CABPp-BGsjq3ts4A6wKLYcopD9rknM+LXXi8qR_SLEpmU+x7KNQ@mail.gmail.com/,
starting with "It'd be _much_ slower". Or the email I just sent to
Junio.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-01 17:02 ` Junio C Hamano
2021-10-01 17:39 ` Neeraj Singh
@ 2021-10-01 18:12 ` Elijah Newren
2021-10-01 22:02 ` Junio C Hamano
1 sibling, 1 reply; 40+ messages in thread
From: Elijah Newren @ 2021-10-01 18:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Neeraj Singh, Git Mailing List, Jonathan Nieder
On Fri, Oct 1, 2021 at 10:02 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Said another way, I don't think anything should be writing a critical
> > file that needs to be durable when we're in the middle of a
> > "read-only" process like git-log. The only things written should be
> > temporary stuff, like the automatic remerge calculation from
> > merge-ort, the textconv cache optimization stuff, or perhaps future
> > gitattributes transformation caching. All that stuff can safely be
> > blown away at the completion of each merge.
>
> The textconv cache can be populated/written during "git log -p" into
> the object store to persist. With or without "--remerge-diff", we
> can make design decision to either
>
> - use temporary object store to discard everything we create at the
> end in one-go, or
>
> - write them into the object store to let later gc to handle the
> crufts.
>
> The former will disable persistent write access to the cache. It
> still allows accesses the cached data during the same process,
> though. We so far deemed that textconv cache, when the user enables
> it, is valuable enough to make persistent. Perhaps remerge-diff's
> tentative merge result may fall into the same category? Some folks
> may want to cache, others may not.
>
> If we were to use the same notes-cache mechanism, we record the tree
> object (perhaps the object name) as the cached value for the merge
> commit in question. Hopefully most of the merges are clean merges,
> and "caching" the results of the recreation of the merge would cost
> almost nothing. We need objects to record the fact that "this merge
> has cached result" in the notes-cache, but the tree object that
> represents the cached result is already in the history the merge
> belongs to.
I'm not sure this performance caching for remerge-diff makes any
sense, for multiple reasons:
1) Does the cache become invalidated when the merge algorithm changes
or when config options change (--remerge-diff shows the difference
between what an automatic merge would do, but an automatic merge's
results depend on the current algorithm and various merge.*, diff.*,
etc. config options the user can set). Do we re-cache each variant?
Do we expire the cache at some point?
2) The performance aspect seems suspect at best, and likely to backfire. Badly:
2a) Let's say there's 100,000 merge commits in your history (so small
history roughly the size of linux.git). `git log --remerge-diff` thus
repeats 100,000 merges, creating loose objects for them. Perhaps only
1 in 10 merges needs any loose objects created (because the rest of
the merges were clean). But those that do have conflicts will need to
create both new blobs and any necessary new trees that use those new
blobs. Who knows how many blobs and trees are needed, but let's just
use 10 total blobs+trees per merge that has conflicts as a guesstimate
of the average. Thus `git log --remerge-diff` will need to create
100,000 * 1/10 * 10 = 100,000 loose objects while it runs. Preserving
all those files to cache the results slows things down
considerably...until they are gc'ed. If we do the notes-cache thing,
then yes these objects would become packed. Perhaps you're suggested
doing an automatic gc (or several) as part of running --remerge-diff,
but that's super slow itself. When I implemented that, it felt worse
to me than just letting loose objects pile up.
2b) Note that my implementation didn't just clear out the tmp-objdir
at the _end_ of the remerge-diff (which was the easiest to implement),
but cleared it out after each merge, because that was much faster with
as many objects as can accumulate.
2c) Is --remerge-diff slow enough to even merit the complicated effort
of caching results? --remerge-diff is faster than --cc in my testing,
so why is it even worth the pain to cache?
So with that out of the way, let's return to discussing the textconv
cache. If the remerge-diff results aren't cached, isn't it unsafe to
allow the textconv cache to persist anything while remerge-diff is
running because it could create corruption? We could still let the
textconv cache persist results when remerge-diff is not in use, but I
think we _want_ anything written by the textconv cache during
remerge-diff to be thrown away because there's a risk it references a
temporary object created by remerge-diff that will be deleted.
With all that in mind, to me, it seems like using the tmp-objdir as
the primary object store in combination with the refs quarantine is
actually the safest solution for this usecase. Am I missing
something?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-01 18:12 ` Elijah Newren
@ 2021-10-01 22:02 ` Junio C Hamano
2021-10-01 23:05 ` Elijah Newren
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-10-01 22:02 UTC (permalink / raw)
To: Elijah Newren; +Cc: Jeff King, Neeraj Singh, Git Mailing List, Jonathan Nieder
Elijah Newren <newren@gmail.com> writes:
> So with that out of the way, let's return to discussing the textconv
> cache. If the remerge-diff results aren't cached, isn't it unsafe to
> allow the textconv cache to persist anything while remerge-diff is
> running because it could create corruption?
I do not think anybody involved in this thread thinks it is
practical to annotate each write_object_file() with "this is
temporary" vs "this is to persist", so it is a given that it would
be all-or-none. If we want write_object_file() called while we are
running remerge-diff to write to a temporary object store, we have
to accept any other write_object_file() called by somebody else,
like textconv cache, must become temporary.
It may be sufficient to plug ref updates (which would cover the
finialization of notes-cache used by the textconv cache) to avoid
corruption, but that might give us a pointless and unpleasant error
message, so it may be necessary to teach the notes-cache stuff to
allow getting existing cached data while disabling it to accept
cache updates.
Thanks.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-10-01 22:02 ` Junio C Hamano
@ 2021-10-01 23:05 ` Elijah Newren
0 siblings, 0 replies; 40+ messages in thread
From: Elijah Newren @ 2021-10-01 23:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Neeraj Singh, Git Mailing List, Jonathan Nieder
On Fri, Oct 1, 2021 at 3:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > So with that out of the way, let's return to discussing the textconv
> > cache. If the remerge-diff results aren't cached, isn't it unsafe to
> > allow the textconv cache to persist anything while remerge-diff is
> > running because it could create corruption?
>
> I do not think anybody involved in this thread thinks it is
> practical to annotate each write_object_file() with "this is
> temporary" vs "this is to persist", so it is a given that it would
> be all-or-none. If we want write_object_file() called while we are
> running remerge-diff to write to a temporary object store, we have
> to accept any other write_object_file() called by somebody else,
> like textconv cache, must become temporary.
Ok, good.
But is this write_object_file() specific? I think
pretend_object_file() has the same problem where the textconv cache
could reference a pretend_object_file() and thus write objects and/or
refs that become corrupt. (In particular, if the userdiff fires on
any of the new files created by the three-way content merges, then I
think we hit the exact same problem) So, I think as long as there are
any "temporary" objects being fed to diff, the textconv cache needs to
also be considered temporary. Or am I misunderstanding something?
> It may be sufficient to plug ref updates (which would cover the
> finialization of notes-cache used by the textconv cache) to avoid
> corruption, but that might give us a pointless and unpleasant error
> message, so it may be necessary to teach the notes-cache stuff to
> allow getting existing cached data while disabling it to accept
> cache updates.
All we need to do here is set the quarantine environment, as per
Neeraj's report[1], it already handles all of this:
"""
Peff's test case was invoking ref updates via the cachetextconv
setting. That particular code silently does nothing when a ref
update is forbidden
"""
Looking at the code, this appears to happen because
ref_transaction_prepare() doesn't print an error in the quarantine
environment, but returns one. That error is passed all the way up the
stack to notes_cache_write() and then to fill_textconv().
fill_textconv() is the first level in the stack that ignores the
return code of what it's calling, namely notes_cache_write(), which
means that things work fine. There's even a comment a few lines
stating, "ignore errors, as we might be in a readonly repository".
So, I think we're good as long as we ensure the quarantine environment
is setup.
[1] https://lore.kernel.org/git/20210929184339.GA19712@neerajsi-x1.localdomain/
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 23:40 ` Jeff King
2021-09-28 23:49 ` Jeff King
2021-09-29 18:43 ` Neeraj Singh
@ 2021-10-04 13:45 ` Elijah Newren
2 siblings, 0 replies; 40+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git Mailing List, Jonathan Nieder
On Tue, Sep 28, 2021 at 4:40 PM Jeff King <peff@peff.net> wrote:
>
> I just left a rather long-ish mail in the thread, but the gist of it is
> that I'm worried that there's some possibility of corruption there if
> the diff code writes objects. I didn't do a proof-of-concept there, but
> I worked one up just now. Try this:
Thanks for the testcase; it's very helpful.
Let's make it a little more interesting, though...
>
> # make a repo
> git init repo
> cd repo
> echo base >file
> git add file
> git commit -m base
>
> # two diverging branches
> echo main >file
> git commit -am main
> git checkout -b side HEAD^
> echo side >file
> git commit -am side
>
> # we get a conflict, and we resolve
> git merge main
> echo resolved >file
> git commit -am merged
>
> # now imagine we had a file with a diff driver. I stuffed it
> # in here after the fact, but it could have been here all along,
> # or come as part of the merge, etc.
> echo whatever >unrelated
> echo "unrelated diff=foo" >.gitattributes
If we change this line to:
echo "file diff=foo" >.gitattributes
Then the userdiff will fire on the file involved in the remerge-diff,
including on the file included in the automatic 3-way content
conflict, giving us a diff that looks like this:
diff --git a/file b/file
index 5c66bc5..2ab19ae 100644
--- a/file
+++ b/file
@@ -1,7 +1 @@
-<<<<<<< 7D4147C (SIDE)
-SIDE
-||||||| BE54C04
-BASE
-=======
-MAIN
->>>>>>> 03FC4E3 (MAIN)
+RESOLVED
> git add .
> git commit --amend --no-edit
>
> # set up the diff driver not just to do a textconv, but to cache the
> # result. This will require writing out new objects for the cache
> # as part of the diff operation.
> git config diff.foo.textconv "$PWD/upcase"
> git config diff.foo.cachetextconv true
> cat >upcase <<\EOF &&
> #!/bin/sh
> tr a-z A-Z <$1
> EOF
> chmod +x upcase
>
> # now show the diff
> git log -1 --remerge-diff
>
> # and make sure the repo is still OK
> git fsck
>
> The remerge diff will correctly show the textconv'd content (because
> it's not in either parent, and hence an evil merge):
>
> diff --git a/unrelated b/unrelated
> new file mode 100644
> index 0000000..982793c
> --- /dev/null
> +++ b/unrelated
> @@ -0,0 +1 @@
> +WHATEVER
>
> but then fsck shows that our cache is corrupted:
>
> Checking object directories: 100% (256/256), done.
> error: refs/notes/textconv/foo: invalid sha1 pointer 1e9b3ecffca73411001043fe914a7ec0e7291043
> error: refs/notes/textconv/foo: invalid reflog entry 1e9b3ecffca73411001043fe914a7ec0e7291043
> dangling tree 569b6e414203d2f50bdaafc1585eae11e28afc37
> Now I'll admit the textconv-cache is a pretty seldom-used feature. And
> that there _probably_ aren't a lot of other ways that the diff code
> would try to write objects or references. But I think it speaks to the
> kind of subtle interactions we should worry about (and certainly
> corrupting the repository is a pretty bad outcome, though at least in
> this case it's "just" a cache and we could blow away that ref manually).
I implemented the pretend_object_file() solution on Saturday and tried
this out, and found with the above change that it has corruption
problems as well. This really doesn't feel much safer too me.
The move-the-tmp-objdir-to-just-being-an-alternate-instead-of-a-primary
solution, as suggested at
https://lore.kernel.org/git/YVOiggCWAdZcxAb6@coredump.intra.peff.net/
has the same problem.
But, more importantly, neither of these solutions can be made safe.
Even if we adopt the GIT_QUARANTINE_PATH suggestion from Neeraj
(https://lore.kernel.org/git/20210929184339.GA19712@neerajsi-x1.localdomain/),
we still have objects referencing now-missing objects.
*However*, if we use the tmp-objdir-as-primary solution, and leave it
as a primary, and use your modification to the GIT_QUARANTINE_PATH
idea from Neeraj (i.e. create a "the-tmp-objdir" and have the refs
code check for it and turn on the quarantine automatically when one
exists), then we prevent adding or modifying refs to point to bad
objects, and any new objects that textconv might write that could
depend on transient objects, will themselves be transient and go away.
So, this solution is safe from corruption. I'll be implementing it.
(not sure how quickly I'll get it done, as my time is mostly limited
to early mornings or evenings or Saturdays right now...)
^ permalink raw reply related [flat|nested] 40+ messages in thread
* da/difftool (was: Re: What's cooking in git.git (Sep 2021, #08; Mon, 27))
2021-09-28 0:52 What's cooking in git.git (Sep 2021, #08; Mon, 27) Junio C Hamano
2021-09-28 1:57 ` Ævar Arnfjörð Bjarmason
2021-09-28 6:46 ` Elijah Newren
@ 2021-09-28 8:22 ` Ævar Arnfjörð Bjarmason
2021-09-28 8:23 ` ns/batched-fsync & en/remerge-diff (was " Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-28 8:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Sep 27 2021, Junio C Hamano wrote:
> * da/difftool (2021-09-23) 4 commits
> - difftool: add a missing space to the run_dir_diff() comments
> - difftool: create a tmpdir path without repeated slashes
> - Merge branch 'da/difftool-dir-diff-symlink-fix' into da/difftool
> - Merge branch 'ab/retire-option-argument' into da/difftool
> (this branch uses da/difftool-dir-diff-symlink-fix.)
>
> Code clean-up in "git difftool".
>
> Will merge to 'next'?
I think so, I looked it over and left a comment about a weird edge case
in the pre-image to do with negative return codes, but all of the logic
seems OK to me.
^ permalink raw reply [flat|nested] 40+ messages in thread* ns/batched-fsync & en/remerge-diff (was Re: What's cooking in git.git (Sep 2021, #08; Mon, 27))
2021-09-28 0:52 What's cooking in git.git (Sep 2021, #08; Mon, 27) Junio C Hamano
` (2 preceding siblings ...)
2021-09-28 8:22 ` da/difftool (was: Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)) Ævar Arnfjörð Bjarmason
@ 2021-09-28 8:23 ` Ævar Arnfjörð Bjarmason
2021-09-28 8:31 ` sg/test-split-index-fix " Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
6 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-28 8:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Neeraj K. Singh, Elijah Newren
On Mon, Sep 27 2021, Junio C Hamano wrote:
> * ns/batched-fsync (2021-09-27) 8 commits
> - core.fsyncobjectfiles: performance tests for add and stash
> - core.fsyncobjectfiles: tests for batch mode
> - unpack-objects: use the bulk-checkin infrastructure
> - update-index: use the bulk-checkin infrastructure
> - core.fsyncobjectfiles: add windows support for batch mode
> - core.fsyncobjectfiles: batched disk flushes
> - bulk-checkin: rename 'state' variable and separate 'plugged' boolean
> - object-file.c: do not rename in a temp odb
>
> The "core.fsyncobjectfiles" configuration variable can now be set
> to "batch" for improved performance.
>
> Will merge to 'next'?
This version was significantly re-rolled in response to my feedback, I
haven't had time to look at this latest version in detail, but will try
to do so soon.
I did note in
https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/ that it
one of Elijah's in-flight seem to have independently come up with
slightly incompatible versions of the same tmp-objdir.c API....
> * en/remerge-diff (2021-08-31) 7 commits
> - doc/diff-options: explain the new --remerge-diff option
> - show, log: provide a --remerge-diff capability
> - tmp-objdir: new API for creating and removing primary object dirs
> - merge-ort: capture and print ll-merge warnings in our preferred fashion
> - ll-merge: add API for capturing warnings in a strbuf instead of stderr
> - merge-ort: add ability to record conflict messages in a file
> - merge-ort: mark a few more conflict messages as omittable
>
> A new presentation for two-parent merge "--remerge-diff" can be
> used to show the difference between mechanical (and possibly
> conflicted) merge results and the recorded resolution.
>
> Will merge to 'next'?
...i.e. this series
^ permalink raw reply [flat|nested] 40+ messages in thread* sg/test-split-index-fix (was Re: What's cooking in git.git (Sep 2021, #08; Mon, 27))
2021-09-28 0:52 What's cooking in git.git (Sep 2021, #08; Mon, 27) Junio C Hamano
` (3 preceding siblings ...)
2021-09-28 8:23 ` ns/batched-fsync & en/remerge-diff (was " Ævar Arnfjörð Bjarmason
@ 2021-09-28 8:31 ` Ævar Arnfjörð Bjarmason
2021-09-28 8:35 ` hn/reftable (Re: " Ævar Arnfjörð Bjarmason
2021-09-29 8:12 ` What's cooking in git.git (Sep 2021, #08; Mon, 27) Fabian Stelzer
6 siblings, 0 replies; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-28 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Sep 27 2021, Junio C Hamano wrote:
> * sg/test-split-index-fix (2021-09-07) 7 commits
> - read-cache: fix GIT_TEST_SPLIT_INDEX
> - tests: disable GIT_TEST_SPLIT_INDEX for sparse index tests
> - read-cache: look for shared index files next to the index, too
> - t1600-index: disable GIT_TEST_SPLIT_INDEX
> - t1600-index: don't run git commands upstream of a pipe
> - t1600-index: remove unnecessary redirection
> - Merge branch 'ds/sparse-index-ignored-files' into sg/test-split-index-fix
>
> Test updates.
>
> Will merge to 'next'?
It LGTM. It introduces a "new" memory leak that I re-rolled
ab/sanitize-leak-ci for, but on closer inspection it's a leak that was
there all along, but we didn't see due to the issue the series fixes:
The GIT_TEST_SPLIT_INDEX test mode was broken.
^ permalink raw reply [flat|nested] 40+ messages in thread* hn/reftable (Re: What's cooking in git.git (Sep 2021, #08; Mon, 27))
2021-09-28 0:52 What's cooking in git.git (Sep 2021, #08; Mon, 27) Junio C Hamano
` (4 preceding siblings ...)
2021-09-28 8:31 ` sg/test-split-index-fix " Ævar Arnfjörð Bjarmason
@ 2021-09-28 8:35 ` Ævar Arnfjörð Bjarmason
2021-09-28 12:18 ` Han-Wen Nienhuys
2021-09-29 8:12 ` What's cooking in git.git (Sep 2021, #08; Mon, 27) Fabian Stelzer
6 siblings, 1 reply; 40+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-28 8:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Han-Wen Nienhuys, Carlo Marcelo Arenas Belón
On Mon, Sep 27 2021, Junio C Hamano wrote:
> * hn/reftable (2021-09-10) 20 commits
> - fixup! reftable: implement stack, a mutable database of reftable files.
> - Add "test-tool dump-reftable" command.
> - reftable: add dump utility
> - reftable: implement stack, a mutable database of reftable files.
> - reftable: implement refname validation
> - reftable: add merged table view
> - reftable: add a heap-based priority queue for reftable records
> - reftable: reftable file level tests
> - reftable: read reftable files
> - reftable: generic interface to tables
> - reftable: write reftable files
> - reftable: a generic binary tree implementation
> - reftable: reading/writing blocks
> - Provide zlib's uncompress2 from compat/zlib-compat.c
> - reftable: (de)serialization for the polymorphic record type.
> - reftable: add blocksource, an abstraction for random access reads
> - reftable: utility functions
> - reftable: add error related functionality
> - reftable: RFC: add LICENSE
> - hash.h: provide constants for the hash IDs
>
> The "reftable" backend for the refs API, without integrating into
> the refs subsystem.
>
> Will merge to 'next'?
I think we've reached approximately "good enough" with this for the next
steps, and can hopefully fix any remaining nits (such as my [1])
post-merge.
Maybe hold off until Han-Wen gets a chance to ack it, and whether he's
ok with the proposed fixup(s)?
Wasn't there an outstanding "some of this in reftable/* should be static
functions" from someone, Carlo? (CC'd). In any case that sort of thing
could also be a post-cleanup, I couldn't find a reference to that
discussion in anything except my vague memory of it as I wrote this.
1. https://lore.kernel.org/git/87wnn62nhp.fsf@evledraar.gmail.com/
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: hn/reftable (Re: What's cooking in git.git (Sep 2021, #08; Mon, 27))
2021-09-28 8:35 ` hn/reftable (Re: " Ævar Arnfjörð Bjarmason
@ 2021-09-28 12:18 ` Han-Wen Nienhuys
2021-09-30 5:06 ` Carlo Arenas
0 siblings, 1 reply; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-09-28 12:18 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, git, Carlo Marcelo Arenas Belón
On Tue, Sep 28, 2021 at 10:38 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > * hn/reftable (2021-09-10) 20 commits
> > - fixup! reftable: implement stack, a mutable database of reftable files.
> > - Add "test-tool dump-reftable" command.
> > - reftable: add dump utility
> > - reftable: implement stack, a mutable database of reftable files.
> > - reftable: implement refname validation
> > - reftable: add merged table view
> > - reftable: add a heap-based priority queue for reftable records
> > - reftable: reftable file level tests
> > - reftable: read reftable files
> > - reftable: generic interface to tables
> > - reftable: write reftable files
> > - reftable: a generic binary tree implementation
> > - reftable: reading/writing blocks
> > - Provide zlib's uncompress2 from compat/zlib-compat.c
> > - reftable: (de)serialization for the polymorphic record type.
> > - reftable: add blocksource, an abstraction for random access reads
> > - reftable: utility functions
> > - reftable: add error related functionality
> > - reftable: RFC: add LICENSE
> > - hash.h: provide constants for the hash IDs
> >
> > The "reftable" backend for the refs API, without integrating into
> > the refs subsystem.
> >
> > Will merge to 'next'?
>
> I think we've reached approximately "good enough" with this for the next
> steps, and can hopefully fix any remaining nits (such as my [1])
> post-merge.
There is still an RFC in 02/19. Maybe we can get agreement that this
is OK and drop the RFC ?
> Maybe hold off until Han-Wen gets a chance to ack it, and whether he's
> ok with the proposed fixup(s)?
regarding uncompress2: I thought it was functionality best left to
zlib to implement, but I imagine git.git offers something similar.
Sounds good to use that.
> Wasn't there an outstanding "some of this in reftable/* should be static
> functions" from someone, Carlo? (CC'd). In any case that sort of thing
> could also be a post-cleanup, I couldn't find a reference to that
> discussion in anything except my vague memory of it as I wrote this.
I think I have addressed all outstanding issues in my github PR, and
I'll send it out once I see CI pass.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: hn/reftable (Re: What's cooking in git.git (Sep 2021, #08; Mon, 27))
2021-09-28 12:18 ` Han-Wen Nienhuys
@ 2021-09-30 5:06 ` Carlo Arenas
0 siblings, 0 replies; 40+ messages in thread
From: Carlo Arenas @ 2021-09-30 5:06 UTC (permalink / raw)
To: Han-Wen Nienhuys
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, git, ramsay
On Tue, Sep 28, 2021 at 5:19 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> On Tue, Sep 28, 2021 at 10:38 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > > * hn/reftable (2021-09-10) 20 commits
> > > - fixup! reftable: implement stack, a mutable database of reftable files.
> > > - Add "test-tool dump-reftable" command.
> > > - reftable: add dump utility
> > > - reftable: implement stack, a mutable database of reftable files.
> > > - reftable: implement refname validation
> > > - reftable: add merged table view
> > > - reftable: add a heap-based priority queue for reftable records
> > > - reftable: reftable file level tests
> > > - reftable: read reftable files
> > > - reftable: generic interface to tables
> > > - reftable: write reftable files
> > > - reftable: a generic binary tree implementation
> > > - reftable: reading/writing blocks
> > > - Provide zlib's uncompress2 from compat/zlib-compat.c
> > > - reftable: (de)serialization for the polymorphic record type.
> > > - reftable: add blocksource, an abstraction for random access reads
> > > - reftable: utility functions
> > > - reftable: add error related functionality
> > > - reftable: RFC: add LICENSE
> > > - hash.h: provide constants for the hash IDs
> > >
> > > The "reftable" backend for the refs API, without integrating into
> > > the refs subsystem.
> > >
> > > Will merge to 'next'?
> >
> > I think we've reached approximately "good enough" with this for the next
> > steps, and can hopefully fix any remaining nits (such as my [1])
> > post-merge.
>
> There is still an RFC in 02/19. Maybe we can get agreement that this
> is OK and drop the RFC ?
>
> > Maybe hold off until Han-Wen gets a chance to ack it, and whether he's
> > ok with the proposed fixup(s)?
>
> regarding uncompress2: I thought it was functionality best left to
> zlib to implement, but I imagine git.git offers something similar.
> Sounds good to use that.
>
> > Wasn't there an outstanding "some of this in reftable/* should be static
> > functions" from someone, Carlo? (CC'd). In any case that sort of thing
> > could also be a post-cleanup, I couldn't find a reference to that
> > discussion in anything except my vague memory of it as I wrote this.
>
> I think I have addressed all outstanding issues in my github PR, and
> I'll send it out once I see CI pass.
It was actually raised by Ramsay[1], and might have been a red herring,
but there was for sure no follow up.
I have a few minor fixups that will post for discussion, and might be even
ok to merge to next without squashing if included on top, but I am not
sure if all those issues could be addressed post-cleanup, since IMHO
once this gets into master, then it will be considered a stable API
and it can't be changed.
For example. while looking at Ramsay message noticed the API for "reader"
in libreftable might be better with s/init_reader/reader_init/g
Carlo
[1] https://lore.kernel.org/git/bc387e32-321e-4726-2a02-2e6cf6ed5250@ramsayjones.plus.com/
CC +Ramsay
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-28 0:52 What's cooking in git.git (Sep 2021, #08; Mon, 27) Junio C Hamano
` (5 preceding siblings ...)
2021-09-28 8:35 ` hn/reftable (Re: " Ævar Arnfjörð Bjarmason
@ 2021-09-29 8:12 ` Fabian Stelzer
2021-09-30 21:26 ` Junio C Hamano
6 siblings, 1 reply; 40+ messages in thread
From: Fabian Stelzer @ 2021-09-29 8:12 UTC (permalink / raw)
To: Junio C Hamano, git
On 28.09.21 02:52, Junio C Hamano wrote > * fs/ssh-signing (2021-09-10)
9 commits
> - ssh signing: test that gpg fails for unknown keys
> - ssh signing: tests for logs, tags & push certs
> - ssh signing: duplicate t7510 tests for commits
> - ssh signing: verify signatures using ssh-keygen
> - ssh signing: provide a textual signing_key_id
> - ssh signing: retrieve a default key from ssh-agent
> - ssh signing: add ssh key format and signing code
> - ssh signing: add test prereqs
> - ssh signing: preliminary refactoring and clean-up
>
> Use ssh public crypto for object and push-cert signing.
>
> On hold.
> cf. <pull.1041.v8.git.git.1631304462.gitgitgadget@gmail.com>
> cf. <532d97e7-8c91-df6a-6d90-70668256f513@gigacodes.de>
>
>
Openssh 8.8 has been released a few days ago and includes the needed fix
for the find-principal segfault.
I ran the full git testsuite against it without issues.
Also, we (~30developers) have been running this patch with
openssh-portable (2d678c5e3bdc2f5c99f7af5122e9d054925d560d / post 8.7 -
pre 8.8) in our organization for the last 2 weeks without problems.
The only issues we saw with our users are related to some misleading
openssh error messages.
For example if you configure a public key and the private key is not
available via the ssh-agent the error message is: "invalid format".
Or if the public key contains a typo (forgot a char in copy&pase) it
will error with "no such file or directory".
I will need to dig a bit deeper into openssh to see if we can make these
more specific without breaking any compatibility. Both errors originate
from some lower level lib functions which i don't want to change.
But vverall i think this is ready for some broader usage/testing via next.
I'd suggest to send the additional patches for valid-before/after
functionality in a new patchset for review after.
Best regards,
Fabian
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)
2021-09-29 8:12 ` What's cooking in git.git (Sep 2021, #08; Mon, 27) Fabian Stelzer
@ 2021-09-30 21:26 ` Junio C Hamano
0 siblings, 0 replies; 40+ messages in thread
From: Junio C Hamano @ 2021-09-30 21:26 UTC (permalink / raw)
To: Fabian Stelzer; +Cc: git
Fabian Stelzer <fs@gigacodes.de> writes:
> On 28.09.21 02:52, Junio C Hamano wrote > * fs/ssh-signing (2021-09-10)
> 9 commits
>> - ssh signing: test that gpg fails for unknown keys
>> - ssh signing: tests for logs, tags & push certs
>> - ssh signing: duplicate t7510 tests for commits
>> - ssh signing: verify signatures using ssh-keygen
>> - ssh signing: provide a textual signing_key_id
>> - ssh signing: retrieve a default key from ssh-agent
>> - ssh signing: add ssh key format and signing code
>> - ssh signing: add test prereqs
>> - ssh signing: preliminary refactoring and clean-up
>>
>> Use ssh public crypto for object and push-cert signing.
>>
>> On hold.
>> cf. <pull.1041.v8.git.git.1631304462.gitgitgadget@gmail.com>
>> cf. <532d97e7-8c91-df6a-6d90-70668256f513@gigacodes.de>
>>
>>
> Openssh 8.8 has been released a few days ago and includes the needed fix
> for the find-principal segfault.
> I ran the full git testsuite against it without issues.
>
> Also, we (~30developers) have been running this patch with
> openssh-portable (2d678c5e3bdc2f5c99f7af5122e9d054925d560d / post 8.7 -
> pre 8.8) in our organization for the last 2 weeks without problems.
> ...
> But vverall i think this is ready for some broader usage/testing via next.
>
> I'd suggest to send the additional patches for valid-before/after
> functionality in a new patchset for review after.
OK, so with the new OpenSSH, there wasn't anything that contradicts
what the above set of patches wanted to do and the series is good to
go? Let's merge it to 'next' for wider audience, then.
Thanks.
^ permalink raw reply [flat|nested] 40+ messages in thread