* What's cooking in git.git (Jan 2023, #07; Thu, 26)
From: Junio C Hamano @ 2023-01-26 22:54 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a future
release). Commits prefixed with '-' are only in 'seen', and aren't
considered "accepted" at all. A topic without enough support may be
discarded after a long period of no activity.
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* ab/cache-api-cleanup (2023-01-16) 5 commits
(merged to 'next' on 2023-01-16 at a0f388b149)
+ cache API: add a "INDEX_STATE_INIT" macro/function, add release_index()
+ read-cache.c: refactor set_new_index_sparsity() for subsequent commit
+ sparse-index API: BUG() out on NULL ensure_full_index()
+ sparse-index.c: expand_to_path() can assume non-NULL "istate"
+ builtin/difftool.c: { 0 }-initialize rather than using memset()
(this branch is used by ab/cache-api-cleanup-users.)
Code clean-up to tighten the use of in-core index in the API.
source: <cover-v2-0.6-00000000000-20230112T124842Z-avarab@gmail.com>
* ab/test-env-helper (2023-01-14) 1 commit
(merged to 'next' on 2023-01-16 at 82c17f02e5)
+ env-helper: move this built-in to "test-tool env-helper"
Remove "git env--helper" and demote it to a test-tool subcommand.
source: <patch-1.1-e662c570f1d-20230112T155226Z-avarab@gmail.com>
* ds/omit-trailing-hash-in-index (2023-01-17) 1 commit
(merged to 'next' on 2023-01-17 at 8dde3cf2db)
+ t1600: fix racy index.skipHash test
(this branch is used by ab/cache-api-cleanup-users.)
Quickfix for a topic already in 'master'.
source: <76204710-356a-2a85-9057-302e6619b9df@github.com>
* en/t6426-todo-cleanup (2023-01-14) 1 commit
(merged to 'next' on 2023-01-16 at 7d13842eeb)
+ t6426: fix TODO about making test more comprehensive
Test clean-up.
source: <pull.1462.v2.git.1673722187025.gitgitgadget@gmail.com>
* jc/format-patch-v-unleak (2023-01-16) 1 commit
(merged to 'next' on 2023-01-16 at 2155d512bc)
+ format-patch: unleak "-v <num>"
Plug a small leak.
source: <xmqqv8l8gr6s.fsf@gitster.g>
* kn/attr-from-tree (2023-01-14) 2 commits
(merged to 'next' on 2023-01-16 at 426f357683)
+ attr: add flag `--source` to work with tree-ish
+ t0003: move setup for `--all` into new block
"git check-attr" learned to take an optional tree-ish to read the
.gitattributes file from.
source: <cover.1673684790.git.karthik.188@gmail.com>
* rs/ls-tree-path-expansion-fix (2023-01-14) 2 commits
(merged to 'next' on 2023-01-16 at 6359f28ba7)
+ ls-tree: remove dead store and strbuf for quote_c_style()
+ ls-tree: fix expansion of repeated %(path)
"git ls-tree --format='%(path) %(path)' $tree $path" showed the
path three times, which has been corrected.
source: <55ae7333-3a13-0575-93ed-f858a1c2877e@web.de>
* rs/use-enhanced-bre-on-macos (2023-01-08) 1 commit
(merged to 'next' on 2023-01-16 at 9b80d4253f)
+ use enhanced basic regular expressions on macOS
Newer regex library macOS stopped enabling GNU-like enhanced BRE,
where '\(A\|B\)' works as alternation, unless explicitly asked with
the REG_ENHANCED flag. "git grep" now can be compiled to do so, to
retain the old behaviour.
source: <26a0d4ca-3d97-ace4-1a1f-92b1ee6715a6@web.de>
* sk/win32-close-handle-upon-pthread-join (2023-01-04) 2 commits
(merged to 'next' on 2023-01-16 at faa279fd5b)
+ win32: close handles of threads that have been joined
+ win32: prepare pthread.c for change by formatting
Pthread emulation on Win32 leaked thread handle when a thread is
joined.
source: <pull.1406.v13.git.git.1672762819.gitgitgadget@gmail.com>
* zh/scalar-progress (2023-01-16) 1 commit
(merged to 'next' on 2023-01-17 at d4c25cc71f)
+ scalar: show progress if stderr refers to a terminal
"scalar" learned to give progress bar.
source: <pull.1441.v3.git.1673442860379.gitgitgadget@gmail.com>
--------------------------------------------------
[New Topics]
* ar/markup-em-dash (2023-01-23) 1 commit
(merged to 'next' on 2023-01-24 at 0367e3035f)
+ Documentation: render dash correctly
Doc mark-up updates.
Will merge to 'master'.
source: <20230123090114.429844-1-rybak.a.v@gmail.com>
* ab/hook-api-with-stdin (2023-01-23) 5 commits
- hook: support a --to-stdin=<path> option for testing
- sequencer: use the new hook API for the simpler "post-rewrite" call
- hook API: support passing stdin to hooks, convert am's 'post-rewrite'
- run-command: allow stdin for run_processes_parallel
- run-command.c: remove dead assignment in while-loop
Extend the run-hooks API to allow feeding data from the standard
input when running the hook script(s).
Expecting review responses.
source: <cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com>
* as/ssh-signing-improve-key-missing-error (2023-01-25) 1 commit
(merged to 'next' on 2023-01-25 at 140f2c2c60)
+ ssh signing: better error message when key not in agent
Improve the error message given when private key is not loaded in
the ssh agent in the codepath to sign with an ssh key.
Will merge to 'master'.
source: <pull.1270.v3.git.git.1674650450662.gitgitgadget@gmail.com>
* en/rebase-incompatible-opts (2023-01-25) 10 commits
- rebase: provide better error message for apply options vs. merge config
- rebase: put rebase_options initialization in single place
- rebase: fix formatting of rebase --reapply-cherry-picks option in docs
- rebase: clarify the OPT_CMDMODE incompatibilities
- rebase: add coverage of other incompatible options
- rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
- rebase: fix docs about incompatibilities with --root
- rebase: remove --allow-empty-message from incompatible opts
- rebase: flag --apply and --merge as incompatible
- rebase: mark --update-refs as requiring the merge backend
"git rebase" often ignored incompatible options instead of
complaining, which has been corrected.
Will merge to 'next'.
Replaces en/rebase-update-refs-needs-merge-backend.
source: <pull.1466.v5.git.1674619434.gitgitgadget@gmail.com>
* gm/request-pull-with-non-pgp-signed-tags (2023-01-25) 1 commit
- request-pull: filter out SSH/X.509 tag signatures
source: <20230125234725.3918563-1-gwymor@tilde.club>
--------------------------------------------------
[Stalled]
* ja/worktree-orphan (2023-01-13) 4 commits
- worktree add: add hint to direct users towards --orphan
- worktree add: add --orphan flag
- worktree add: refactor opt exclusion tests
- worktree add: include -B in usage docs
'git worktree add' learned how to create a worktree based on an
orphaned branch with `--orphan`.
Expecting a reroll.
cf. <11be1b0e-ee38-119f-1d80-cb818946116b@dunelm.org.uk>
source: <20230109173227.29264-1-jacobabel@nullpo.dev>
* ab/avoid-losing-exit-codes-in-tests (2022-12-20) 6 commits
- tests: don't lose misc "git" exit codes
- tests: don't lose "git" exit codes in "! ( git ... | grep )"
- tests: don't lose exit status with "test <op> $(git ...)"
- tests: don't lose exit status with "(cd ...; test <op> $(git ...))"
- t/lib-patch-mode.sh: fix ignored exit codes
- auto-crlf tests: don't lose exit code in loops and outside tests
Test clean-up.
Expecting a hopefully minor and final reroll.
cf. <1182283a-4a78-3c99-e716-a8c3e58a5823@web.de>
cf. <xmqqsfhb0vum.fsf@gitster.g>
source: <cover-v4-0.6-00000000000-20221219T101240Z-avarab@gmail.com>
* tl/notes--blankline (2022-11-09) 5 commits
- notes.c: introduce "--no-blank-line" option
- notes.c: provide tips when target and append note are both empty
- notes.c: drop unreachable code in 'append_edit()'
- notes.c: cleanup for "designated init" and "char ptr init"
- notes.c: cleanup 'strbuf_grow' call in 'append_edit'
'git notes append' was taught '--[no-]blank-line' to conditionally
add a LF between a new and existing note.
Expecting a reroll.
cf. <CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com>
source: <cover.1667980450.git.dyroneteng@gmail.com>
* mc/switch-advice (2022-11-09) 1 commit
- po: use `switch` over `checkout` in error message
Use 'switch' instead of 'checkout' in an error message.
Waiting for review response.
source: <pull.1308.git.git.1668018620148.gitgitgadget@gmail.com>
* js/range-diff-mbox (2022-11-23) 1 commit
- range-diff: support reading mbox files
'git range-diff' gained support for reading either side from an .mbox
file instead of a revision range.
Waiting for review response.
cf. <xmqqr0xupmnf.fsf@gitster.g>
source: <pull.1420.v3.git.1669108102092.gitgitgadget@gmail.com>
* ab/tag-object-type-errors (2022-11-22) 5 commits
- tag: don't emit potentially incorrect "object is a X, not a Y"
- tag: don't misreport type of tagged objects in errors
- object tests: add test for unexpected objects in tags
- object-file.c: free the "t.tag" in check_tag()
- Merge branch 'jk/parse-object-type-mismatch' into ab/tag-object-type-errors
Hardening checks around mismatched object types when one of those
objects is a tag.
Expecting a reroll.
cf. <xmqqzgb5jz5c.fsf@gitster.g>
cf. <xmqqsfgxjugi.fsf@gitster.g>
source: <cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com>
* ab/config-multi-and-nonbool (2022-11-27) 9 commits
- for-each-repo: with bad config, don't conflate <path> and <cmd>
- config API: add "string" version of *_value_multi(), fix segfaults
- config API users: test for *_get_value_multi() segfaults
- for-each-repo: error on bad --config
- config API: have *_multi() return an "int" and take a "dest"
- versioncmp.c: refactor config reading next commit
- config tests: add "NULL" tests for *_get_value_multi()
- config tests: cover blind spots in git_die_config() tests
- for-each-repo tests: test bad --config keys
Assorted config API updates.
Expecting a reroll.
source: <cover-v3-0.9-00000000000-20221125T093158Z-avarab@gmail.com>
* ed/fsmonitor-inotify (2022-12-13) 6 commits
- fsmonitor: update doc for Linux
- fsmonitor: test updates
- fsmonitor: enable fsmonitor for Linux
- fsmonitor: implement filesystem change listener for Linux
- fsmonitor: determine if filesystem is local or remote
- fsmonitor: prepare to share code between Mac OS and Linux
Bundled fsmonitor for Linux using inotify API.
Needs review on the updated round.
source: <pull.1352.v5.git.git.1670882286.gitgitgadget@gmail.com>
* jc/spell-id-in-both-caps-in-message-id (2022-12-17) 1 commit
- e-mail workflow: Message-ID is spelled with ID in both capital letters
Consistently spell "Message-ID" as such, not "Message-Id".
Needs review.
source: <xmqqsfhgnmqg.fsf@gitster.g>
* cb/grep-fallback-failing-jit (2022-12-17) 1 commit
- grep: fall back to interpreter mode if JIT fails
In an environment where dynamically generated code is prohibited to
run (e.g. SELinux), failure to JIT pcre patterns is expected. Fall
back to interpreted execution in such a case.
Expecting a reroll.
cf. <f680b274-fa85-6624-096a-7753a2671c15@grsecurity.net>
source: <20221216121557.30714-1-minipli@grsecurity.net>
* ad/test-record-count-when-harness-is-in-use (2022-12-25) 1 commit
- test-lib: allow storing counts with test harnesses
Allow summary results from tests to be written to t/test-results
directory even when a test harness like 'prove' is in use.
Needs review.
source: <20221224225200.1027806-1-adam@dinwoodie.org>
* so/diff-merges-more (2022-12-18) 5 commits
- diff-merges: improve --diff-merges documentation
- diff-merges: issue warning on lone '-m' option
- diff-merges: support list of values for --diff-merges
- diff-merges: implement log.diffMerges-m-imply-p config
- diff-merges: implement [no-]hide option and log.diffMergesHide config
Assorted updates to "--diff-merges=X" option.
May want to discard. Breaking compatibility does not seem worth it.
source: <20221217132955.108542-1-sorganov@gmail.com>
--------------------------------------------------
[Cooking]
* ab/cache-api-cleanup-users (2023-01-17) 3 commits
(merged to 'next' on 2023-01-18 at c5a4374652)
+ treewide: always have a valid "index_state.repo" member
+ Merge branch 'ds/omit-trailing-hash-in-index' into ab/cache-api-cleanup-users
+ Merge branch 'ab/cache-api-cleanup' into ab/cache-api-cleanup-users
Updates the users of the cache API.
Will merge to 'master'.
cf. <db312853-81a1-542b-db96-d816c463516c@github.com>
source: <patch-1.1-b4998652822-20230117T135234Z-avarab@gmail.com>
* cb/checkout-same-branch-twice (2023-01-20) 1 commit
- checkout/switch: disallow checking out same branch in multiple worktrees
"git checkout -B $branch" failed to protect against checking out
a branch that is checked out elsewhere, unlike "git branch -f" did.
Expecting a (hopefully final) reroll.
cf. <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk>
source: <20230120113553.24655-1-carenas@gmail.com>
* sa/cat-file-mailmap--batch-check (2023-01-18) 1 commit
(merged to 'next' on 2023-01-18 at 25ecb1dd3a)
+ git-cat-file.txt: fix list continuations rendering literally
Docfix.
Will merge to 'master'.
source: <20230118082749.1252459-1-martin.agren@gmail.com>
* pb/branch-advice-recurse-submodules (2023-01-18) 1 commit
(merged to 'next' on 2023-01-19 at 13747fc72d)
+ branch: improve advice when --recurse-submodules fails
Improve advice message given when "git branch --resurse-submodules"
fails.
Will merge to 'master'.
source: <pull.1464.git.1673890908453.gitgitgadget@gmail.com>
* ab/sequencer-unleak (2023-01-18) 8 commits
- commit.c: free() revs.commit in get_fork_point()
- builtin/rebase.c: free() "options.strategy_opts"
- sequencer.c: always free() the "msgbuf" in do_pick_commit()
- builtin/rebase.c: fix "options.onto_name" leak
- builtin/revert.c: move free-ing of "revs" to replay_opts_release()
- rebase & sequencer API: fix get_replay_opts() leak in "rebase"
- sequencer.c: split up sequencer_remove_state()
- rebase: use "cleanup" pattern in do_interactive_rebase()
Plug leaks in sequencer subsystem and its users.
Expecting a hopefully minor and final reroll.
cf. <xmqqedry17r4.fsf@gitster.g>
source: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
* jk/hash-object-literally-fd-leak (2023-01-19) 1 commit
(merged to 'next' on 2023-01-19 at fff9b60a36)
+ hash-object: fix descriptor leak with --literally
Leakfix.
Will merge to 'master'.
source: <Y8ijpJqtkDTi792i@coredump.intra.peff.net>
* jc/doc-branch-update-checked-out-branch (2023-01-18) 1 commit
(merged to 'next' on 2023-01-19 at 970900a232)
+ branch: document `-f` and linked worktree behaviour
Document that "branch -f <branch>" disables only the safety to
avoid recreating an existing branch.
Will merge to 'master'.
source: <xmqqa62f2dj1.fsf_-_@gitster.g>
* jc/doc-checkout-b (2023-01-19) 1 commit
(merged to 'next' on 2023-01-23 at 95340e1941)
+ checkout: document -b/-B to highlight the differences from "git branch"
Clarify how "checkout -b/-B" and "git branch [-f]" are similar but
different in the documentation.
Will merge to 'master'.
source: <xmqqtu0m1m9i.fsf@gitster.g>
* cw/fetch-remote-group-with-duplication (2023-01-19) 1 commit
(merged to 'next' on 2023-01-20 at 7f00e43209)
+ fetch: fix duplicate remote parallel fetch bug
"git fetch <group>", when "<group>" of remotes lists the same
remote twice, unnecessarily failed when parallel fetching was
enabled, which has been corrected.
Will merge to 'master'.
source: <20230119220538.1522464-1-calvinwan@google.com>
* po/pretty-format-columns-doc (2023-01-19) 5 commits
(merged to 'next' on 2023-01-23 at d41cb5f527)
+ doc: pretty-formats note wide char limitations, and add tests
+ doc: pretty-formats describe use of ellipsis in truncation
+ doc: pretty-formats document negative column alignments
+ doc: pretty-formats: delineate `%<|(` parameter values
+ doc: pretty-formats: separate parameters from placeholders
Clarify column-padding operators in the pretty format string.
Will merge to 'master'.
source: <20230119181827.1319-1-philipoakley@iee.email>
* jk/hash-object-fsck (2023-01-19) 7 commits
(merged to 'next' on 2023-01-23 at 985e87fc34)
+ fsck: do not assume NUL-termination of buffers
+ hash-object: use fsck for object checks
+ fsck: provide a function to fsck buffer without object struct
+ t: use hash-object --literally when created malformed objects
+ t7030: stop using invalid tag name
+ t1006: stop using 0-padded timestamps
+ t1007: modernize malformed object tests
"git hash-object" now checks that the resulting object is well
formed with the same code as "git fsck".
Will merge to 'master'.
source: <Y8hX+pIZUKXsyYj5@coredump.intra.peff.net>
source: <Y8ifa7hyqxSbL92U@coredump.intra.peff.net>
* tb/t0003-invoke-dd-more-portably (2023-01-22) 1 commit
(merged to 'next' on 2023-01-23 at 917aa24a27)
+ t0003: call dd with portable blocksize
Test portability fix.
Will merge to 'master'.
source: <20230122062839.14542-1-tboegi@web.de>
* jc/attr-doc-fix (2023-01-22) 1 commit
(merged to 'next' on 2023-01-24 at a25d37cb0f)
+ attr: fix instructions on how to check attrs
Comment fix.
Will merge to 'master'.
source: <pull.1441.v2.git.git.1674447742078.gitgitgadget@gmail.com>
* rj/avoid-switching-to-already-used-branch (2023-01-22) 3 commits
- switch: reject if the branch is already checked out elsewhere (test)
- rebase: refuse to switch to a branch already checked out elsewhere (test)
- branch: fix die_if_checked_out() when ignore_current_worktree
A few subcommands have been taught to stop users from working on a
branch that is being used in another worktree linked to the same
repository.
Expecting a (hopefully final) reroll.
cf. <d61a2393-64c8-da49-fe13-00bc4a52d5e3@gmail.com>
source: <f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com>
* rj/bisect-already-used-branch (2023-01-22) 1 commit
- bisect: fix "reset" when branch is checked out elsewhere
Allow "git bisect reset [name]" to check out the named branch (or
the original one) even when the branch is already checked out in a
different worktree linked to the same repository.
Leaning negative. Why is it a good thing?
cf. <xmqqo7qqovp1.fsf@gitster.g>
source: <1c36c334-9f10-3859-c92f-3d889e226769@gmail.com>
* en/ls-files-doc-update (2023-01-13) 4 commits
- ls-files: guide folks to --exclude-standard over other --exclude* options
- ls-files: clarify descriptions of status tags for -t
- ls-files: clarify descriptions of file selection options
- ls-files: add missing documentation for --resolve-undo option
Doc update to ls-files.
Will merge to 'next'.
source: <pull.1463.git.1673584914.gitgitgadget@gmail.com>
* ms/send-email-feed-header-to-validate-hook (2023-01-19) 2 commits
- send-email: expose header information to git-send-email's sendemail-validate hook
- send-email: refactor header generation functions
"git send-email" learned to give the e-mail headers to the validate
hook by passing an extra argument from the command line.
Expecting a (hopefully final) reroll.
cf. <c1ba0a28-3c39-b313-2757-dceb02930334@amd.com>
source: <20230120012459.920932-1-michael.strawbridge@amd.com>
* ds/bundle-uri-5 (2023-01-23) 10 commits
- bundle-uri: test missing bundles with heuristic
- bundle-uri: store fetch.bundleCreationToken
- fetch: fetch from an external bundle URI
- bundle-uri: drop bundle.flag from design doc
- clone: set fetch.bundleURI if appropriate
- bundle-uri: download in creationToken order
- bundle-uri: parse bundle.<id>.creationToken values
- bundle-uri: parse bundle.heuristic=creationToken
- t5558: add tests for creationToken heuristic
- bundle: optionally skip reachability walk
The bundle-URI subsystem adds support for creation-token heuristics
to help incremental fetches.
Expecting a reroll.
cf. <771a2993-85bd-0831-0977-24204f84e206@github.com>
cf. <01f97aff-58a1-ef2c-e668-d37ea513c64e@github.com>
cf. <ecc6b167-f5c4-48ce-3973-461d1659ed40@github.com>
source: <pull.1454.v2.git.1674487310.gitgitgadget@gmail.com>
* tc/cat-file-z-use-cquote (2023-01-16) 1 commit
- cat-file: quote-format name in error when using -z
"cat-file" in the batch mode that is fed NUL-terminated pathnames
learned to cquote them in its error output (otherwise, a funny
pathname with LF in it would break the lines in the output stream).
Expecting a reroll.
cf. <2a2a46f0-a9bc-06a6-72e1-28800518777c@dunelm.org.uk>
source: <20230116190749.4141516-1-toon@iotcl.com>
* cb/grep-pcre-ucp (2023-01-18) 1 commit
(merged to 'next' on 2023-01-19 at 2c7e531839)
+ grep: correctly identify utf-8 characters with \{b,w} in -P
"grep -P" learned to use Unicode Character Property to grok
character classes when processing \b and \w etc.
Will merge to 'master'.
cf. <xmqqzgaf2zpt.fsf@gitster.g>
source: <20230108155217.2817-1-carenas@gmail.com>
* cw/submodule-status-in-parallel (2023-01-17) 6 commits
- submodule: call parallel code from serial status
- diff-lib: parallelize run_diff_files for submodules
- diff-lib: refactor match_stat_with_submodule
- submodule: move status parsing into function
- submodule: strbuf variable rename
- run-command: add duplicate_output_fn to run_processes_parallel_opts
"git submodule status" learned to run the comparison in submodule
repositories in parallel.
Expecting a reroll.
cf. <CAFySSZBiW7=ZTmXRaLzCoKUi0Jd=fzvW5PJ6=Ka0jKHoP2ddSw@mail.gmail.com>
cf. <kl6lo7qlvg4h.fsf@chooglen-macbookpro.roam.corp.google.com>
source: <20230104215415.1083526-1-calvinwan@google.com>
* ab/various-leak-fixes (2023-01-18) 19 commits
- push: free_refs() the "local_refs" in set_refspecs()
- receive-pack: free() the "ref_name" in "struct command"
- grep API: plug memory leaks by freeing "header_list"
- grep.c: refactor free_grep_patterns()
- object-file.c: release the "tag" in check_tag()
- builtin/merge.c: free "&buf" on "Your local changes..." error
- builtin/merge.c: use fixed strings, not "strbuf", fix leak
- show-branch: free() allocated "head" before return
- commit-graph: fix a parse_options_concat() leak
- http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()
- http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main()
- worktree: fix a trivial leak in prune_worktrees()
- repack: fix leaks on error with "goto cleanup"
- name-rev: don't xstrdup() an already dup'd string
- various: add missing clear_pathspec(), fix leaks
- clone: use free() instead of UNLEAK()
- commit-graph: use free_commit_graph() instead of UNLEAK()
- bundle.c: don't leak the "args" in the "struct child_process"
- tests: mark tests as passing with SANITIZE=leak
Leak fixes.
Needs review.
source: <cover-v5-00.19-00000000000-20230118T120334Z-avarab@gmail.com>
* rj/branch-unborn-in-other-worktrees (2023-01-19) 3 commits
- branch: rename orphan branches in any worktree
- branch: description for orphan branch errors
- avoid unnecessary worktrees traversing
Error messages given when working on an unborn branch that is
checked out in another worktree have been improvved.
Expecting a reroll.
cf. <527f7315-be7b-7ec0-04fc-d07da7d4fefa@gmail.com>
source: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com>
* km/send-email-with-v-reroll-count (2022-11-27) 1 commit
(merged to 'next' on 2023-01-19 at 9b3543471c)
+ send-email: relay '-v N' to format-patch
"git send-email -v 3" used to be expanded to "git send-email
--validate 3" when the user meant to pass them down to
"format-patch", which has been corrected.
Will merge to 'master'.
source: <87edtp5uws.fsf@kyleam.com>
* mc/credential-helper-auth-headers (2023-01-20) 12 commits
(merged to 'next' on 2023-01-25 at cb95006bb2)
+ credential: add WWW-Authenticate header to cred requests
+ http: read HTTP WWW-Authenticate response headers
+ http: replace unsafe size_t multiplication with st_mult
+ test-http-server: add sending of arbitrary headers
+ test-http-server: add simple authentication
+ test-http-server: pass Git requests to http-backend
+ test-http-server: add HTTP request parsing
+ test-http-server: add HTTP error response function
+ test-http-server: add stub HTTP server test helper
+ daemon: rename some esoteric/laboured terminology
+ daemon: libify child process handling functions
+ daemon: libify socket setup and option functions
Extending credential helper protocol.
Will kick out of 'next'. The test-only server is an eyesore.
cf. <e57c1ca3-c21c-db41-a386-e5887f46055c@github.com>
cf. <Y9JkMLueCwjkLHOr@coredump.intra.peff.net>
source: <pull.1352.v7.git.1674252530.gitgitgadget@gmail.com>
--------------------------------------------------
[Discarded]
* jc/ci-deprecated-declarations-are-not-fatal (2023-01-14) 1 commit
(merged to 'next' on 2023-01-14 at 5efb778ab0)
+ ci: do not die on deprecated-declarations warning
CI build fix for overzealous -Werror.
Reverted out of 'next'
Preferring jk/curl-avoid-deprecated-api that fixes the code properly.
source: <xmqq7cxpkpjp.fsf@gitster.g>
* po/pretty-hard-trunc (2022-11-13) 1 commit
. pretty-formats: add hard truncation, without ellipsis, options
Add a new pretty format which truncates without ellipsis.
Superseded by the 'po/pretty-format-columns-doc' topic.
source: <20221112143616.1429-1-philipoakley@iee.email>
* en/rebase-update-refs-needs-merge-backend (2023-01-22) 9 commits
(merged to 'next' on 2023-01-23 at 1b65346647)
+ rebase: provide better error message for apply options vs. merge config
+ rebase: put rebase_options initialization in single place
+ rebase: fix formatting of rebase --reapply-cherry-picks option in docs
+ rebase: clarify the OPT_CMDMODE incompatibilities
+ rebase: add coverage of other incompatible options
+ rebase: fix docs about incompatibilities with --root
+ rebase: remove --allow-empty-message from incompatible opts
+ rebase: flag --apply and --merge as incompatible
+ rebase: mark --update-refs as requiring the merge backend
The "--update-refs" feature of "git rebase" requires the use of the
merge backend, while "--whitespace=fix" feature does not work with
the said backend. Notice the combination and error out, instead of
silently ignoring one of the features requested.
Reverted out of 'next' to be replaced with en/rebase-incompatible-opts
source: <pull.1466.v4.git.1674367961.gitgitgadget@gmail.com>
* rs/tree-parse-mode-overflow-check (2023-01-21) 1 commit
. tree-walk: disallow overflowing modes
Reject tree objects with entries whose mode bits are overly wide.
Retracted.
cf. <b4b48877-5b80-e96f-d09f-2fe275f42950@web.de>
source: <d673fde7-7eb2-6306-86b6-1c1a4c988ee8@web.de>
* cc/filtered-repack (2022-12-25) 3 commits
. gc: add gc.repackFilter config option
. repack: add --filter=<filter-spec> option
. pack-objects: allow --filter without --stdout
"git repack" learns to discard objects that ought to be retrievable
again from the promisor remote.
May want to discard. Its jaggy edges may be a bit too sharp.
cf. <Y7WTv19aqiFCU8au@ncase>
source: <20221221040446.2860985-1-christian.couder@gmail.com>
^ permalink raw reply
* Re: [PATCH v7 08/12] test-http-server: add simple authentication
From: Junio C Hamano @ 2023-01-26 22:27 UTC (permalink / raw)
To: Jeff King
Cc: Matthew John Cheetham via GitGitGadget, git, Derrick Stolee,
Lessley Dennington, Matthew John Cheetham, M Hickford,
Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason
In-Reply-To: <Y9LvFMzriAWUsS58@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I suspect this could all be done as a CGI wrapping git-http-backend. You
>> can influence the HTTP response code by sending:
> ...
> And here's a minimally worked-out example of that approach. It's on top
> of your patches so I could use your credential-helper infrastructure in
> the test, but the intent is that it would replace all of the test-tool
> server patches and be rolled into t5556 as appropriate.
Thanks for helping Matthew's topic move forward. I very much like
seeing apache used for tests in this sample approach, like all the
other http tests we do with apache, instead of a custom server that
we need to ensure that it mimics the real-world use cases and we
have to maintain.
^ permalink raw reply
* Re: [PATCH v3] attr: fix instructions on how to check attrs
From: Junio C Hamano @ 2023-01-26 22:17 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <pull.1441.v3.git.git.1674768107941.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since V2:
>
> * updated with adding second argument after rebasing against master
Ahh, thanks for being so careful. I forgot all about that other
topic changing the function signature.
^ permalink raw reply
* Re: [PATCH v7 08/12] test-http-server: add simple authentication
From: Jeff King @ 2023-01-26 21:22 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason
In-Reply-To: <Y9JPslSoEayaCJ3n@coredump.intra.peff.net>
On Thu, Jan 26, 2023 at 05:02:27AM -0500, Jeff King wrote:
> I suspect this could all be done as a CGI wrapping git-http-backend. You
> can influence the HTTP response code by sending:
>
> Status: 401 Authorization Required
> WWW-Authenticate: whatever you want
>
> And likewise you can see what the client sends by putting something like
> this in apache.conf:
>
> SetEnvIf Authorization "(.*)" HTTP_AUTHORIZATION=$1
>
> and then reading $HTTP_AUTHORIZATION as you like. At that point, it
> feels like a simple shell or perl script could then decide whether to
> return a 401 or not (and if not, then just exec git-http-backend to do
> the rest). And the scripts would be simple enough that you could have
> individual scripts to implement various schemes, rather than
> implementing this configuration scheme. You can control which script is
> run based on the URL; see the way we match /broken_smart/, etc, in
> t/lib-httpd/apache.conf.
And here's a minimally worked-out example of that approach. It's on top
of your patches so I could use your credential-helper infrastructure in
the test, but the intent is that it would replace all of the test-tool
server patches and be rolled into t5556 as appropriate.
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 6 ++++++
t/lib-httpd/custom-auth.sh | 18 ++++++++++++++++
t/t5563-simple-http-auth.sh | 42 +++++++++++++++++++++++++++++++++++++
4 files changed, 67 insertions(+)
create mode 100644 t/lib-httpd/custom-auth.sh
create mode 100755 t/t5563-simple-http-auth.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 608949ea80..ab255bdbc5 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -137,6 +137,7 @@ prepare_httpd() {
install_script error-smart-http.sh
install_script error.sh
install_script apply-one-time-perl.sh
+ install_script custom-auth.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 0294739a77..4b2256363f 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_HTTP_EXPORT_ALL
SetEnv GIT_PROTOCOL
</LocationMatch>
+<LocationMatch /custom_auth/>
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+ CGIPassAuth on
+</LocationMatch>
ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/
ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/
ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
@@ -144,6 +149,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error_smart/ error-smart-http.sh/
ScriptAlias /error/ error.sh/
ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
+ScriptAliasMatch /custom_auth/(.*) custom-auth.sh/$1
<Directory ${GIT_EXEC_PATH}>
Options FollowSymlinks
</Directory>
diff --git a/t/lib-httpd/custom-auth.sh b/t/lib-httpd/custom-auth.sh
new file mode 100644
index 0000000000..686895ee8c
--- /dev/null
+++ b/t/lib-httpd/custom-auth.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+# Our acceptable auth here is hard-coded, but we could
+# read it from a file provided by individual tests, etc.
+#
+# base64("alice:secret-passwd")
+USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+
+case "$HTTP_AUTHORIZATION" in
+"Basic $USERPASS64")
+ exec "$GIT_EXEC_PATH"/git-http-backend
+ ;;
+*)
+ echo 'Status: 401 Auth Required'
+ echo 'WWW-Authenticate: Basic realm="whatever"'
+ echo
+ ;;
+esac
diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh
new file mode 100755
index 0000000000..314f9217e6
--- /dev/null
+++ b/t/t5563-simple-http-auth.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='test http auth header and credential helper interop'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+. "$TEST_DIRECTORY"/lib-credential-helper.sh
+
+start_httpd
+
+setup_credential_helper
+
+test_expect_success 'setup repository' '
+ test_commit foo &&
+ git init --bare "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
+'
+
+test_expect_success 'access using custom auth' '
+ set_credential_reply get <<-EOF &&
+ username=alice
+ password=secret-passwd
+ EOF
+
+ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote \
+ "$HTTPD_URL/custom_auth/repo.git" &&
+
+ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HTTPD_DEST
+ wwwauth[]=Basic realm="whatever"
+ EOF
+
+ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HTTPD_DEST
+ username=alice
+ password=secret-passwd
+ EOF
+'
+
+test_done
--
2.39.1.738.g5e5f8a2714
^ permalink raw reply related
* [PATCH v3] attr: fix instructions on how to check attrs
From: John Cai via GitGitGadget @ 2023-01-26 21:21 UTC (permalink / raw)
To: git; +Cc: John Cai, John Cai
In-Reply-To: <pull.1441.v2.git.git.1674447742078.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
The instructions in attr.h describing what functions to call to check
attributes is missing the index as the first argument to
git_check_attr(), as well as tree_oid as the second argument.
When 7a400a2c (attr: remove an implicit dependency on the_index,
2018-08-13) started passing an index_state instance to git_check_attr(),
it forgot to update the API documentation in
Documentation/technical/api-gitattributes.txt. Later, 3a1b3415
(attr: move doc to attr.h, 2019-11-17) moved the API documentation to
attr.h as a comment, but still left out the index_state as an argument.
In 47cfc9b (attr: add flag `--source` to work with tree-ish 2023-01-14)
added tree_oid as an optional parameter but was not added to the docs in
attr.h
Fix this to make the documentation in the comment consistent with the
actual function signature.
Signed-off-by: John Cai <johncai86@gmail.com>
---
attr: fix instructions on how to check attrs
The instructions in attr.h describing what functions to call to check
attributes is missing the index as the first argument to git_check_attr.
Fix this to make it consistent with the actual function signature.
Changes since V2:
* updated with adding second argument after rebasing against master
Changes since V1:
* updated commit message to include some history
Signed-off-by: John Cai johncai86@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1441%2Fjohn-cai%2Fjc%2Ffix-attr-docs-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1441/john-cai/jc/fix-attr-docs-v3
Pull-Request: https://github.com/git/git/pull/1441
Range-diff vs v2:
1: 8cfee55e48f ! 1: cf6f456af47 attr: fix instructions on how to check attrs
@@ Commit message
The instructions in attr.h describing what functions to call to check
attributes is missing the index as the first argument to
- git_check_attr().
+ git_check_attr(), as well as tree_oid as the second argument.
When 7a400a2c (attr: remove an implicit dependency on the_index,
2018-08-13) started passing an index_state instance to git_check_attr(),
@@ Commit message
(attr: move doc to attr.h, 2019-11-17) moved the API documentation to
attr.h as a comment, but still left out the index_state as an argument.
+ In 47cfc9b (attr: add flag `--source` to work with tree-ish 2023-01-14)
+ added tree_oid as an optional parameter but was not added to the docs in
+ attr.h
+
Fix this to make the documentation in the comment consistent with the
actual function signature.
@@ attr.h
*
* setup_check();
- * git_check_attr(path, check);
-+ * git_check_attr(&the_index, path, check);
++ * git_check_attr(&the_index, tree_oid, path, check);
* ------------
*
* - Act on `.value` member of the result, left in `check->items[]`:
attr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.h b/attr.h
index 58a2bc1344f..9884ea2bc60 100644
--- a/attr.h
+++ b/attr.h
@@ -45,7 +45,7 @@
* const char *path;
*
* setup_check();
- * git_check_attr(path, check);
+ * git_check_attr(&the_index, tree_oid, path, check);
* ------------
*
* - Act on `.value` member of the result, left in `check->items[]`:
base-commit: 5dec958dcf965fc75e0f459f8e8ccf9c9f495b15
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 0/1] ref-filter: add new "signature" atom
From: Junio C Hamano @ 2023-01-26 21:07 UTC (permalink / raw)
To: Nsengiyumva Wilberforce; +Cc: git
In-Reply-To: <20230116173814.11338-1-nsengiyumvawilberforce@gmail.com>
Nsengiyumva Wilberforce <nsengiyumvawilberforce@gmail.com> writes:
> iv) I also moved check_commit_signature(commit, &sigc) out of the
> to avoid running GPG twice.still this change is in grab_signature()
> in ref-filter.c.
Is there some code to avoid calling check_commit_signature() or
grab_signature() itself when no %(signature) atoms is used? When
nobody asked to use signature atom at all, calling GPG once is
already once too many.
Thanks.
^ permalink raw reply
* Re: [PATCH v7 08/12] test-http-server: add simple authentication
From: Jeff King @ 2023-01-26 20:33 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason
In-Reply-To: <b8d3e81b5534148359c7e92807cf1e2795480ddf.1674252531.git.gitgitgadget@gmail.com>
On Fri, Jan 20, 2023 at 10:08:46PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> +static int split_auth_param(const char *str, char **scheme, char **val)
> +{
> + struct strbuf **p = strbuf_split_str(str, ':', 2);
> +
> + if (!p[0])
> + return -1;
> +
> + /* trim trailing ':' */
> + if (p[0]->len && p[0]->buf[p[0]->len - 1] == ':')
> + strbuf_setlen(p[0], p[0]->len - 1);
> +
> + *scheme = strbuf_detach(p[0], NULL);
> + *val = p[1] ? strbuf_detach(p[1], NULL) : NULL;
> +
> + strbuf_list_free(p);
> + return 0;
> +}
Oh, I forgot one more Coverity-detected problem here when reviewing last
night. The early "return -1" here leaks "p" (there are no strbufs in the
resulting array, but strbuf_split_str() will still have allocated the
array). It needs a call to strbuf_list_free(p) there.
-Peff
^ permalink raw reply
* Re: [PATCH v6 5/6] diff-lib: parallelize run_diff_files for submodules
From: Calvin Wan @ 2023-01-26 18:52 UTC (permalink / raw)
To: Glen Choo
Cc: git, emilyshaffer, avarab, phillip.wood123, newren, jonathantanmy
In-Reply-To: <kl6lilgtveoe.fsf@chooglen-macbookpro.roam.corp.google.com>
On Thu, Jan 26, 2023 at 1:16 AM Glen Choo <chooglen@google.com> wrote:
>
>
> Calvin Wan <calvinwan@google.com> writes:
>
> > @@ -226,6 +242,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> > newmode = ce->ce_mode;
> > } else {
> > struct stat st;
> > + unsigned ignore_untracked = 0;
> > + int defer_submodule_status = !!revs->repo;
>
> What is the reasoning behind this condition? I would expect revs->repo
> to always be set, and we would always end up deferring.
Ah looks like a vestigial sanity check. You're correct that we would
always be deferring anyways.
>
> > newmode = ce_mode_from_stat(ce, st.st_mode);
> > + if (defer_submodule_status) {
> > + struct submodule_status_util tmp = {
> > + .changed = changed,
> > + .dirty_submodule = 0,
> > + .ignore_untracked = ignore_untracked,
> > + .newmode = newmode,
> > + .ce = ce,
> > + .path = ce->name,
> > + };
> > + struct string_list_item *item;
> > +
> > + item = string_list_append(&submodules, ce->name);
> > + item->util = xmalloc(sizeof(tmp));
> > + memcpy(item->util, &tmp, sizeof(tmp));
>
> (Not a C expert) Since we don't return the string list, I wonder if we
> can avoid the memcpy() by using &tmp like so:
>
> struct string_list_item *item;
> item = string_list_append(&submodules, ce->name);
> item->util = &tmp;
>
> And then when we call string_list_clear(), we wouldn't need to free the
> util since we exit the stack frame.
Unfortunately this doesn't work because tmp is deallocated off the stack
after changing scope.
> > +test_expect_success 'diff in superproject with submodules respects parallel settings' '
> > + test_when_finished "rm -f trace.out" &&
> > + (
> > + GIT_TRACE=$(pwd)/trace.out git diff &&
> > + grep "1 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + git config submodule.diffJobs 8 &&
> > + GIT_TRACE=$(pwd)/trace.out git diff &&
> > + grep "8 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 diff &&
> > + grep "preparing to run up to [0-9]* tasks" trace.out &&
> > + ! grep "up to 0 tasks" trace.out &&
> > + >trace.out
> > + )
> > +'
> > +
>
> Could we get tests to check that the output of git diff isn't changed by
> setting parallelism? This might not be feasible for submodule.diffJobs >
> 1 due to raciness, but it would be good to see for submodule.diffJobs =
> 1 at least.
ack.
>
> > test_expect_success 'git diff --raw HEAD' '
> > hexsz=$(test_oid hexsz) &&
> > git diff --raw --abbrev=$hexsz HEAD >actual &&
> > diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
> > index d050091345..52a82b703f 100755
> > --- a/t/t7506-status-submodule.sh
> > +++ b/t/t7506-status-submodule.sh
> > @@ -412,4 +412,23 @@ test_expect_success 'status with added file in nested submodule (short)' '
> > EOF
> > '
> >
> > +test_expect_success 'status in superproject with submodules respects parallel settings' '
> > + test_when_finished "rm -f trace.out" &&
> > + (
> > + GIT_TRACE=$(pwd)/trace.out git status &&
> > + grep "1 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + git config submodule.diffJobs 8 &&
> > + GIT_TRACE=$(pwd)/trace.out git status &&
> > + grep "8 tasks" trace.out &&
> > + >trace.out &&
> > +
> > + GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 status &&
> > + grep "preparing to run up to [0-9]* tasks" trace.out &&
> > + ! grep "up to 0 tasks" trace.out &&
> > + >trace.out
> > + )
> > +'
> > +
>
> Ditto for "status".
ack.
^ permalink raw reply
* Re: git log --follow wrongly includes delete commit
From: Steinar H. Gunderson @ 2023-01-26 17:18 UTC (permalink / raw)
To: Derrick Stolee; +Cc: git
In-Reply-To: <fcc08ee2-dc58-09da-dc60-c438cfbf6602@github.com>
On Thu, Jan 26, 2023 at 11:38:42AM -0500, Derrick Stolee wrote:
> It's actually a third option: it was deleted but also renamed in an
> independent point in history, but the delete is more recent "in time"
> that it shows up first, and the merge that resolves the issue doesn't
> show up at all.
I see! So basically my parser needs to also start tracking merges,
except --follow seems to be sort of odd with those as well (though
I think maybe it's changed in some fairly recent git version).
> (Note: I didn't include --follow here as it filtered the --graph
> output in a strange way, including dropping the merge commitswhich was
> confusing to me.)
I think you need to give -c or --cc or something similar to see merges in
--follow, but the man pages are not entirely clear to me.
/* Steinar */
--
Homepage: https://www.sesse.net/
^ permalink raw reply
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere
From: Junio C Hamano @ 2023-01-26 17:13 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Phillip Wood
In-Reply-To: <xmqqzga5b4yz.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> the branch itself. And more importantly, the branch being checked
> out in another worktree and modified there should not break the
> bisection, EXCEPT that the final "git bisect reset" (without
> arguments) would fail if the other worktree removed the branch.
And "bisect reset [<branch>]" (with or without arguments) should not
ignore other worktrees when it runs "checkout" internally. You
might have done
* checkout 'work' in worktree A
* start bisection of it there
* checkout 'work' in worktree B
* finish bisection of 'work' in worktree A
* "git bisect reset"
and the third step should allow you work on 'work' in the other
worktree, but then the last step should not allow 'work' to be
checked out in two places (it is OK for the user to use "git bisect
reset main" and then "git checkout --ignore-other work" to work it
around, of course, but the default should be safe).
Hmm?
^ permalink raw reply
* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere
From: Junio C Hamano @ 2023-01-26 17:06 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Phillip Wood
In-Reply-To: <0d04f8ed-6933-9354-1f64-24d827424c71@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> But, and this is what makes me think that "checking out will fail" is the wrong
> choice for "bisect", while bisecting, with the worktree in a detached HEAD
> state, the branch to which "bisect reset" will check out back (BISECT_START),
> is still considered checked out in the working tree:
>
> $ git checkout -b work
> $ git bisect start HEAD HEAD~3
> ... bisect detaches the current worktree ...
> $ git worktree add work
> Preparing worktree (checkout out 'work')
> fatal: 'work' is already checked out at ...
>
> So, checking out back to the implicitly checked out branch sounds like it
> should not fail.
If that is what you are aiming at, I suspect that the posted patch
is doing it in a wrong way. Instead, we should just declare that
the branch being bisected does not mean the branch cannot be checked
out elsewhere, so that
$ git worktree add --detach ../another HEAD^0
$ git checkout -b work
$ git bisect start work work~3
... detaches ...
$ git -C ../another checkout work
should just work, no?
I admit I haven't thought things through, but I tend to be
sympathetic to such a declaration. After all, "bisect" is a
read-only operation as far as the branch you happened to be on when
you started a bisect session is concerned. Jumping around and
materializing tree states recorded in various commits leading to the
tip of the branch and inspecting them would not change anything on
the branch itself. And more importantly, the branch being checked
out in another worktree and modified there should not break the
bisection, EXCEPT that the final "git bisect reset" (without
arguments) would fail if the other worktree removed the branch.
So, how about removing the is_worktree_being_bisected() check from
find_shared_symref(), so that not just "worktree add" and "bisect
reset", but "checkout" and "switch" are allowed to make the branch
current even it is being bisected elsewhere?
That would affect the other topic, I suspect, as well. It may be a
positive change. Or are there cases I missed, where the branch
being bisected should not be touched from elsewhere, and we cannot
remove the check from find_shared_symref()?
Thanks.
^ permalink raw reply
* Re: git log --follow wrongly includes delete commit
From: Derrick Stolee @ 2023-01-26 16:38 UTC (permalink / raw)
To: Steinar H. Gunderson, git
In-Reply-To: <20230126130509.ovii7ji7hi5wm7qx@sesse.net>
On 1/26/2023 8:05 AM, Steinar H. Gunderson wrote:
> I'm in the Chromium repository; it can be checked out at
> https://chromium.googlesource.com/chromium/src.git (you don't need the
> sub-repostiories). HEAD is pointing to 4e0db738b37c. git 2.39.1.
>
> When I run
>
> git log --raw --follow base/third_party/xdg_user_dirs/xdg_user_dir_lookup.h
>
> this is the first commit that it lists (snipped):
>
> commit 5d4451ebf298d9d71f716cc0135f465cec41fcd0
> [...]
> :100644 000000 9e81e1b53029f 0000000000000 D base/third_party/xdg_user_dirs/xdg_user_dir_lookup.h
>
> This indicates that the last thing that happened to the file is a delete.
> However, the file isn't deleted; it's alive and well. git log without
> --follow does not list this commit at all.
>
> So either git log --follow is listing a delete commit that doesn't make
> sense, or it's missing whatever commit put it back into place afterwards.
It's actually a third option: it was deleted but also renamed in an
independent point in history, but the delete is more recent "in time"
that it shows up first, and the merge that resolves the issue doesn't
show up at all.
You can see this when using --full-history --simplify-merges --graph,
since it will explore enough of the graph to recognize that deletion
while also showing how things got merged strangely:
$ git log --summary --full-history --simplify-merges --graph -- base/third_party/xdg_user_dirs/xdg_user_dir_lookup.h
* commit 1e78967ed2f1937b3809c19d91e7dd62d756d307
|\ Merge: 5d4451ebf298 9e9b6e8ee772
| | Author: grt@chromium.org <grt@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
| | Date: Tue Jul 19 16:14:55 2011 +0000
| |
| | FileManagerDialogTest.SelectFileAndCancel flaky.
| |
| | BUG=89733
| | TBR=robertshield@chromium.org
| | TEST=browser_tests
| | Review URL: http://codereview.chromium.org/7447001
| |
| | git-svn-id: svn://svn.chromium.org/chrome/trunk/src@93027 0039d316-1c4b-4281-b951-d872f2087c98
| |
* | commit 5d4451ebf298d9d71f716cc0135f465cec41fcd0
|/ Author: jbauman@chromium.org <jbauman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
| Date: Tue Jul 19 15:45:49 2011 +0000
|
| Roll ANGLE r704:r705
|
| BUG=
| TEST=try
|
| Review URL: http://codereview.chromium.org/7375016
|
| git-svn-id: svn://svn.chromium.org/chrome/trunk/src@93026 0039d316-1c4b-4281-b951-d872f2087c98
|
| delete mode 100644 base/third_party/xdg_user_dirs/xdg_user_dir_lookup.h
|
* commit 9e9b6e8ee77229781fa8581b7f46413024404a5f
Author: thestig@chromium.org <thestig@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed Dec 2 08:45:01 2009 +0000
Move some XDG code from chrome to base, make DIR_USER_CACHE generic rather than Chromium specific, and clean up a few headers.
BUG=none
TEST=none
Review URL: http://codereview.chromium.org/449048
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@33565 0039d316-1c4b-4281-b951-d872f2087c98
create mode 100644 base/third_party/xdg_user_dirs/xdg_user_dir_lookup.h
(Note: I didn't include --follow here as it filtered the --graph
output in a strange way, including dropping the merge commitswhich was
confusing to me.)
In conclusion: while I agree that this output is confusing, it is due
to the interesting shape of the commit history, and not actually a bug
in --follow.
Thanks,
-Stolee
^ permalink raw reply
* Re: Feature Request - GIT config - Reference value of init.defaultBranch in alias
From: Junio C Hamano @ 2023-01-26 16:21 UTC (permalink / raw)
To: Michal Aron; +Cc: git
In-Reply-To: <CAHoQa4-o-=pB4zPR-1SG96KB02rixQG23mFgh0H9ojWrQ_pREg@mail.gmail.com>
Michal Aron <aronmgv@gmail.com> writes:
> 1) global config having init.defaultBranch = master
> 2) global config having alias com = checkout [ init.defaultBranch ]
I do not think it is a good idea to special case just a single
configuration variable and come up with a special syntax to refer to
it.
Isn't
[alias]
com = !git checkout $(git config init.defaultBranch)
sufficient?
^ permalink raw reply
* Re: [PATCH v7 00/12] Enhance credential helper protocol to include auth headers
From: Junio C Hamano @ 2023-01-26 16:05 UTC (permalink / raw)
To: Jeff King
Cc: Victoria Dye, Matthew John Cheetham via GitGitGadget, git,
Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo,
Ævar Arnfjörð Bjarmason
In-Reply-To: <Y9JkMLueCwjkLHOr@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Thanks, both. Let's merge it down.
>
> Sorry, I'm a bit late to the party, but I left some comments just now
> (this topic had been on my review backlog for ages, but I never quite
> got to it).
>
> Many of my comments were small bits that could be fixed on top (tiny
> leaks, etc). But some of my comments were of the form "no, do it totally
> differently". It may simply be too late for those ones, but let's see if
> Matthew finds anything compelling in them.
I do not mind reverting the merge to 'next' to have an improved
version. Your "do we really want to add a custom server based on
questionable codebase whose quality as a test-bed for real world
usage is dubious?" is a valid concern.
^ permalink raw reply
* Re: When using several ssh key, Git match ssh key by host, instead of hostname in ssh config file.
From: Junio C Hamano @ 2023-01-26 15:56 UTC (permalink / raw)
To: Yangyang Hua; +Cc: git@vger.kernel.org
In-Reply-To: <OSYP286MB0215D880FA9D82B74E393DD691CF9@OSYP286MB0215.JPNP286.PROD.OUTLOOK.COM>
Yangyang Hua <hyy_41@live.com> writes:
> Hi, I find when I use several ssh keys with the right config file
> and clone my private repo, git can't match the key by hostname.
> ...
> I think when git read ssh config, it uses host to match the key
> instead of hostname. Is this bug?
This useful feature is given by ssh, and Git does not deserve credit
for it. The config file of SSH allows you to write multiple entries
that points at the same host, e.g.
Host host1
HostName host.example.com
IdentityFile ~/.ssh/id_rsa_111
Host host2
HostName host.example.com
IdentityFile ~/.ssh/id_rsa_222
so that you can specify which key to use for the same destination
when you have more than one user there. "ssh host1" uses id_rsa_111
while "ssh host2" uses the other one, both connections going to the
same destination host.
If host.example.com were a hosting site like github.com, you can use this
feature to say
$ git push git@host1:/me/lesson1
to connect using id_rsa_111. If you use
$ git clone git@github.com:/me/lesson1
there is no clue which of the two entries you want to use.
^ permalink raw reply
* Re: [PATCH v2] ls-tree: add --sparse-filter-oid argument
From: William Sprent @ 2023-01-26 14:55 UTC (permalink / raw)
To: Victoria Dye, William Sprent via GitGitGadget, git
Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason,
Derrick Stolee, Elijah Newren
In-Reply-To: <42e14dda-cd2b-09df-dea8-246b3fcfac42@github.com>
On 25/01/2023 19.32, Victoria Dye wrote:
> William Sprent wrote:
>> On 24/01/2023 21.11, Victoria Dye wrote>> I haven't looked at your implementation in detail yet, but I did want to
>>> offer a recommendation in case you hadn't considered it: if you want to
>>> allow the use of patterns from a user-specified specific file, it would be
>>> nice to do it in a way that fully replaces the "default" sparse-checkout
>>> settings at the lowest level (i.e., override the values of
>>> 'core_apply_sparse_checkout', 'core_sparse_checkout_cone', and
>>> 'get_sparse_checkout_filename()'). Doing it that way would both make it
>>> easier for other commands to add a '--sparse-patterns' option, and avoid the
>>> separate code path ('path_in_sparse_checkout_1()' vs.
>>> 'recursively_match_path_with_sparse_patterns()', for example) when dealing
>>> with '.git/info/sparse-checkout' patterns vs. manually-specified patterns.
>>>
>>
>> Thanks for the pointers. I'll see what I can do. Do you mean something
>> along the line of the following?
>>
>> set_sparse_checkout_file(filename);
>> init_sparse_checkout_patterns(istate);
>> _ = path_in_sparse_checkout_1(some_path, istate, ...);
>>
>
> Sort of. I mentioned separating the options into "specify the sparse pattern
> file" and "restrict the displayed files to the active pattern set, if there
> is one". For the former, you might add an option like:
>
> OPT_FILENAME(0, "sparse-patterns", &sparse_pattern_file,
> N_("override sparse-checkout behavior using patterns from <file>"))
>
> Then do something like what you have, right after option parsing:
>
> if (sparse_pattern_file) {
> core_apply_sparse_checkout = 1;
> core_sparse_checkout_cone = <???>;
> set_sparse_checkout_file(filename);
> }
>
> If this option is specified, but the repo already has sparse
> patterns/settings of its own, you'll need to (carefully) override the repo's
> existing configuration:
>
> * 'core_apply_sparse_checkout' & 'core_sparse_checkout_cone' are set based
> on the repo config. You'll need to make sure those values are overridden
> before loading the sparse-checkout patterns, and also that they're set
> *after* loading the config.
This sounds a bit easy to get wrong to me, but I assume I can trust that the
config has been loaded by the time 'cmd_ls_tree()' is invoked.
> * Speaking of 'core_sparse_checkout_cone', there are a bunch of ways you
> might configure "cone-" vs. "non-cone" for your patterns (user-specified
> with yet another option, always assume one or the other, try deriving from
> the patterns). My preference would be to always assume "cone mode" - it's
> the direction Git has been moving with sparse-checkout over the past year,
> and still automatically "falls back" on non-cone patterns if the patterns
> can't be used in cone mode (with a warning from
> 'add_pattern_to_hashsets()': "disabling cone pattern matching").
Alright. I've had similar thoughts. But I ended up deciding to respect the
config value since there wouldn't be any way for the user to silence the warning
when passing non-cone mode patterns to the command. I don't feel too strongly about
it, though.
> * If the repo is using a sparse index, the existing sparse directories may
> not be compatible with the custom patterns. Probably easiest to force use
> of a full index, e.g. with 'command_requires_full_index = 1'.
>
OK.
> Fair warning: this probably isn't an exhaustive list of things that would
> need updating, and it'll need thorough testing to make sure there are no
> regressions. But, extending the underlying sparse-checkout infrastructure
> will (ideally) avoid duplicating code and make this behavior reusable across
> other commands.
>
Alright. I'll give it a shot.
> For the other desired behavior ("limit the files to the active
> sparse-checkout patterns"), you could add an option:
>
> OPT_CALLBACK_F(0, "scope", &sparse_scope, "(sparse|all)",
> N_("specify the scope of results with respect to the sparse-checkout"),
> PARSE_OPT_NONEG, option_parse_scope),
>
> ...whose callback parses the string arg into a 'restrict_scope'
> boolean/enum/something. Then, wherever in 'ls-files' a tree or the index are
> iterated over, you can gate the per-file operation on:
>
> if (!restrict_scope || path_in_sparse_checkout(<path>, istate)) {
> /* do something with <path> */
> }
>
> Note that you should use 'path_in_sparse_checkout()', *not* the
> internal/private function 'path_in_sparse_checkout_1()'; you also don't need
> to explicitly 'init_sparse_checkout_patterns()'. Regardless of whether you
> specified custom patterns or are using the ones in
> '.git/info/sparse-checkout', 'path_in_sparse_checkout()' will initialize &
> use the appropriate patterns.
>
Yeah. And I wouldn't need the refactor of 'path_in_sparse_checkout_1()'.
Thanks a lot (again) for the pointers.
^ permalink raw reply
* When using several ssh key, Git match ssh key by host, instead of hostname in ssh config file.
From: Yangyang Hua @ 2023-01-26 14:46 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
I find when I use several ssh keys with the right config file and clone my private repo, git can't match the key by hostname.
I try ssh-add command to add the keys. "ssh -T git@github.com" can work, but git clone/push/pull these action display "Permission denied (publickey)".
Test in git version 2.38.1.windows.1 with win10 and git version 2.34.1 with Ubuntu 22.04.1 LTS
I check the -v and -vvv log, git does read my config file in ~/.ssh.
After I change the repo address to host("github") in my ssh config file, i can clone the repo and push/pull.
Host github
HostName github.com
PreferredAuthentications publickey
IdentityFile ~/.ssh/id_rsa_111
So I change the host to "github.com" in ssh config, everything is back to normal.
Host github.com
HostName github.com
PreferredAuthentications publickey
IdentityFile ~/.ssh/id_rsa_111
I think when git read ssh config, it uses host to match the key instead of hostname. Is this bug?
^ permalink raw reply
* git log --follow wrongly includes delete commit
From: Steinar H. Gunderson @ 2023-01-26 13:05 UTC (permalink / raw)
To: git
Hi,
I'm in the Chromium repository; it can be checked out at
https://chromium.googlesource.com/chromium/src.git (you don't need the
sub-repostiories). HEAD is pointing to 4e0db738b37c. git 2.39.1.
When I run
git log --raw --follow base/third_party/xdg_user_dirs/xdg_user_dir_lookup.h
this is the first commit that it lists (snipped):
commit 5d4451ebf298d9d71f716cc0135f465cec41fcd0
[...]
:100644 000000 9e81e1b53029f 0000000000000 D base/third_party/xdg_user_dirs/xdg_user_dir_lookup.h
This indicates that the last thing that happened to the file is a delete.
However, the file isn't deleted; it's alive and well. git log without
--follow does not list this commit at all.
So either git log --follow is listing a delete commit that doesn't make
sense, or it's missing whatever commit put it back into place afterwards.
/* Steinar */
--
Homepage: http://www.sesse.net/
^ permalink raw reply
* Re: t5559 breaks with apache 2.4.55
From: Jeff King @ 2023-01-26 11:39 UTC (permalink / raw)
To: Todd Zullinger; +Cc: git
In-Reply-To: <Y81lQwG85+Skujja@pobox.com>
On Sun, Jan 22, 2023 at 11:33:07AM -0500, Todd Zullinger wrote:
> > So I haven't reported the bug further yet. But I thought I'd post this
> > here before anybody else wastes time digging in the same hole.
>
> FWIW, I think this is the same issue we discussed about 2
> months back, in <Y4fUntdlc1mqwad5@pobox.com>¹.
>
> I haven't done much else with it since then. It's almost
> surely either an apache httpd/mod_http2 or curl issue. If I
> had to bet, I'd say mod_http2. (But then, it could be curl
> and just has yet to be exposed widely because not many are
> using the current mod_http2 code.)
>
> ¹ https://lore.kernel.org/git/Y4fUntdlc1mqwad5@pobox.com/
Ah, I somehow completely forgot about that issue. Despite being one of
the two participants on the thread.
Yeah, after seeing that, I'm quite sure this is a mod_http2 issue. It
would be nice to bisect within the mod_http2 history to find the
culprit, but I'd first have to figure out how to build standalone apache
modules. ;)
I may try to poke at it later if I have time. It might also be worth
submitting a bug report to the mod_http2 folks. I'd hope to have a more
compact reproduction, but it does at least seem to fail reliably for me
(not even racily).
-Peff
^ permalink raw reply
* Re: [PATCH] tree-walk: disallow overflowing modes
From: Jeff King @ 2023-01-26 11:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Ævar Arnfjörð Bjarmason,
Git List
In-Reply-To: <xmqqmt67fysf.fsf@gitster.g>
On Tue, Jan 24, 2023 at 12:44:16PM -0800, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
> > ... It's basically harmless. Perhaps
> > we just need a comment stating that, to contain the urge to "fix" this.
>
> Yeah, especially since fsck finds and warns about bad mode with
> FSCK_MSG_BAD_FILEMODE and also FSCK_MSG_ZERO_PADDED_FILEMODE in a
> separate codepath, adding another piece of code that checks a
> slightly different condition does not sound like a good idea.
Yeah, I'm happy to drop this whole thing. I do think it would be
reasonable for fsck to check for overflow alongside BAD_FILEMODE, etc,
but it's annoying to do since we are relying on the existing parser.
I actually have a suspicion that it might be reasonable for fsck to just
parse the trees itself, rather than relying on decode_tree_entry(), etc.
But that's a much bigger topic (and a possible maintenance burden) for
questionable gain, so unless we find some compelling reason (some case
that we really want to detect but which we want the regular decoder to
ignore), it's probably not worth exploring.
-Peff
^ permalink raw reply
* Re: [PATCH v7 00/12] Enhance credential helper protocol to include auth headers
From: Jeff King @ 2023-01-26 11:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Victoria Dye, Matthew John Cheetham via GitGitGadget, git,
Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo,
Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqqwn5bg695.fsf@gitster.g>
On Tue, Jan 24, 2023 at 10:03:02AM -0800, Junio C Hamano wrote:
> Victoria Dye <vdye@github.com> writes:
>
> > Matthew John Cheetham via GitGitGadget wrote:
> >> Updates in v6
> >> =============
> >> ...
> > I've re-read the patches in this version; all of my comments from v5 have
> > been addressed, and the additional updates w.r.t. other reviewer feedback
> > all look good as well. At this point, I think the series is ready for
> > 'next'.
> >
> > Thanks!
>
> Thanks, both. Let's merge it down.
Sorry, I'm a bit late to the party, but I left some comments just now
(this topic had been on my review backlog for ages, but I never quite
got to it).
Many of my comments were small bits that could be fixed on top (tiny
leaks, etc). But some of my comments were of the form "no, do it totally
differently". It may simply be too late for those ones, but let's see if
Matthew finds anything compelling in them.
-Peff
^ permalink raw reply
* Re: [PATCH v7 12/12] credential: add WWW-Authenticate header to cred requests
From: Jeff King @ 2023-01-26 11:25 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason
In-Reply-To: <09164f77d56e8efd1450091cf1b12af2bc6cf2f5.1674252531.git.gitgitgadget@gmail.com>
On Fri, Jan 20, 2023 at 10:08:50PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Add the value of the WWW-Authenticate response header to credential
> requests. Credential helpers that understand and support HTTP
> authentication and authorization can use this standard header (RFC 2616
> Section 14.47 [1]) to generate valid credentials.
>
> WWW-Authenticate headers can contain information pertaining to the
> authority, authentication mechanism, or extra parameters/scopes that are
> required.
I'm definitely on board with sending these to the helpers. It does feel
a bit weird that we don't parse them at all, and just foist that on the
helpers.
If I understand the RFC correctly, you can have multiple challenges per
header, but also multiple headers. So:
WWW-Authenticate: Basic realm="foo", OtherAuth realm="bar"
WWW-Authenticate: YetAnotherScheme some-token
could be normalized as:
www-auth-challenge=Basic realm="foo"
www-auth-challenge=OtherAuth realm="bar"
www-auth-challenge=YetAnotherScheme some-token
which saves each helper from having to do the same work. Likewise, we
can do a _little_ more parsing to get:
www-auth-basic=realm="foo"
www-auth-otherauth=realm="bar"
www-auth-yetanotherscheme=some-token
I don't think we can go beyond there, though, without understanding the
syntax of individual schemes. Which is a shame, as one of the goals of
the credential format was to let the helpers do as little as possible
(so they can't get it wrong!). But helpers are stuck doing things like
handling backslashed double-quotes, soaking up extra whitespace, etc.
I'm not really sure what we expect to see in the real world. I guess for
your purposes, you are working on an already-big helper that is happy to
just get the raw values and process them according to the rfc. I'm just
wondering if there are use cases where somebody might want to do
something with this header, but in a quick shell script kind of way. For
example, my credential config is still:
[credential "https://github.com"]
username = peff
helper = "!f() { test $1 = get && echo password=$(pass ...); }; f"
That's an extreme example, but I'm wondering if there's _anything_
useful somebody would want to do in a similar quick-and-dirty kind of
way. For example, deciding which cred to use based on basic realm, like:
realm=foo
while read line; do
case "$line" in
www-auth-basic=)
value=${line#*=}
# oops, we're just assuming it's realm= here, and we're
# not handling quotes at all. I think it could technically be
# realm=foo or realm="foo"
realm=${value#realm=}
;;
esac
done
echo password=$(pass "pats-by-realm/$realm")
which could be made a lot easier if we did more parsing (e.g.,
www-auth-basic-realm or something). I dunno. Maybe that is just opening
up a can of worms, as we're stuffing structured data into a linearized
key-value list. The nice thing about your proposal is that Git does not
even have to know anything about these schemes; it's all the problem of
the helper. My biggest fear is just that we'll want to shift that later,
and we'll be stuck with this microformat forever.
> The current I/O format for credential helpers only allows for unique
> names for properties/attributes, so in order to transmit multiple header
> values (with a specific order) we introduce a new convention whereby a
> C-style array syntax is used in the property name to denote multiple
> ordered values for the same property.
I don't know if this is strictly necessary. The semantics of duplicate
keys are not really defined anywhere, and just because the
implementations of current readers happen to replace duplicates for the
current set of keys doesn't mean everything has to. So you could just
define "wwwauth" to behave differently. But I don't mind having a
syntactic marker to indicate this new type.
If you're at all convinced by what I said above, then we also might be
able to get away with having unique keys anyway.
> Documentation/git-credential.txt | 19 ++-
> credential.c | 11 ++
> t/lib-credential-helper.sh | 27 ++++
> t/t5556-http-auth.sh | 242 +++++++++++++++++++++++++++++++
> 4 files changed, 298 insertions(+), 1 deletion(-)
> create mode 100644 t/lib-credential-helper.sh
The patch itself looks pretty reasonable to me.
One small thing I noticed:
> + git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
As you undoubtedly figured out, the helper path is fed to the shell, so
spaces in the trash directory are a problem. You've solved it here by
adding a layer of double quotes, which handles spaces. But you'd run
into problems if the absolute path that somebody is using for the test
suite has a backslash or a double quote in it.
I don't know how careful we want to be here (or how careful we already
are[1]), but one simple-ish solution is:
export CREDENTIAL_HELPER
git -c "credential.helper=!\"\$CREDENTIAL_HELPER\"" ...
I.e., letting the inner shell expand the variable itself. Another option
is to put the helper into $TRASH_DIRECTORY/bin and add that to the
$PATH.
I also wondered if it was worth having setup_credential_helper() just
stick it in $TRASH_DIRECTORY/.gitconfig so that individual tests don't
have to keep doing that ugly "-c" invocation. Or if you really want to
have each test enable it, perhaps have set_credential_reply() turn it on
via test_config (which will auto-remove it at the end of the test).
-Peff
[1] Curious, I tried cloning git into this directory:
mkdir '/tmp/foo/"horrible \"path\"'
and we do indeed already fail. The first breakage I saw was recent,
but going further back, it looks like bin-wrappers don't correctly
handle this case anyway. So maybe that's evidence that nobody would
do something so ridiculous in practice.
^ permalink raw reply
* Re: [PATCH v7 11/12] http: read HTTP WWW-Authenticate response headers
From: Jeff King @ 2023-01-26 10:31 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason
In-Reply-To: <5f5e46038cf526714f3c5b89ffef2b895b503242.1674252531.git.gitgitgadget@gmail.com>
On Fri, Jan 20, 2023 at 10:08:49PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Read and store the HTTP WWW-Authenticate response headers made for
> a particular request.
>
> This will allow us to pass important authentication challenge
> information to credential helpers or others that would otherwise have
> been lost.
Makes sense, and the code looks pretty reasonable overall.
A few observations:
> @@ -115,6 +116,19 @@ struct credential {
> */
> struct string_list helpers;
>
> + /**
> + * A `strvec` of WWW-Authenticate header values. Each string
> + * is the value of a WWW-Authenticate header in an HTTP response,
> + * in the order they were received in the response.
> + */
> + struct strvec wwwauth_headers;
> +
> + /**
> + * Internal use only. Used to keep track of split header fields
> + * in order to fold multiple lines into one value.
> + */
> + unsigned header_is_last_match:1;
> +
Stuffing this into a "struct credential" feels a little weird, just
because it's specific to http parsing (especially this internal flag).
And the credential code is seeing full header lines, not broken down at
all.
I guess I would have expected some level of abstraction here between the
credential subsystem and the http subsystem, where the latter is parsing
and then sticking opaque data into the credential to ferry to the
helpers.
But it probably isn't that big a deal either way. Even though there are
non-http credentials, it's not too unreasonable for the credential
system to be aware of http specifically.
> +static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
> +{
> + size_t size = st_mult(eltsize, nmemb);
Here's that st_mult() again. Same comment as the previous patch. :)
> + /*
> + * Header lines may not come NULL-terminated from libcurl so we must
> + * limit all scans to the maximum length of the header line, or leverage
> + * strbufs for all operations.
> + *
> + * In addition, it is possible that header values can be split over
> + * multiple lines as per RFC 2616 (even though this has since been
> + * deprecated in RFC 7230). A continuation header field value is
> + * identified as starting with a space or horizontal tab.
> + *
> + * The formal definition of a header field as given in RFC 2616 is:
> + *
> + * message-header = field-name ":" [ field-value ]
> + * field-name = token
> + * field-value = *( field-content | LWS )
> + * field-content = <the OCTETs making up the field-value
> + * and consisting of either *TEXT or combinations
> + * of token, separators, and quoted-string>
> + */
> +
> + strbuf_add(&buf, ptr, size);
OK, so we just copy the buffer. I don't think it would be too hard to
handle the buffer as-is, but this does make things a bit easier. Given
that we're going to immediately throw away the copy for anything except
www-authenticate, we could perhaps wait until we've matched it. That
does mean trimming the CRLF ourselves and using skip_prefix_mem() to
match the start (you'd want skip_iprefix_mem(), of course, but it
doesn't yet exist; I'll leave that as an exercise).
Maybe not worth it to save a few allocations, as an http request is
already pretty heavyweight. Mostly I flagged it because this is going to
run for every header of every request, even though most requests won't
trigger it at all.
> + /* Strip the CRLF that should be present at the end of each field */
> + strbuf_trim_trailing_newline(&buf);
> +
> + /* Start of a new WWW-Authenticate header */
> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
> + while (isspace(*val))
> + val++;
> +
> + strvec_push(values, val);
> + http_auth.header_is_last_match = 1;
> + goto exit;
> + }
OK, this looks correct from my knowledge of the RFCs. I saw something
about isspace() matching newlines, etc, in an earlier thread, but I
think we'd never see a newline here, as we're expecting curl to be
splitting on our behalf.
> + /*
> + * This line could be a continuation of the previously matched header
> + * field. If this is the case then we should append this value to the
> + * end of the previously consumed value.
> + * Continuation lines start with at least one whitespace, maybe more,
> + * so we should collapse these down to a single SP (valid per the spec).
> + */
> + if (http_auth.header_is_last_match && isspace(*buf.buf)) {
> + /* Trim leading whitespace from this continuation hdr line. */
> + strbuf_ltrim(&buf);
OK, makes sense. This will memmove(), which is needlessly inefficient
(we could just advance a pointer), but probably not a big deal in
practice. Using the strbuf functions is a nice simplification.
> + /*
> + * At this point we should always have at least one existing
> + * value, even if it is empty. Do not bother appending the new
> + * value if this continuation header is itself empty.
> + */
> + if (!values->nr) {
> + BUG("should have at least one existing header value");
> + } else if (buf.len) {
> + char *prev = xstrdup(values->v[values->nr - 1]);
> +
> + /* Join two non-empty values with a single space. */
> + const char *const sp = *prev ? " " : "";
> +
> + strvec_pop(values);
> + strvec_pushf(values, "%s%s%s", prev, sp, buf.buf);
> + free(prev);
> + }
Likewise here we end up with an extra allocation of "prev", just because
we can't pop/push in the right order. But that's probably OK in
practice, as this is triggering only for the header we care about.
The concatenation itself makes the whole thing quadratic, but unless we
are worried about a malicious server DoS-ing us with a billion
www-authenticate continuations, I think we can disregard that.
-Peff
^ permalink raw reply
* Re: [PATCH v7 10/12] http: replace unsafe size_t multiplication with st_mult
From: Jeff King @ 2023-01-26 10:09 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason
In-Reply-To: <4b1635b3f6968f8d755bdf6bc4ec7af77aefd315.1674252531.git.gitgitgadget@gmail.com>
On Fri, Jan 20, 2023 at 10:08:48PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> Replace direct multiplication of two size_t parameters in curl response
> stream handling callback functions with `st_mult` to guard against
> overflows.
Hmm. So part of me says that more overflow detection is better than
less, but...I really doubt this is doing anything, and it feels odd to
me to do overflow checks when there is no allocation.
There are tons of integer multiplications in Git. Our usual strategy is
to try to handle overflow like this when we're about to allocate a
buffer, with the idea that we'll avoid a truncated size (that we may
later fill with too many bytes).
In these cases, we could possibly avoid a weird or wrong result due to
truncation, but I don't see how that is different than most of the rest
of Git. What makes these worth touching?
Moreover...
> @@ -176,7 +176,7 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
>
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> {
> - size_t size = eltsize * nmemb;
> + size_t size = st_mult(eltsize, nmemb);
> struct strbuf *buffer = buffer_;
>
> strbuf_add(buffer, ptr, size);
The caller is already claiming to have eltsize*nmemb bytes accessible
via "ptr". How did it get such a buffer if that overflows size_t?
> diff --git a/http.c b/http.c
> index 8a5ba3f4776..a2a80318bb2 100644
> --- a/http.c
> +++ b/http.c
> @@ -146,7 +146,7 @@ static int http_schannel_use_ssl_cainfo;
>
> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> {
> - size_t size = eltsize * nmemb;
> + size_t size = st_mult(eltsize, nmemb);
> struct buffer *buffer = buffer_;
>
> if (size > buffer->buf.len - buffer->posn)
Likewise the caller is asking us to fill a buffer that is eltsize*nmemb.
So they must have allocated it already. How can it be bigger than a
size_t?
In practice, of course, these are both coming from curl, and I strongly
suspect that curl always sets "1" for eltsize anyway, since it's working
with bytes. The two fields only exist to conform to the weird fread()
interface for historical reasons.
So I don't think this patch is really hurting much. It just feels like a
weird one-off that makes the code inconsistent. If somebody who was
wanting to write similar code later asks "why is this one st_mult(), but
not other multiplications", I wouldn't have an answer for them.
-Peff
^ permalink raw reply
* Re: [PATCH v7 08/12] test-http-server: add simple authentication
From: Jeff King @ 2023-01-26 10:02 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason
In-Reply-To: <b8d3e81b5534148359c7e92807cf1e2795480ddf.1674252531.git.gitgitgadget@gmail.com>
On Fri, Jan 20, 2023 at 10:08:46PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> +struct auth_module {
> + char *scheme;
> + char *challenge_params;
> + struct string_list *tokens;
> +};
This is a really minor nit, but: why is "tokens" a pointer? It's always
initialized, so you never need or want to test it for NULL.
That would make this:
> + if (create) {
> + struct auth_module *mod = xmalloc(sizeof(struct auth_module));
> + mod->scheme = xstrdup(scheme);
> + mod->challenge_params = NULL;
> + ALLOC_ARRAY(mod->tokens, 1);
> + string_list_init_dup(mod->tokens);
simplify to:
string_list_init_dup(&mod->tokens);
and one does not have to wonder why we use ALLOC_ARRAY() there, but not
when allocating the module itself. :)
Likewise you could skip freeing it, but since the memory is held until
program end anyway, that doesn't happen either way.
Certainly what you have won't behave wrong; I'd consider this more like
a coding style thing.
> + cat >auth.config <<-EOF &&
> + [auth]
> + challenge = no-params
> + challenge = with-params:foo=\"bar\" p=1
> + challenge = with-params:foo=\"replaced\" q=1
> +
> + token = no-explicit-challenge:valid-token
> + token = no-explicit-challenge:also-valid
> + token = reset-tokens:these-tokens
> + token = reset-tokens:will-be-reset
> + token = reset-tokens:
> + token = reset-tokens:the-only-valid-one
> +
> + allowAnonymous = false
> + EOF
> +
> + cat >OUT.expected <<-EOF &&
> + WWW-Authenticate: no-params
> + WWW-Authenticate: with-params foo="replaced" q=1
> + WWW-Authenticate: no-explicit-challenge
> + WWW-Authenticate: reset-tokens
> +
> + Error: 401 Unauthorized
> + EOF
OK, so I think now we are getting to the interesting part of what your
custom http-server does compared to something like apache. And the
answer so far is: custom WWW-Authenticate lines.
I think we could do that with mod_headers pretty easily. But presumably
we also want to check that we are getting the correct tokens, generate a
401, etc.
I suspect this could all be done as a CGI wrapping git-http-backend. You
can influence the HTTP response code by sending:
Status: 401 Authorization Required
WWW-Authenticate: whatever you want
And likewise you can see what the client sends by putting something like
this in apache.conf:
SetEnvIf Authorization "(.*)" HTTP_AUTHORIZATION=$1
and then reading $HTTP_AUTHORIZATION as you like. At that point, it
feels like a simple shell or perl script could then decide whether to
return a 401 or not (and if not, then just exec git-http-backend to do
the rest). And the scripts would be simple enough that you could have
individual scripts to implement various schemes, rather than
implementing this configuration scheme. You can control which script is
run based on the URL; see the way we match /broken_smart/, etc, in
t/lib-httpd/apache.conf.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox