* Re: [PATCH 5/5] strbuf: simplify strbuf_expand_literal_cb()
From: Jeff King @ 2023-06-27 20:21 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <3654f95d-5709-28f6-eeb2-ca62a1ee789c@web.de>
On Tue, Jun 27, 2023 at 06:32:22PM +0200, René Scharfe wrote:
> A format-advancing version would also look a bit weird in an if/else
> cascade:
>
> else if (strbuf_expand_literal(&sb, &format))
> ; /* nothing */
> else ...
>
> > I dunno. It is a little thing, and I am OK with it either way (I would
> > not even think of changing it if you were not already touching the
> > function).
>
> Unsure. Should I overcome my horror vacui and let the /* nothing */ in?
Heh. It is a little funny to have an empty block. But at the same time,
it aligns the conditional with all of the skip_prefix() calls, which are
advancing "format" as a side effect of matching.
So I could go either way (and we can always change it on top).
I think based on your responses that there's nothing that would require
a re-roll. The only thing left from my review is the extra parentheses
in format_commit_item, which could possibly be fixed up while applying.
-Peff
^ permalink raw reply
* Re: [PATCH] ll-merge: killing the external merge driver aborts the merge
From: Joshua Hudson @ 2023-06-27 21:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Elijah Newren
In-Reply-To: <xmqqttusfz9b.fsf@gitster.g>
On 6/27/2023 1:04 PM, Junio C Hamano wrote:
> Joshua Hudson <jhudson@cedaron.com> writes:
>
>> On 6/27/2023 12:08 PM, Junio C Hamano wrote:
>>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>>
>>>> On Fri, 23 Jun 2023, Junio C Hamano wrote:
>>>>
>>>>> Junio C Hamano <gitster@pobox.com> writes:
>>>>>
>>>>>> Elijah Newren <newren@gmail.com> writes:
>>>>>>
>>>>>>> Reviewed-by: Elijah Newren <newren@gmail.com>
>>>>>> Thanks for a quick review.
>>>>> Unfortunately Windows does not seem to correctly detect the aborting
>>> Sorry, I did not mean "abort(3)" literally. What I meant was that
>>> an external merge driver that gets spawned via the run_command()
>>> interface may not die by calling exit()---like "killed by signal"
>>> (including "segfaulting"). The new test script piece added in the
>>> patch did "kill -9 $$" to kill the external merge driver itself,
>>> which gets reported as "killed by signal" from run_command() by
>>> returning the signal number + 128, but that did not pass Windows CI.
>>>
>> Do you need me to provide a windows test harness?
> Sorry, I do not understand the question.
>
> FWIW how "external merge driver that kills itself by sending a
> signal to itself does not get noticed on Windows" appears in our
> tests can be seen at
>
> https://github.com/git/git/actions/runs/5360824580/jobs/9727137272
>
> The job is "win test(0)", part of our standard Windows test harness
> implemented as part of our GitHub Actions CI test.
>
> Thanks.
Try changing kill -9 $$ to exit 137 # 128 + 9
^ permalink raw reply
* Re: [PATCH] ll-merge: killing the external merge driver aborts the merge
From: Junio C Hamano @ 2023-06-27 21:26 UTC (permalink / raw)
To: Joshua Hudson; +Cc: Johannes Schindelin, git, Elijah Newren
In-Reply-To: <cd5046d4-d66c-9dac-87e7-cdb638124170@cedaron.com>
Joshua Hudson <jhudson@cedaron.com> writes:
> Try changing kill -9 $$ to exit 137 # 128 + 9
Yeah, but then (1) we are not simulating a case where the external
merge driver hits a segfault or receives a signal from outside and
dies involuntarily, and (2) we are codifying that even on Windows,
program that was killed by signal N must exit with 128 + N, and
these are the reasons why I did not go that route.
Stepping back a bit, how does one typically diagnose programatically
on Windows, after "spawning" a separate program, if the program died
involuntarily and/or got killed? It does not have to be "exit with
128 + signal number"---as long as it can be done programatically and
reliably, we would be happy. The code to diagnose how the spawned
program exited in run_command(), which is in finish_command() and
then in wait_or_whine(), may have to be updated with such a piece of
Windows specific knowledge.
^ permalink raw reply
* Re: [PATCH] ll-merge: killing the external merge driver aborts the merge
From: Joshua Hudson @ 2023-06-27 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Elijah Newren
In-Reply-To: <xmqqilb8fvgu.fsf@gitster.g>
On 6/27/2023 2:26 PM, Junio C Hamano wrote:
> Joshua Hudson <jhudson@cedaron.com> writes:
>
>> Try changing kill -9 $$ to exit 137 # 128 + 9
> Yeah, but then (1) we are not simulating a case where the external
> merge driver hits a segfault or receives a signal from outside and
> dies involuntarily, and (2) we are codifying that even on Windows,
> program that was killed by signal N must exit with 128 + N, and
> these are the reasons why I did not go that route.
>
> Stepping back a bit, how does one typically diagnose programatically
> on Windows, after "spawning" a separate program, if the program died
> involuntarily and/or got killed? It does not have to be "exit with
> 128 + signal number"---as long as it can be done programatically and
> reliably, we would be happy. The code to diagnose how the spawned
> program exited in run_command(), which is in finish_command() and
> then in wait_or_whine(), may have to be updated with such a piece of
> Windows specific knowledge.
abort() => 3
Killed => no you can't detect it
Faulted => exit code has the high bit set ( >= 0x8000000 )
My starting off with "the logical equivalent of calling abort()" has
proven to be an unfortunate word choice. I need to harden up the exit
pathway on my side _anyway_. An OOM at least does turn into Faulted.
^ permalink raw reply
* What's cooking in git.git (Jun 2023, #07; Tue, 27)
From: Junio C Hamano @ 2023-06-27 22:11 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 and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
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']
* jk/commit-use-no-divider-with-interpret-trailers (2023-06-16) 1 commit
(merged to 'next' on 2023-06-20 at 08e5f2a6b5)
+ commit: pass --no-divider to interpret-trailers
When "git commit --trailer=..." invokes the interpret-trailers
machinery, it knows what it feeds to interpret-trailers is a full
log message without any patch, but failed to express that by
passing the "--no-divider" option, which has been corrected.
source: <20230617042624.GA562686@coredump.intra.peff.net>
* jk/redact-h2h3-headers-fix (2023-06-17) 1 commit
(merged to 'next' on 2023-06-20 at c1247fd527)
+ http: handle both "h2" and "h2h3" in curl info lines
Curl library recently changed how http2 traces are shown and broke
the code to redact sensitive info header, which has been fixed.
The GitHub Actions CI has been broken for recent macOS jobs, so
this topic has been fast-tracked to help them.
source: <20230617051559.GD562686@coredump.intra.peff.net>
* tb/collect-pack-filenames-fix (2023-06-12) 1 commit
(merged to 'next' on 2023-06-20 at abcc6892c8)
+ builtin/repack.c: only collect fully-formed packs
Avoid breakage of "git pack-objects --cruft" due to inconsistency
between the way the code enumerates packfiles in the repository.
source: <20230607101617.ges6tnMry4E52lDGld43QgtNUsIS4YQq6w-t71hEfkQ@z>
--------------------------------------------------
[New Topics]
* mh/credential-erase-improvements-more (2023-06-24) 2 commits
- credential/wincred: erase matching creds only
- credential/libsecret: erase matching creds only
Needs review.
source: <pull.1529.git.git.1687596777147.gitgitgadget@gmail.com>
* tb/gc-recent-object-hook (2023-06-24) 1 commit
(merged to 'next' on 2023-06-24 at e8c295841b)
+ t7701: make annotated tag unreachable
Test update.
Will merge to 'master'.
source: <259b1b559114ab1a9a0bd7f1ad29a4cba2612ae0.1687617197.git.me@ttaylorr.com>
* gc/config-context (2023-06-26) 12 commits
- config: pass source to config_parser_event_fn_t
- config: add kvi.path, use it to evaluate includes
- config.c: remove config_reader from configsets
- config: pass kvi to die_bad_number()
- trace2: plumb config kvi
- config.c: pass ctx with CLI config
- builtin/config.c: test misuse of format_config()
- config: pass ctx with config files
- config.c: pass ctx in configsets
- config: add ctx arg to config_fn_t
- urlmatch.h: use config_fn_t type
- config: inline git_color_default_config
source: <pull.1497.v4.git.git.1687803083.gitgitgadget@gmail.com>
* gc/config-partial-submodule-kvi-fix (2023-06-26) 1 commit
- config: don't BUG when both kvi and source are set
source: <pull.1535.git.git.1687801297404.gitgitgadget@gmail.com>
* pw/apply-too-large (2023-06-26) 1 commit
- apply: improve error messages when reading patch
source: <pull.1552.git.1687772253869.gitgitgadget@gmail.com>
* pw/diff-no-index-from-named-pipes (2023-06-27) 3 commits
. diff --no-index: support reading from named pipes
. t4054: test diff --no-index with stdin
. diff --no-index: die on error reading stdin
source: <cover.1687874975.git.phillip.wood@dunelm.org.uk>
--------------------------------------------------
[Stalled]
* ed/fsmonitor-windows-named-pipe (2023-03-24) 1 commit
- fsmonitor: handle differences between Windows named pipe functions
Fix fsmonitor on Windows when the filesystem path contains certain
characters.
Expecting a reroll.
cf. <b9cf67e4-22a7-2ff0-8310-9223bea10d6d@jeffhostetler.com>
source: <pull.1503.git.1679678090412.gitgitgadget@gmail.com>
* rn/sparse-diff-index (2023-04-10) 1 commit
- diff-index: enable sparse index
"git diff-index" command has been taught to work better with the
sparse index.
Expecting a reroll.
cf. <62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com>
source: <20230408112342.404318-1-nanth.raghul@gmail.com>
* es/recurse-submodules-option-is-a-bool (2023-04-10) 1 commit
- usage: clarify --recurse-submodules as a boolean
The "--[no-]recurse-submodules" option of "git checkout" and others
supported an undocumented syntax --recurse-submodules=<value> where
the value can spell a Boolean in various ways. The support for the
syntax is being dropped.
Expecting a reroll.
cf. <ZDSTFwMFO7vbj/du@google.com>
source: <ZDSTFwMFO7vbj/du@google.com>
* cb/checkout-same-branch-twice (2023-03-22) 2 commits
- SQUASH??? the test marked to expect failure passes from day one
- 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 minor and final reroll.
cf. <CAPUEspj_Bh+LgYLnWfeBdcq_uV5Cbou-7H51GLFjzSa5Qzby9w@mail.gmail.com>
source: <20230120113553.24655-1-carenas@gmail.com>
* tk/pull-conflict-suggest-rebase-merge-not-rebase-true (2023-02-13) 1 commit
- pull: conflict hint pull.rebase suggestion should offer "merges" vs "true"
In an advice message after failed non-ff pull, we used to suggest
setting pull.rebase=true, but these days pull.rebase=merges may be
more inline with the original spirit of "rebuild your side on top
of theirs".
May want to discard.
This is too much of a departure from the existing practice.
cf. <CAMMLpeTPEoKVTbfc17w+Y9qn7jOGmQi_Ux0Y3sFW5QTgGWJ=SA@mail.gmail.com>
cf. <CABPp-BGqAxKnxDRVN4cYMteLp33hvto07R3=TJBT5WubJT4+Og@mail.gmail.com>
source: <pull.1474.git.1675614276549.gitgitgadget@gmail.com>
* ab/tag-object-type-errors (2023-05-10) 4 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
- 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.
source: <cover-v2-0.3-00000000000-20221230T011725Z-avarab@gmail.com>
* 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.
Expecting a reroll.
cf. <CABPp-BGoPuGCZw+9wCgdYyRR4Zf4y9Kun27GrQhtMdYWpOUsYQ@mail.gmail.com>
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>
* ab/imap-send-requires-curl (2023-02-02) 6 commits
- imap-send: correctly report "host" when using "tunnel"
- imap-send: remove old --no-curl codepath
- imap-send: make --curl no-optional
- imap-send: replace auto-probe libcurl with hard dependency
- imap-send doc: the imap.sslVerify is used with imap.tunnel
- imap-send: note "auth_method", not "host" on auth method failure
Give a hard dependency on cURL library to build "git imap-send",
and remove the code to interact with IMAP server without using cURL.
Expecting a reroll.
The 'tunnel' part is still iffy.
cf. <230203.86bkmabfjr.gmgdl@evledraar.gmail.com>
source: <cover-v2-0.6-00000000000-20230202T093706Z-avarab@gmail.com>
* cw/submodule-status-in-parallel (2023-03-02) 6 commits
- diff-lib: parallelize run_diff_files for submodules
- diff-lib: refactor out diff_change logic
- submodule: refactor is_submodule_modified()
- submodule: move status parsing into function
- submodule: rename strbuf variable
- run-command: add on_stderr_output_fn to run_processes_parallel_opts
"git submodule status" learned to run the comparison in submodule
repositories in parallel.
Expecting a reroll.
cf. <CAFySSZDk05m6gU5-V1R+y3YnQ5PPduVW54+_gjBwD0rmacsLsw@mail.gmail.com>
cf. <230307.865ybc273g.gmgdl@evledraar.gmail.com>
source: <20230302215237.1473444-1-calvinwan@google.com>
--------------------------------------------------
[Cooking]
* ds/remove-idx-before-pack (2023-06-20) 1 commit
(merged to 'next' on 2023-06-23 at fa97bf0e41)
+ packfile: delete .idx files before .pack files
We create .pack and then .idx, we consider only packfiles that have
.idx usable (those with only .pack are not ready yet), so we should
remove .idx before removing .pack for consistency.
Will merge to 'master'.
source: <pull.1547.git.1687287675248.gitgitgadget@gmail.com>
* bc/more-git-var (2023-06-27) 8 commits
(merged to 'next' on 2023-06-27 at ea14687e91)
+ var: add config file locations
+ var: add attributes files locations
+ attr: expose and rename accessor functions
+ var: adjust memory allocation for strings
+ var: format variable structure with C99 initializers
+ var: add support for listing the shell
+ t: add a function to check executable bit
+ var: mark unused parameters in git_var callbacks
<<
* bc/more-git-var (2023-06-27) 8 commits
. var: add config file locations
. var: add attributes files locations
. attr: expose and rename accessor functions
. var: adjust memory allocation for strings
. var: format variable structure with C99 initializers
. var: add support for listing the shell
. t: add a function to check executable bit
. var: mark unused parameters in git_var callbacks
>>
Add more "git var" for toolsmiths to learn various locations Git is
configured with either via the configuration or hardcoded defaults.
Will merge to 'master'.
source: <20230627161902.754472-1-sandals@crustytoothpaste.net>
* jc/doc-hash-object-types (2023-06-23) 1 commit
- docs: add git hash-object -t option's possible values
Doc update.
Will merge to 'next'.
source: <pull.1533.v2.git.git.1687555504551.gitgitgadget@gmail.com>
* jc/abort-ll-merge-with-a-signal (2023-06-23) 2 commits
(merged to 'next' on 2023-06-24 at 685eb5d25c)
+ t6406: skip "external merge driver getting killed by a signal" test on Windows
(merged to 'next' on 2023-06-23 at 9c9c37e95e)
+ ll-merge: killing the external merge driver aborts the merge
When the external merge driver is killed by a signal, its output
should not be trusted as a resolution with conflicts that is
proposed by the driver, but the code did.
Will merge to 'master'.
source: <xmqq4jmzc91e.fsf_-_@gitster.g>
* cc/repack-sift-filtered-objects-to-separate-pack (2023-06-14) 9 commits
- gc: add `gc.repackFilterTo` config option
- repack: implement `--filter-to` for storing filtered out objects
- gc: add `gc.repackFilter` config option
- repack: add `--filter=<filter-spec>` option
- repack: refactor finishing pack-objects command
- repack: refactor piping an oid to a command
- t/helper: add 'find-pack' test-tool
- pack-objects: add `--print-filtered` to print omitted objects
- pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
Needs review.
source: <20230614192541.1599256-1-christian.couder@gmail.com>
* ps/revision-stdin-with-options (2023-06-15) 3 commits
(merged to 'next' on 2023-06-26 at eda3e4d0b5)
+ revision: handle pseudo-opts in `--stdin` mode
+ revision: small readability improvement for reading from stdin
+ revision: reorder `read_revisions_from_stdin()`
The set-up code for the get_revision() API now allows feeding
options like --all and --not in the --stdin mode.
Will merge to 'master'.
source: <cover.1686839572.git.ps@pks.im>
* rs/strbuf-expand-step (2023-06-18) 5 commits
- strbuf: simplify strbuf_expand_literal_cb()
- replace strbuf_expand() with strbuf_expand_step()
- replace strbuf_expand_dict_cb() with strbuf_expand_step()
- strbuf: factor out strbuf_expand_step()
- pretty: factor out expand_separator()
Code clean-up around strbuf_expand() API.
Will merge to 'next'.
source: <767baa64-20a6-daf2-d34b-d81f72363749@web.de>
* js/doc-unit-tests (2023-06-13) 1 commit
- unit tests: Add a project plan document
Process to add some form of low-level unit tests has started.
Comments? Filling in blanks?
source: <8afdb215d7e10ca16a2ce8226b4127b3d8a2d971.1686352386.git.steadmon@google.com>
* mh/mingw-case-sensitive-build (2023-06-12) 1 commit
- mingw: use lowercase includes for some Windows headers
Names of MinGW header files are spelled in mixed case in some
source files, but the build host can be using case sensitive
filesystem with header files with their name spelled in all
lowercase.
Needs review.
source: <20230604211934.1365289-1-mh@glandium.org>
* pb/complete-diff-options (2023-06-26) 24 commits
- diff.c: mention completion above add_diff_options
- completion: complete --remerge-diff
- completion: complete --diff-merges, its options and --no-diff-merges
- completion: move --pickaxe-{all,regex} to __git_diff_common_options
- completion: complete --ws-error-highlight
- completion: complete --unified
- completion: complete --output-indicator-{context,new,old}
- completion: complete --output
- completion: complete --no-stat
- completion: complete --no-relative
- completion: complete --line-prefix
- completion: complete --ita-invisible-in-index and --ita-visible-in-index
- completion: complete --irreversible-delete
- completion: complete --ignore-matching-lines
- completion: complete --function-context
- completion: complete --find-renames
- completion: complete --find-object
- completion: complete --find-copies
- completion: complete --default-prefix
- completion: complete --compact-summary
- completion: complete --combined-all-paths
- completion: complete --cc
- completion: complete --break-rewrites
- completion: add comments describing __git_diff_* globals
Completion updates.
Will merge to 'next'.
source: <pull.1543.v3.git.1687796688.gitgitgadget@gmail.com>
* ks/ref-filter-signature (2023-06-06) 2 commits
- ref-filter: add new "signature" atom
- t/lib-gpg: introduce new prereq GPG2
The "git for-each-ref" family of commands learned placeholders
related to GPG signature verification.
Needs review.
source: <20230604185815.15761-1-five231003@gmail.com>
* jt/path-filter-fix (2023-06-13) 4 commits
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- gitformat-commit-graph: describe version 2 of BDAT
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Expecting a reroll.
cf. <c7b66d2c-cdc3-1f0f-60a0-a2ee21c277bf@github.com>
source: <cover.1686677910.git.jonathantanmy@google.com>
* tk/cherry-pick-sequence-requires-clean-worktree (2023-06-01) 1 commit
- cherry-pick: refuse cherry-pick sequence if index is dirty
"git cherry-pick A" that replays a single commit stopped before
clobbering local modification, but "git cherry-pick A..B" did not,
which has been corrected.
Expecting a reroll.
cf. <999f12b2-38d6-f446-e763-4985116ad37d@gmail.com>
source: <pull.1535.v2.git.1685264889088.gitgitgadget@gmail.com>
* mh/credential-libsecret-attrs (2023-06-16) 1 commit
- credential/libsecret: store new attributes
The way authentication related data other than passwords (e.g.
oath token and password expiration data) are stored in libsecret
keyrings has been rethought.
Needs review.
source: <pull.1469.v5.git.git.1686945306242.gitgitgadget@gmail.com>
* tb/refs-exclusion-and-packed-refs (2023-06-20) 16 commits
- ls-refs.c: avoid enumerating hidden refs where possible
- upload-pack.c: avoid enumerating hidden refs where possible
- builtin/receive-pack.c: avoid enumerating hidden references
- refs.h: let `for_each_namespaced_ref()` take excluded patterns
- refs/packed-backend.c: ignore complicated hidden refs rules
- revision.h: store hidden refs in a `strvec`
- refs/packed-backend.c: add trace2 counters for jump list
- refs/packed-backend.c: implement jump lists to avoid excluded pattern(s)
- refs/packed-backend.c: refactor `find_reference_location()`
- refs: plumb `exclude_patterns` argument throughout
- builtin/for-each-ref.c: add `--exclude` option
- ref-filter.c: parameterize match functions over patterns
- ref-filter: add `ref_filter_clear()`
- ref-filter: clear reachable list pointers after freeing
- ref-filter.h: provide `REF_FILTER_INIT`
- refs.c: rename `ref_filter`
Enumerating refs in the packed-refs file, while excluding refs that
match certain patterns, has been optimized.
source: <cover.1687270849.git.me@ttaylorr.com>
* en/header-split-cache-h-part-3 (2023-06-21) 28 commits
(merged to 'next' on 2023-06-23 at 84ad22bf36)
+ fsmonitor-ll.h: split this header out of fsmonitor.h
+ hash-ll, hashmap: move oidhash() to hash-ll
+ object-store-ll.h: split this header out of object-store.h
+ khash: name the structs that khash declares
+ merge-ll: rename from ll-merge
+ git-compat-util.h: remove unneccessary include of wildmatch.h
+ builtin.h: remove unneccessary includes
+ list-objects-filter-options.h: remove unneccessary include
+ diff.h: remove unnecessary include of oidset.h
+ repository: remove unnecessary include of path.h
+ log-tree: replace include of revision.h with simple forward declaration
+ cache.h: remove this no-longer-used header
+ read-cache*.h: move declarations for read-cache.c functions from cache.h
+ repository.h: move declaration of the_index from cache.h
+ merge.h: move declarations for merge.c from cache.h
+ diff.h: move declaration for global in diff.c from cache.h
+ preload-index.h: move declarations for preload-index.c from elsewhere
+ sparse-index.h: move declarations for sparse-index.c from cache.h
+ name-hash.h: move declarations for name-hash.c from cache.h
+ run-command.h: move declarations for run-command.c from cache.h
+ statinfo: move stat_{data,validity} functions from cache/read-cache
+ read-cache: move shared add/checkout/commit code
+ add: modify add_files_to_cache() to avoid globals
+ read-cache: move shared commit and ls-files code
+ setup: adopt shared init-db & clone code
+ init-db, clone: change unnecessary global into passed parameter
+ init-db: remove unnecessary global variable
+ init-db: document existing bug with core.bare in template config
Header files cleanup.
Will merge to 'master'.
source: <pull.1525.v3.git.1684218848.gitgitgadget@gmail.com>
* cc/git-replay (2023-06-03) 15 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- replay: disallow revision specific options and pathspecs
- replay: use standard revision ranges
- replay: make it a minimal server side command
- replay: remove HEAD related sanity check
- replay: remove progress and info output
- replay: add an important FIXME comment about gpg signing
- replay: don't simplify history
- replay: introduce pick_regular_commit()
- replay: die() instead of failing assert()
- replay: start using parse_options API
- replay: introduce new builtin
- t6429: remove switching aspects of fast-rebase
source: <20230602102533.876905-1-christian.couder@gmail.com>
* ob/revert-of-revert (2023-05-05) 1 commit
- sequencer: beautify subject of reverts of reverts
Instead of "Revert "Revert "original"", give "Reapply "original""
as the title for a revert of a revert.
Expecting a hopefully final reroll.
Looking much better, except for minor cosmetic issues.
source: <20230428083528.1699221-1-oswald.buddenhagen@gmx.de>
* cw/strbuf-cleanup (2023-06-12) 7 commits
- strbuf: remove global variable
- path: move related function to path
- object-name: move related functions to object-name
- credential-store: move related functions to credential-store file
- abspath: move related functions to abspath
- strbuf: clarify dependency
- strbuf: clarify API boundary
Move functions that are not about pure string manipulation out of
strbuf.[ch]
Will merge to 'next'.
source: <20230606194720.2053551-1-calvinwan@google.com>
* tl/notes-separator (2023-06-21) 7 commits
- notes: introduce "--no-separator" option
- notes.c: introduce "--[no-]stripspace" option
- notes.c: append separator instead of insert by pos
- notes.c: introduce '--separator=<paragraph-break>' option
- t3321: add test cases about the notes stripspace behavior
- notes.c: use designated initializers for clarity
- notes.c: cleanup 'strbuf_grow' call in 'append_edit'
'git notes append' was taught '--separator' to specify string to insert
between paragraphs.
source: <cover.1685174011.git.dyroneteng@gmail.com>
* pw/rebase-i-after-failure (2023-04-21) 6 commits
- rebase -i: fix adding failed command to the todo list
- rebase: fix rewritten list for failed pick
- rebase --continue: refuse to commit after failed command
- sequencer: factor out part of pick_commits()
- rebase -i: remove patch file after conflict resolution
- rebase -i: move unlink() calls
Various fixes to the behaviour of "rebase -i" when the command got
interrupted by conflicting changes.
Expecting a reroll.
cf. <xmqqsfcthrpb.fsf@gitster.g>
cf. <1fd54422-b66a-c2e4-7cd7-934ea01190ad@gmail.com>
source: <pull.1492.v2.git.1682089074.gitgitgadget@gmail.com>
--------------------------------------------------
[Discarded]
* mh/credential-password-expiry-libsecret (2023-05-05) 1 commit
. credential/libsecret: support password_expiry_utc
Originally merged to 'next' on 2023-05-09
The libsecret credential helper learns to handle the password
expiry time information.
Superseded by mh/credential-libsecret-attrs.
cf. <CAGJzqskMwOJkriH6serqdwAVYi+fftEL8ohJd-suP6v+OxB_bg@mail.gmail.com>
source: <pull.1469.v3.git.git.1683270298313.gitgitgadget@gmail.com>
* tb/pack-bitmap-index-seek (2023-03-20) 6 commits
. pack-bitmap.c: factor out `bitmap_index_seek_commit()`
. pack-bitmap.c: use `bitmap_index_seek()` where possible
. pack-bitmap.c: factor out manual `map_pos` manipulation
. pack-bitmap.c: drop unnecessary 'inline's
. pack-bitmap.c: hide bitmap internals in `read_be32()`
. pack-bitmap.c: hide bitmap internals in `read_u8()`
Clean-up the pack-bitmap codepath.
Retracted for now.
cf. <ZJCI6FHtbuOKPlV1@nand.local>
source: <cover.1679342296.git.me@ttaylorr.com>
* js/cmake-wo-cache-h (2023-06-15) 1 commit
. cmake: adapt to `cache.h` being no more
Build fix in en/header-split-cache-h-part-3 topic.
Ejected out of 'next' and made into a part of en/header-split-cache-h-part-3
source: <pull.1525.v3.git.1684218848.gitgitgadget@gmail.com>
* jc/notes-separator-fix (2023-06-13) 7 commits
. notes: do not access before the beginning of an array
Fix to tl/notes-separator topic.
Discarded as the updated base topic should not require it anymore.
source: <cover.1682671758.git.dyroneteng@gmail.com>
^ permalink raw reply
* [PATCH v2] fix cherry-pick/revert status when doing multiple commits
From: Jacob Keller @ 2023-06-27 22:41 UTC (permalink / raw)
To: git, Phillip Wood; +Cc: Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
The status report for an in-progress cherry-pick does not show the
current commit if the cherry-pick happens as part of a series of
multiple commits:
$ git cherry-pick <commit1> <commit2>
< one of the cherry-picks fails to merge clean >
Cherry-pick currently in progress.
(run "git cherry-pick --continue" to continue)
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
$ git status
On branch <branch>
Your branch is ahead of '<upstream>' by 1 commit.
(use "git push" to publish your local commits)
Cherry-pick currently in progress.
(run "git cherry-pick --continue" to continue)
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
The show_cherry_pick_in_progress() function prints "Cherry-pick
currently in progress". That function does have a more verbose print
based on whether the cherry_pick_head_oid is null or not. If it is not
null, then a more helpful message including which commit is actually
being picked is displayed.
The introduction of the "Cherry-pick currently in progress" message
comes from 4a72486de97b ("fix cherry-pick/revert status after commit",
2019-04-17). This commit modified wt_status_get_state() in order to
detect that a cherry-pick was in progress even if the user has used `git
commit` in the middle of the sequence.
The check used to detect this is the call to sequencer_get_last_command.
If the sequencer indicates that the lass command was a REPLAY_PICK, then
the state->cherry_pick_in_progress is set to 1 and the
cherry_pick_head_oid is initialized to the null_oid. Similar behavior is
done for the case of REPLAY_REVERT.
It happens that this call of sequencer_get_last_command will always
report the action even if the user hasn't interrupted anything. Thus,
during a range of cherry-picks or reverts, the cherry_pick_head_oid and
revert_head_oid will always be overwritten and initialized to the null
oid.
This results in status always displaying the terse message which does
not include commit information.
Fix this by adding an additional check so that we do not re-initialize
the cherry_pick_head_oid or revert_head_oid if we have already set the
cherry_pick_in_progress or revert_in_progress bits. This ensures that
git status will display the more helpful information when its available.
Add a test case covering this behavior.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes since v1:
* add the missing test case that I had locally but forgot to squash in
* use else if as suggested by Phillip
t/t7512-status-help.sh | 22 ++++++++++++++++++++++
wt-status.c | 4 ++--
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 2f16d5787edf..c2ab8a444a83 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -774,6 +774,28 @@ EOF
test_cmp expected actual
'
+test_expect_success 'status when cherry-picking multiple commits' '
+ git reset --hard cherry_branch &&
+ test_when_finished "git cherry-pick --abort" &&
+ test_must_fail git cherry-pick cherry_branch_second one_cherry &&
+ TO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&
+ cat >expected <<EOF &&
+On branch cherry_branch
+You are currently cherry-picking commit $TO_CHERRY_PICK.
+ (fix conflicts and run "git cherry-pick --continue")
+ (use "git cherry-pick --skip" to skip this patch)
+ (use "git cherry-pick --abort" to cancel the cherry-pick operation)
+
+Unmerged paths:
+ (use "git add <file>..." to mark resolution)
+ both modified: main.txt
+
+no changes added to commit (use "git add" and/or "git commit -a")
+EOF
+ git status --untracked-files=no >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'status when cherry-picking after committing conflict resolution' '
git reset --hard cherry_branch &&
test_when_finished "git cherry-pick --abort" &&
diff --git a/wt-status.c b/wt-status.c
index 068b76ef6d96..8d23ff8ced23 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1790,10 +1790,10 @@ void wt_status_get_state(struct repository *r,
oidcpy(&state->revert_head_oid, &oid);
}
if (!sequencer_get_last_command(r, &action)) {
- if (action == REPLAY_PICK) {
+ if (action == REPLAY_PICK && !state->cherry_pick_in_progress) {
state->cherry_pick_in_progress = 1;
oidcpy(&state->cherry_pick_head_oid, null_oid());
- } else {
+ } else if (action == REPLAY_REVERT && !state->revert_in_progress) {
state->revert_in_progress = 1;
oidcpy(&state->revert_head_oid, null_oid());
}
--
2.41.0.1.g9857a21e0017.dirty
^ permalink raw reply related
* Re: [RFC PATCH 5/8] parse: create new library for parsing strings and env values
From: Junio C Hamano @ 2023-06-27 22:58 UTC (permalink / raw)
To: Calvin Wan; +Cc: git, nasamuffin, chooglen, johnathantanmy
In-Reply-To: <20230627195251.1973421-6-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> While string and environment value parsing is mainly consumed by
> config.c, there are other files that only need parsing functionality and
> not config functionality. By separating out string and environment value
> parsing from config, those files can instead be dependent on parse,
> which has a much smaller dependency chain than config.
>
> Move general string and env parsing functions from config.[ch] to
> parse.[ch].
Quite sensible and ...
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
> Makefile | 1 +
> attr.c | 2 +-
> config.c | 180 +-----------------------------------
... long overdue to have this.
> config.h | 14 +--
> pack-objects.c | 2 +-
> pack-revindex.c | 2 +-
> parse-options.c | 3 +-
> parse.c | 182 +++++++++++++++++++++++++++++++++++++
> parse.h | 20 ++++
> pathspec.c | 2 +-
> preload-index.c | 2 +-
> progress.c | 2 +-
> prompt.c | 2 +-
> rebase.c | 2 +-
> t/helper/test-env-helper.c | 2 +-
> unpack-trees.c | 2 +-
> wrapper.c | 2 +-
> write-or-die.c | 2 +-
> 18 files changed, 219 insertions(+), 205 deletions(-)
> create mode 100644 parse.c
> create mode 100644 parse.h
It is somewhat surprising and very pleasing to see so many *.c files
had and now can lose dependency on <config.h>. Very nice.
^ permalink raw reply
* Re: [RFC PATCH 6/8] pager: remove pager_in_use()
From: Junio C Hamano @ 2023-06-27 23:00 UTC (permalink / raw)
To: Calvin Wan; +Cc: git, nasamuffin, chooglen, johnathantanmy
In-Reply-To: <20230627195251.1973421-7-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> pager_in_use() is simply a wrapper around
> git_env_bool("GIT_PAGER_IN_USE", 0). Other places that call
> git_env_bool() in this fashion also do not have a wrapper function
> around it. By removing pager_in_use(), we can also get rid of the
> pager.h dependency from a few files.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
> builtin/log.c | 2 +-
> color.c | 2 +-
> column.c | 2 +-
> date.c | 4 ++--
> git.c | 2 +-
> pager.c | 5 -----
> pager.h | 1 -
> 7 files changed, 6 insertions(+), 12 deletions(-)
With so many (read: more than 3) callsites, I am not sure if this is
an improvement. pager_in_use() cannot be misspelt without getting
noticed by compilers, but git_env_bool("GIT_PAGOR_IN_USE", 0) will
go silently unnoticed. Is there no other way to lose the dependency
you do not like?
^ permalink raw reply
* Re: [RFC PATCH 6/8] pager: remove pager_in_use()
From: Calvin Wan @ 2023-06-27 23:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, nasamuffin, chooglen, Jonathan Tan
In-Reply-To: <xmqq1qhwfr46.fsf@gitster.g>
> With so many (read: more than 3) callsites, I am not sure if this is
> an improvement. pager_in_use() cannot be misspelt without getting
> noticed by compilers, but git_env_bool("GIT_PAGOR_IN_USE", 0) will
> go silently unnoticed. Is there no other way to lose the dependency
> you do not like?
I thought about only changing this call site, but that creates an
inconsistency that shouldn't exist. The other way is to move this
function into a different file, but it is also unclear to me which
file that would be. It would be awkward in parse.c and if it was in
environment.c then we would have many more inherited dependencies from
that. I agree that the value of this patch is dubious in and of
itself, which is why it's coupled together with this series rather
than in a separate standalone cleanup series.
^ permalink raw reply
* [PATCH] submodule: show inconsistent .gitmodules precedence
From: Glen Choo via GitGitGadget @ 2023-06-27 23:57 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Glen Choo
From: Glen Choo <chooglen@google.com>
Demonstrate, using tests, an inconsistency in how Git treats repeated
configuration keys in .gitmodules depending on whether we read it from
the working tree or from an object. Do not attempt to fix it yet,
because we don't know how much test coverage we have here, and what the
'right' fix is.
When a .gitmodules file contains multiple configurations for a submodule
like so:
[submodule "sub"]
path = path1
path = path2
It's clearly misconfigured, but our docs don't state what we do in this
situation. If one checks this with "test-tool submodule-config", you'd
see that we ignore every value after the first (aka first-one-wins) and
issue a warning. *But* if you actually tried this with "git submodule",
you'd find it practically impossible to trigger this behavior - what you
actually see is last-one-wins!
The difference between the two is somewhat complicated because there are
two factors at play. The first is a probable bug in how
parse_config_parameter.overwrite affects the way submodule config is
cached. In submodule-config.c:parse_config(), when ".overwrite = 1", the
submodule machinery gladly overwrites the existing value (last-one-wins)
instead of issuing the warning (first-one-wins). This is probably a bug
because it seems that .overwrite is intended to overwrite cached values
from a previous .gitmodules (e.g. if we read .gitmodules from the index
and want to overwrite it with a newer version), not to overwrite values
that we read in the same file.
The second factor is that the two are reading differently cached values:
"git submodule" is reading cached values with ".overwrite = 1", but
test-tool is reading from cached values with ".overwrite = 0". The
submodule cache stores each submodule config based on the submodule name
and the .gitmodules oid it was read from (null_oid() if it's from the
working tree). Both code paths eventually call repo_read_gitmodules() to
eagerly cache .gitmodules from the working tree, and which happens to
use ".overwrite = 1". "git submodule" typically passes null_oid(), which
reads back this value. However, test-tool reads back values from the
actual .gitmodules oid. This causes a cache miss, but when we try to
populate the cache at that oid, we do it with ".overwrite = 0", causing
the difference in behavior.
To make this behavior easy to demonstrate, I've opted to teach test-tool
how to use null_oid() rather than use a real Git command, but this is
almost certainly affecting us in real Git. It's probably flying under
the radar due to some combination of submodule_from_[path|name]() being
relatively uncommon in the codebase, and such misconfigurations being
rare in practice.
We could fix this bug by targeting either of these factors:
- Make ".overwrite = 1" and ".overwrite = 0" do the same thing with
repeated values in a .gitmodules.
- Remove the eager caching (repo_read_gitmodules()). It seems like we
can lazily populate the cache on a miss, so we might not need this.
But I'm not sure how long this has been around, and whether users have
come to expect one over the other, so I've opted not to send a fix yet.
Signed-off-by: Glen Choo <chooglen@google.com>
---
submodule: show inconsistent .gitmodules precedence
While I was writing a .gitmodules parser for jj
(https://github.com/martinvonz/jj, check it out, it's great!), a
reviewer asked what would happen if a submodule had repeated fields
(like .path). It turns out that the answer isn't just undefined (it's
nowhere in the docs), it's inconsistent!
Here's a bug report patch that demonstrates the issue using test-tool.
I've stopped short of sending a fix because 1) I'm frankly not sure what
behavior users have come to rely on 2) this problem is multi-faceted, so
we could fix this in quite a few ways, but I'm not sure which way is
'right'.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1538%2Fchooglen%2Fpush-lzmyuzkpxxxq-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1538/chooglen/push-lzmyuzkpxxxq-v1
Pull-Request: https://github.com/git/git/pull/1538
submodule-config.c | 7 +++++++
t/helper/test-submodule-config.c | 22 +++++++++++++++++++++-
t/t7411-submodule-config.sh | 22 ++++++++++++++++++++++
3 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/submodule-config.c b/submodule-config.c
index 7eb7a0d88d2..1c4b5afa8e4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -441,6 +441,13 @@ static int parse_config(const char *var, const char *value, void *data)
me->gitmodules_oid,
name.buf);
+ /*
+ * FIXME me->overwrite=1 is only meant to overwrite existing submodule
+ * configurations when we're reading from another .gitmodules (e.g. from
+ * another commit), but it also unintentionally changes behavior when
+ * there are multiple configurations in a single .gitmodules - instead
+ * of respecting the first value, we now respect the last value.
+ */
if (!strcmp(item.buf, "path")) {
if (!value)
ret = config_error_nonbool(var);
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index 9df2f03ac80..1bb1dc45878 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -29,11 +29,31 @@ int cmd__submodule_config(int argc, const char **argv)
my_argc--;
}
- if (my_argc % 2 != 0)
+ if (my_argc > 1 && my_argc % 2 != 0)
die_usage(argc, argv, "Wrong number of arguments.");
setup_git_directory();
+ if (my_argc == 1) {
+ const struct submodule *submodule;
+ const char *path_or_name;
+
+ path_or_name = arg[0];
+ if (lookup_name) {
+ submodule = submodule_from_name(the_repository,
+ null_oid(), path_or_name);
+ } else
+ submodule = submodule_from_path(the_repository,
+ null_oid(), path_or_name);
+ if (!submodule)
+ die_usage(argc, argv, "Submodule not found.");
+
+ printf("Submodule name: '%s' for path '%s'\n", submodule->name,
+ submodule->path);
+
+ return 0;
+ }
+
while (*arg) {
struct object_id commit_oid;
const struct submodule *submodule;
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index c0167944abd..b67eea7e085 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -258,4 +258,26 @@ test_expect_success 'reading nested submodules config when .gitmodules is not in
)
'
+test_expect_success 'multiple config fields in .gitmodules' '
+ test_when_finished "rm -fr super-duplicate" &&
+ mkdir super-duplicate &&
+ (cd super-duplicate &&
+ git init &&
+ git submodule add ../submodule &&
+ git config --file .gitmodules --add submodule.submodule.path ignored &&
+ git config --file .gitmodules --add submodule.submodule.url ignored &&
+ git add .gitmodules &&
+ git commit -m "duplicate fields in .gitmodules" &&
+ test-tool submodule-config HEAD submodule >actual 2>warning &&
+ grep "Skipping second one" warning &&
+ echo "Submodule name: ${SQ}submodule${SQ} for path ${SQ}submodule${SQ}" >expect &&
+ test_cmp expect actual &&
+ # FIXME this should give the same result as "HEAD", but there is
+ # a bug where if we use null_oid() instead of the real commit
+ # id, the second .path gets respected instead of the first.
+ test_must_fail test-tool submodule-config submodule 2>null-oid-error &&
+ grep "Submodule not found" null-oid-error
+ )
+'
+
test_done
base-commit: 6ff334181cfb6485d3ba50843038209a2a253907
--
gitgitgadget
^ permalink raw reply related
* Re: [RFC PATCH 0/8] Introduce Git Standard Library
From: Glen Choo @ 2023-06-28 0:14 UTC (permalink / raw)
To: Calvin Wan, git; +Cc: Calvin Wan, nasamuffin, johnathantanmy
In-Reply-To: <20230627195251.1973421-1-calvinwan@google.com>
I see that this doesn't apply cleanly to 'master'. Do you have a base
commit that reviewers can easily apply this to?
Calvin Wan <calvinwan@google.com> writes:
> Before looking at this series, it probably makes sense to look at the
> other series that this is built on top of since that is the state I will
> be referring to in this cover letter:
>
> - Elijah's final cache.h cleanup series[2]
> - my strbuf cleanup series[3]
> - my git-compat-util cleanup series[4]
Unfortunately, not all of these series apply cleanly to 'master' either,
so I went digging for the topic branches, which I think are:
- en/header-split-cache-h-part-3
- cw/header-compat-util-shuffle
- cw/strbuf-cleanup
And then I tried merging them, but it looks like they don't merge
cleanly either :/
(Btw Junio, I think cw/header-compat-util-shuffle didn't get called out
in What's Cooking.)
> [2] https://lore.kernel.org/git/pull.1525.v3.git.1684218848.gitgitgadget@gmail.com/
> [3] https://lore.kernel.org/git/20230606194720.2053551-1-calvinwan@google.com/
> [4] https://lore.kernel.org/git/20230606170711.912972-1-calvinwan@google.com/
^ permalink raw reply
* Re: [RFC PATCH 6/8] pager: remove pager_in_use()
From: Glen Choo @ 2023-06-28 0:30 UTC (permalink / raw)
To: Junio C Hamano, Calvin Wan; +Cc: git, nasamuffin, johnathantanmy
In-Reply-To: <xmqq1qhwfr46.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> pager_in_use() is simply a wrapper around
>> git_env_bool("GIT_PAGER_IN_USE", 0). Other places that call
>> git_env_bool() in this fashion also do not have a wrapper function
>> around it. By removing pager_in_use(), we can also get rid of the
>> pager.h dependency from a few files.
>
> With so many (read: more than 3) callsites, I am not sure if this is
> an improvement. pager_in_use() cannot be misspelt without getting
> noticed by compilers, but git_env_bool("GIT_PAGOR_IN_USE", 0) will
> go silently unnoticed. Is there no other way to lose the dependency
> you do not like?
Having the function isn't just nice for typo prevention - it's also a
reasonable boundary around the pager subsystem. We could imagine a
world where we wanted to track the pager status using a static
var instead of an env var (not that we'd even want that :P), and this
inlining makes that harder.
From the cover letter, it seems like we only need this to remove
"#include pager.h" from date.c, and that's only used in
parse_date_format(). Could we add a is_pager/pager_in_use to that
function and push the pager.h dependency upwards?
^ permalink raw reply
* Re: [PATCH] submodule: show inconsistent .gitmodules precedence
From: Junio C Hamano @ 2023-06-28 1:23 UTC (permalink / raw)
To: Glen Choo via GitGitGadget; +Cc: git, Glen Choo
In-Reply-To: <pull.1538.git.git.1687910254473.gitgitgadget@gmail.com>
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> [submodule "sub"]
> path = path1
> path = path2
>
> It's clearly misconfigured, but our docs don't state what we do in this
> situation. If one checks this with "test-tool submodule-config", you'd
> see that we ignore every value after the first (aka first-one-wins) and
> issue a warning. *But* if you actually tried this with "git submodule",
> you'd find it practically impossible to trigger this behavior - what you
> actually see is last-one-wins!
The last-one-wins sounds like a natural outcome for reusing the
config reading machinery, and the first-one-wins sounds like a total
confusion, but we probably should fail any operation before the user
fixes the .gitmodules by removing all but one path for each
submodule. Otherwise we risk operating on wrong submodules (e.g. we
may think we are running deinit on "sub" at path #1, but the code
may deinit something different).
Thanks.
^ permalink raw reply
* Re: [PATCH] submodule: show inconsistent .gitmodules precedence
From: Glen Choo @ 2023-06-28 1:36 UTC (permalink / raw)
To: Junio C Hamano, Glen Choo via GitGitGadget; +Cc: git
In-Reply-To: <xmqqo7l0e5x3.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> The last-one-wins sounds like a natural outcome for reusing the
> config reading machinery, and the first-one-wins sounds like a total
> confusion, but we probably should fail any operation before the user
> fixes the .gitmodules by removing all but one path for each
> submodule.
An informal poll amongst Googlers suggests that my team mostly agrees:
last-one-wins makes more sense than first-one-wins, but erroring out is
the most sensible thing to do.
I'm not sure how reasonable it is to just fail. It makes sense if we
were only reading .gitmodules from the working tree (the user can fix
that), but we also read .gitmodules from commits, and I don't see (yet)
how a user could reasonably recover from that.
^ permalink raw reply
* Re: [RFC PATCH 1/8] trace2: log fsync stats in trace2 rather than wrapper
From: Victoria Dye @ 2023-06-28 2:05 UTC (permalink / raw)
To: Calvin Wan, git; +Cc: nasamuffin, chooglen, johnathantanmy
In-Reply-To: <20230627195251.1973421-2-calvinwan@google.com>
Calvin Wan wrote:
> As a library boundary, wrapper.c should not directly log trace2
> statistics, but instead provide those statistics upon
> request. Therefore, move the trace2 logging code to trace2.[ch.]. This
> also allows wrapper.c to not be dependent on trace2.h and repository.h.
>
...
> diff --git a/trace2.h b/trace2.h
> index 4ced30c0db..689e9a4027 100644
> --- a/trace2.h
> +++ b/trace2.h
> @@ -581,4 +581,9 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason);
>
> const char *trace2_session_id(void);
>
> +/*
> + * Writes out trace statistics for fsync
> + */
> +void trace_git_fsync_stats(void);
> +
This function does not belong in 'trace2.h', IMO. The purpose of that file
is to contain the generic API for Trace2 (e.g., 'trace2_printf()',
'trace2_region_(enter|exit)'), whereas this function is effectively a
wrapper around a specific invocation of that API.
You note in the commit message that "wrapper.c should not directly log
trace2 statistics" with the reasoning of "[it's] a library boundary," but I
suspect the unstated underlying reason is "because it tracks 'count_fsync_*'
in static variables." This case would be better handled, then, by replacing
the usage in 'wrapper.c' with a new Trace2 counter (API introduced in [1]).
That keeps this usage consistent with the API already established for
Trace2, rather than starting an unsustainable trend of creating ad-hoc,
per-metric wrappers in 'trace2.[c|h]'.
An added note re: the commit message - it's extremely important that
functions _anywhere in Git_ are able to use the Trace2 API directly. A
developer could reasonably want to measure performance, keep track of an
interesting metric, log when a region is entered in the larger trace,
capture error information, etc. for any function, regardless of where in
falls in the internal library organization. To that end, I think either the
commit message should be rephrased to remove that statement (if the issue is
really "we're using a static variable and we want to avoid that"), or the
libification effort should be updated to accommodate use of Trace2 anywhere
in Git.
[1] https://lore.kernel.org/git/pull.1373.v4.git.1666618868.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch
From: Jens Lideström @ 2023-06-28 7:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Paul Mackerras [ ], git
In-Reply-To: <7c73cc47-302d-8706-dd7f-fd034ef8d945@kdbg.org>
>> +[mc "<%s-C> Create branch on selected commit" $M1T]
>
> ... "C"? Which one is it?
"C" is a mistake. Good catch, thanks!
I choose Ctrl-B to avoid a conflict with Ctrl-C for copying text.
> The key binding to remove a branch does not make sense to me. It does
> happen that I have more than one branch on a commit, but there is no
> way
> to select which one to remove via the keyboard. I have to use the
> context menu. This needs more thought IMHO.
My intention is to always remove the first branch head that is displayed
for a single commit in the GUI. This caters to the common use case, with
only one branch for a single commit. If there are multiple branch heads
on a commit and the users don't want to remove the first one then they
need to use the mouse context menu to choose which one to delete.
I could change the implementation to display a dialog that lets the user
choose in case of multiple branch heads.
In that case, should I do that as part of this PR, or as a follow up? I
would prefer to finish this one first.
> At a minimum, separate it out into its own commit.
I'll do so.
/Jens
On 2023-06-28 07:59, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>>
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>> gitk-git/gitk | 25 ++++++++++++++++---------
>> 1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 596977abe89..0d83a72a424 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -2692,6 +2692,8 @@ proc makewindow {} {
>> bind $ctext <<Selection>> rehighlight_search_results
>> bind . <$M1B-t> {resethead [selected_line_id]}
>> bind . <$M1B-o> {checkout [selected_line_head]
>> [selected_line_id]}
>> + bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id]
>> 1}
>> + bind . <$M1B-b> {mkbranch [selected_line_id]}
>
> "b" vs...
>
>> for {set i 1} {$i < 10} {incr i} {
>> bind . <$M1B-Key-$i> [list go_to_parent $i]
>> }
>> @@ -2735,7 +2737,7 @@ proc makewindow {} {
>> makemenu $headctxmenu {
>> {mc "Check out this branch" command {checkout $headmenuhead
>> $headmenuid}}
>> {mc "Rename this branch" command mvbranch}
>> - {mc "Remove this branch" command rmbranch}
>> + {mc "Remove this branch" command {rmbranch $headmenuhead
>> $headmenuid 0}}
>> {mc "Copy branch name" command {clipboard clear; clipboard
>> append $headmenuhead}}
>> }
>> $headctxmenu configure -tearoff 0
>> @@ -3185,6 +3187,8 @@ proc keys {} {
>> [mc "<F5> Update"]
>> [mc "<%s-T> Reset current branch to selected commit" $M1T]
>> [mc "<%s-O> Check out selected commit" $M1T]
>> +[mc "<%s-C> Create branch on selected commit" $M1T]
>
> ... "C"? Which one is it?
>
>> +[mc "<%s-M> Remove selected branch" $M1T]
>> " \
>> -justify left -bg $bgcolor -border 2 -relief groove
>> pack $w.m -side top -fill both -padx 2 -pady 2
>> @@ -9576,13 +9580,13 @@ proc wrcomcan {} {
>> unset wrcomtop
>> }
>>
>> -proc mkbranch {} {
>> - global NS rowmenuid
>> +proc mkbranch {id} {
>> + global NS
>>
>> set top .branchdialog
>>
>> set val(name) ""
>> - set val(id) $rowmenuid
>> + set val(id) $id
>> set val(command) [list mkbrgo $top]
>>
>> set ui(title) [mc "Create branch"]
>> @@ -10054,13 +10058,14 @@ proc readcheckoutstat {fd newhead newheadref
>> newheadid} {
>> }
>> }
>>
>> -proc rmbranch {} {
>> - global headmenuid headmenuhead mainhead
>> +proc rmbranch {head id shouldComfirm} {
>> + global mainhead
>> global idheads
>> -
>> - set head $headmenuhead
>> - set id $headmenuid
>> # this check shouldn't be needed any more...
>> + if {$head eq ""} {
>> + error_popup [mc "Cannot delete a detached head"]
>> + return
>> + }
>> if {$head eq $mainhead} {
>> error_popup [mc "Cannot delete the currently checked-out
>> branch"]
>> return
>> @@ -10070,6 +10075,8 @@ proc rmbranch {} {
>> # the stuff on this branch isn't on any other branch
>> if {![confirm_popup [mc "The commits on branch %s aren't on
>> any other\
>> branch.\nReally delete branch %s?" $head
>> $head]]} return
>> + } elseif {$shouldComfirm} {
>> + if {![confirm_popup [mc "Really delete branch %s?" $head]]}
>> return
>> }
>> nowbusy rmbranch
>> update
>
> The key binding to remove a branch does not make sense to me. It does
> happen that I have more than one branch on a commit, but there is no
> way
> to select which one to remove via the keyboard. I have to use the
> context menu. This needs more thought IMHO. At a minimum, separate it
> out into its own commit.
>
> -- Hannes
^ permalink raw reply
* Re: [PATCH 9/9] gitk: default select reset hard in dialog
From: Jens Lideström @ 2023-06-28 7:16 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Paul Mackerras [ ], git
In-Reply-To: <e74cc1b3-8459-101f-4613-17df0f5d69e3@kdbg.org>
> I would prefer to keep the default at "mixed" mode, set the focus on
> the
> radio button to make it easy to switch to "hard" mode by hitting the
> Down arrow key, and then make it so that Enter triggers the OK button.
That indeed sounds better! Safer but still convenient. I'll change the
solution to this.
I noticed that some dialogues have a key bind to close with success
using Enter.
/Jens
On 2023-06-28 07:46, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>>
>> Reset hard is dangerous but also the most common reset type, and not
>> having it pre-selected in the dialog is annoying to users.
>
> I agree that the operation of the Reset dialog is clumsy before this
> series. However, this patch together with the previous patch turns it
> into a foot gun. It becomes far too easy to destroy uncommitted work.
>
> I would prefer to keep the default at "mixed" mode, set the focus on
> the
> radio button to make it easy to switch to "hard" mode by hitting the
> Down arrow key, and then make it so that Enter triggers the OK button.
>
>> It is also less dangerous in the GUI where there is a confirmation
>> dialog. Also, dangling commits remain in the GUI and can be recovered.
>
> The problem with "hard" mode are not the commits. The real danger is
> that it blows away uncommitted changes. Besides of that, I do not
> consider this UI a confirmation dialog.
>
> -- Hannes
>
>>
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>> gitk-git/gitk | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 9d93053e360..5b0a0ea46be 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
>> [mc "Reset branch %s to %s?" $mainhead [commit_name
>> $reset_target_id 1]]
>> pack $w.m -side top -fill x -padx 20 -pady 20
>> ${NS}::labelframe $w.f -text [mc "Reset type:"]
>> - set resettype mixed
>> + # Reset hard is dangerous but also the most common reset type,
>> and not
>> + # having it pre-selected in the dialog is annoying to users.
>> + set resettype hard
>> ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
>> -text [mc "Soft: Leave working tree and index untouched"]
>> grid $w.f.soft -sticky w
^ permalink raw reply
* Re: [PATCH 9/9] gitk: default select reset hard in dialog
From: Johannes Sixt @ 2023-06-28 5:46 UTC (permalink / raw)
To: Jens Lidestrom via GitGitGadget; +Cc: Paul Mackerras [ ], Jens Lidestrom, git
In-Reply-To: <97857c3509fb4b45e1bc2d29588374a2631a7c2d.1687876885.git.gitgitgadget@gmail.com>
Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
>
> Reset hard is dangerous but also the most common reset type, and not
> having it pre-selected in the dialog is annoying to users.
I agree that the operation of the Reset dialog is clumsy before this
series. However, this patch together with the previous patch turns it
into a foot gun. It becomes far too easy to destroy uncommitted work.
I would prefer to keep the default at "mixed" mode, set the focus on the
radio button to make it easy to switch to "hard" mode by hitting the
Down arrow key, and then make it so that Enter triggers the OK button.
> It is also less dangerous in the GUI where there is a confirmation
> dialog. Also, dangling commits remain in the GUI and can be recovered.
The problem with "hard" mode are not the commits. The real danger is
that it blows away uncommitted changes. Besides of that, I do not
consider this UI a confirmation dialog.
-- Hannes
>
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
> gitk-git/gitk | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 9d93053e360..5b0a0ea46be 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
> [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
> pack $w.m -side top -fill x -padx 20 -pady 20
> ${NS}::labelframe $w.f -text [mc "Reset type:"]
> - set resettype mixed
> + # Reset hard is dangerous but also the most common reset type, and not
> + # having it pre-selected in the dialog is annoying to users.
> + set resettype hard
> ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
> -text [mc "Soft: Leave working tree and index untouched"]
> grid $w.f.soft -sticky w
^ permalink raw reply
* Re: [PATCH 0/9] gitk: improve keyboard support
From: Jens Lideström @ 2023-06-28 7:01 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Paul Mackerras [ ], git
In-Reply-To: <0cb94aa5-726f-a57f-858c-b29764c63ce7@kdbg.org>
Thanks for your comments, Hannes!
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate
> patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult
> it
> would be for Paul to integrate patches that were generated by
> gitgitgadget).
I'll try to figure out how to do that.
I looked briefly at Pauls repo but got the impression that it was out of
date. I'll have a second look.
/Jens
On 2023-06-28 08:09, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> It is often convenient to use the keyboard to navigate the gitk GUI
>> and
>> there are keyboard shortcut bindings for many operations such as
>> searching
>> and scrolling. There is however no keyboard binding for the most
>> common
>> operations on branches and commits: Check out, reset, cherry-pick,
>> create
>> and delete branches.
>>
>> This PR adds keyboard bindings for these 5 commands. It also adjusts
>> some
>> GUI focus defaults to simplify keyboard navigation.
>>
>> Some refactoring of the command implementation has been necessary.
>> Originally the commands was using the mouse context menu to get info
>> about
>> the head and commit to act on. When using keyboard binds this
>> information
>> isn't available so instead the row that is selected in the GUI is
>> used. By
>> adding procedures for doing this the PR lays the groundwork for more
>> similar
>> keyboard binds in the future.
>
> I like it when an application can be navigated with the keyboard. These
> changes are very much appreciated.
>
> I've left some comments on individual commits. The important one is
> that
> I think it makes the Reset dialog way too easy to destroy uncommitted
> work.
>
> Please note that gitk-git directory is in its own repository that is
> only subtree-merged into the Git repository. You should generate
> patches
> against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult
> it
> would be for Paul to integrate patches that were generated by
> gitgitgadget).
>
> -- Hannes
>
>>
>> I'm including Paul Mackerras because he seems to be the maintainer of
>> gitk.
>> Can you review, Paul?
>>
>> Jens Lidestrom (9):
>> gitk: add procedures to get commit info from selected row
>> gitk: use term "current branch" in gui
>> gitk: add keyboard bind for reset
>> gitk: show branch name in reset dialog
>> gitk: add keyboard bind for checkout
>> gitk: add keyboard bind for create and remove branch
>> gitk: add keyboard bind to cherry-pick
>> gitk: focus ok button in reset dialog
>> gitk: default select reset hard in dialog
>>
>> gitk-git/gitk | 132
>> ++++++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 96 insertions(+), 36 deletions(-)
>>
>>
>> base-commit: 94486b6763c29144c60932829a65fec0597e17b3
>> Published-As:
>> https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
>> pr-1551/jensli/keyboard-for-gitk-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1551
^ permalink raw reply
* Re: [PATCH 0/9] gitk: improve keyboard support
From: Johannes Sixt @ 2023-06-28 6:09 UTC (permalink / raw)
To: Jens Lidestrom via GitGitGadget; +Cc: Paul Mackerras [ ], Jens Lidestrom, git
In-Reply-To: <pull.1551.git.1687876884.gitgitgadget@gmail.com>
Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
> It is often convenient to use the keyboard to navigate the gitk GUI and
> there are keyboard shortcut bindings for many operations such as searching
> and scrolling. There is however no keyboard binding for the most common
> operations on branches and commits: Check out, reset, cherry-pick, create
> and delete branches.
>
> This PR adds keyboard bindings for these 5 commands. It also adjusts some
> GUI focus defaults to simplify keyboard navigation.
>
> Some refactoring of the command implementation has been necessary.
> Originally the commands was using the mouse context menu to get info about
> the head and commit to act on. When using keyboard binds this information
> isn't available so instead the row that is selected in the GUI is used. By
> adding procedures for doing this the PR lays the groundwork for more similar
> keyboard binds in the future.
I like it when an application can be navigated with the keyboard. These
changes are very much appreciated.
I've left some comments on individual commits. The important one is that
I think it makes the Reset dialog way too easy to destroy uncommitted work.
Please note that gitk-git directory is in its own repository that is
only subtree-merged into the Git repository. You should generate patches
against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it
would be for Paul to integrate patches that were generated by gitgitgadget).
-- Hannes
>
> I'm including Paul Mackerras because he seems to be the maintainer of gitk.
> Can you review, Paul?
>
> Jens Lidestrom (9):
> gitk: add procedures to get commit info from selected row
> gitk: use term "current branch" in gui
> gitk: add keyboard bind for reset
> gitk: show branch name in reset dialog
> gitk: add keyboard bind for checkout
> gitk: add keyboard bind for create and remove branch
> gitk: add keyboard bind to cherry-pick
> gitk: focus ok button in reset dialog
> gitk: default select reset hard in dialog
>
> gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 96 insertions(+), 36 deletions(-)
>
>
> base-commit: 94486b6763c29144c60932829a65fec0597e17b3
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1551
^ permalink raw reply
* Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch
From: Johannes Sixt @ 2023-06-28 5:59 UTC (permalink / raw)
To: Jens Lidestrom via GitGitGadget; +Cc: Paul Mackerras [ ], Jens Lidestrom, git
In-Reply-To: <661f098d882e64391ff76647e3764d58c6cbb50a.1687876885.git.gitgitgadget@gmail.com>
Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
> From: Jens Lidestrom <jens@lidestrom.se>
>
> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
> ---
> gitk-git/gitk | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 596977abe89..0d83a72a424 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -2692,6 +2692,8 @@ proc makewindow {} {
> bind $ctext <<Selection>> rehighlight_search_results
> bind . <$M1B-t> {resethead [selected_line_id]}
> bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
> + bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 1}
> + bind . <$M1B-b> {mkbranch [selected_line_id]}
"b" vs...
> for {set i 1} {$i < 10} {incr i} {
> bind . <$M1B-Key-$i> [list go_to_parent $i]
> }
> @@ -2735,7 +2737,7 @@ proc makewindow {} {
> makemenu $headctxmenu {
> {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
> {mc "Rename this branch" command mvbranch}
> - {mc "Remove this branch" command rmbranch}
> + {mc "Remove this branch" command {rmbranch $headmenuhead $headmenuid 0}}
> {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
> }
> $headctxmenu configure -tearoff 0
> @@ -3185,6 +3187,8 @@ proc keys {} {
> [mc "<F5> Update"]
> [mc "<%s-T> Reset current branch to selected commit" $M1T]
> [mc "<%s-O> Check out selected commit" $M1T]
> +[mc "<%s-C> Create branch on selected commit" $M1T]
... "C"? Which one is it?
> +[mc "<%s-M> Remove selected branch" $M1T]
> " \
> -justify left -bg $bgcolor -border 2 -relief groove
> pack $w.m -side top -fill both -padx 2 -pady 2
> @@ -9576,13 +9580,13 @@ proc wrcomcan {} {
> unset wrcomtop
> }
>
> -proc mkbranch {} {
> - global NS rowmenuid
> +proc mkbranch {id} {
> + global NS
>
> set top .branchdialog
>
> set val(name) ""
> - set val(id) $rowmenuid
> + set val(id) $id
> set val(command) [list mkbrgo $top]
>
> set ui(title) [mc "Create branch"]
> @@ -10054,13 +10058,14 @@ proc readcheckoutstat {fd newhead newheadref newheadid} {
> }
> }
>
> -proc rmbranch {} {
> - global headmenuid headmenuhead mainhead
> +proc rmbranch {head id shouldComfirm} {
> + global mainhead
> global idheads
> -
> - set head $headmenuhead
> - set id $headmenuid
> # this check shouldn't be needed any more...
> + if {$head eq ""} {
> + error_popup [mc "Cannot delete a detached head"]
> + return
> + }
> if {$head eq $mainhead} {
> error_popup [mc "Cannot delete the currently checked-out branch"]
> return
> @@ -10070,6 +10075,8 @@ proc rmbranch {} {
> # the stuff on this branch isn't on any other branch
> if {![confirm_popup [mc "The commits on branch %s aren't on any other\
> branch.\nReally delete branch %s?" $head $head]]} return
> + } elseif {$shouldComfirm} {
> + if {![confirm_popup [mc "Really delete branch %s?" $head]]} return
> }
> nowbusy rmbranch
> update
The key binding to remove a branch does not make sense to me. It does
happen that I have more than one branch on a commit, but there is no way
to select which one to remove via the keyboard. I have to use the
context menu. This needs more thought IMHO. At a minimum, separate it
out into its own commit.
-- Hannes
^ permalink raw reply
* What's cooking in git.git (Jun 2023, #07; Tue, 27)
From: Teng Long @ 2023-06-28 9:54 UTC (permalink / raw)
To: gitster; +Cc: git
In-Reply-To: <xmqqcz1gftdn.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> * tl/notes-separator (2023-06-21) 7 commits
> - notes: introduce "--no-separator" option
> - notes.c: introduce "--[no-]stripspace" option
> - notes.c: append separator instead of insert by pos
> - notes.c: introduce '--separator=<paragraph-break>' option
> - t3321: add test cases about the notes stripspace behavior
> - notes.c: use designated initializers for clarity
> - notes.c: cleanup 'strbuf_grow' call in 'append_edit'
>
> 'git notes append' was taught '--separator' to specify string to insert
> between paragraphs.
> source: <cover.1685174011.git.dyroneteng@gmail.com>
There are no pending issues I think ;-)
Please let me know if there are some obstables to merge.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] diff --no-index: support reading from named pipes
From: Phillip Wood @ 2023-06-28 10:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Taylor Blau, brian m. carlson,
Thomas Guyot-Sionnest
In-Reply-To: <xmqqy1k4g068.fsf@gitster.g>
Hi Junio
On 27/06/2023 20:44, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> In some shells, such as bash and zsh, it's possible to use a command
>> substitution to provide the output of a command as a file argument to
>> another process, like so:
>>
>> diff -u <(printf "a\nb\n") <(printf "a\nc\n")
>>
>> However, this syntax does not produce useful results with "git diff
>> --no-index". On macOS, the arguments to the command are named pipes
>> under /dev/fd, and git diff doesn't know how to handle a named pipe. On
>> Linux, the arguments are symlinks to pipes, so git diff "helpfully"
>> diffs these symlinks, comparing their targets like "pipe:[1234]" and
>> "pipe:[5678]".
>>
>> To address this "diff --no-index" is changed so that if a path given on
>> the commandline is a named pipe or a symbolic link that resolves to a
>> named pipe then we read the data to diff from that pipe. This is
>> implemented by generalizing the code that already exists to handle
>> reading from stdin when the user passes the path "-".
>>
>> As process substitution is not support by POSIX this change is tested by
>> using a pipe and a symbolic link to a pipe.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> diff-no-index.c | 80 ++++++++++++++++++++++++----------------
>> t/t4053-diff-no-index.sh | 25 +++++++++++++
>> 2 files changed, 73 insertions(+), 32 deletions(-)
>
> This looks good, if a bit invasive, to a cursory read, at least to
> me. It is very focused to the real problem at hand, and shows that
> the way we split the "no-index" mode out to its own implementation
> of filespec population code does make sense.
Yes, it is more invasive than I'd like but I think that stems from
needing to treat paths given on the commandline differently to the paths
we find when recursing directories.
>> -static void populate_from_stdin(struct diff_filespec *s)
>> +static void populate_from_pipe(struct diff_filespec *s, int is_stdin)
>> {
>> struct strbuf buf = STRBUF_INIT;
>> size_t size = 0;
>> + int fd = 0;
>>
>> - if (strbuf_read(&buf, 0, 0) < 0)
>> + if (!is_stdin)
>> + fd = xopen(s->path, O_RDONLY);
>> + if (strbuf_read(&buf, fd, 0) < 0)
>> die_errno("error while reading from stdin");
>> + if (!is_stdin)
>> + close(fd);
>
> Given that the error message explicitly says "stdin", and there are
> many "if ([!]is_stdin)" sprinkled in the code, I actually suspect
> that there should be two separate helpers, one for stdin and one for
> non-stdin pipe. It is especially true since there is only one
> caller that does this:
>
>> + if (is_pipe)
>> + populate_from_pipe(s, name == file_from_standard_input);
>
> which can be
>
> if (is_pipe) {
> if (name == file_from_standard_input)
> populate_from_stdin(s);
> else
> populate_from_pipe(s);
> }
>
> without losing clarity. The code that you are sharing by forcing
> them to be a single helper to wrap up a handful of members in the s
> structure can become its own helper that is called from these two
> helper functions.
Sure, and thanks for pointing out the error message, I'd overlooked that.
>> static int queue_diff(struct diff_options *o,
>> - const char *name1, const char *name2)
>> + const char *name1, int is_pipe1,
>> + const char *name2, int is_pipe2)
>> {
>> int mode1 = 0, mode2 = 0;
>>
>> - if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
>> + if (get_mode(name1, is_pipe1, &mode1) ||
>> + get_mode(name2, is_pipe2, &mode2))
>> return -1;
>
> Makes me wonder why the caller of queue_diff() even needs to know if
> these two names are pipes; we are calling get_mode() which would run
> stat(2) anyway, and the result from stat(2) is what you use (in the
> caller) to determine the values of is_pipeN. Wouldn't it be more
> appropriate to leave the caller oblivious of special casing of the
> pipes and let get_mode() handle this? After all, that is how the
> existing code special cases the standard input so there is a strong
> precedence.
Maybe is_pipeN should be called force_pipeN. We only want to
de-reference symbolic links to pipes for paths that are given on the
command line, not when the the user asked to compare two directories.
That means we need to pass some kind of flag around to say whether we're
recursing or not. An earlier draft of this series had a recursing flag
rather than is_pipeN like this
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int recursing)
{
struct stat st;
if (!path || !strcmp(path, "/dev/null"))
*mode = 0;
#ifdef GIT_WINDOWS_NATIVE
else if (!strcasecmp(path, "nul"))
*mode = 0;
#endif
else if (path == file_from_standard_input)
*mode = create_ce_mode(0666);
else if (lstat(path, &st))
return error("Could not access '%s'", path);
else
*mode = st.st_mode;
+
+ /*
+ * For paths given on the command line de-reference symbolic
+ * links that resolve to a fifo.
+ */
+ if (!recursing &&
+ S_ISLNK(*mode) && !stat(path, &st) && S_ISFIFO(st.st_mode))
+ *mode = st.st_mode;
+
return 0;
}
I dropped it in favor of is_pipeN after I realized we need to check if
the paths on the command line are pipes before calling fixup_paths(). I
think we could use the "special" parameter you suggest below as a
recursion indicator by setting it to NULL when recursing.
> If we go that route, it may make sense to further isolate the
> "address comparison" trick used for the standard input mode.
> Perhaps we can and do something like
>
> static int get_mode(const char *path, int *mode, int *special)
> {
> struct stat st;
>
> + *special = 0; /* default - nothing special */
> ...
> else if (path == file_from_standard_input) {
> *mode = create_ce_mode(0666);
> + *pipe_kind = 1; /* STDIN */
> + } else if (stat(path, &st)) {
> + ... error ...
> + } else if (S_ISFIFO(st.st_mode)) {
> + *mode = create_ce_mode(0666);
> + *pipe_kind = 2; /* FIFO */
> } else if (lstat(path, &st)) {
> ... error ...
> } else {
> *mode = st.st_mode;
> }
>
> and have the caller act on "special" to choose among calling
> populate_from_stdin(), populate_from_pipe(), or do nothing for
> the regular files?
>
> Side note: this has an added benefit of highlighting that we do
> stat() and lstat() because of dereferencing. What I suspect is
> that "git diff --no-index" mode was primarily to give Git
> niceties like rename detection and diff algorithms to those who
> wanted to use in contexts (i.e. contents not tracked by Git)
> they use "diff" by other people like GNU, without bothering to
> update "diff" by other people. I further suspect that "compare
> the readlink contents", which is very much necessary within the
> Git context, may not fall into the "Git niceties" when they
> invoke "--no-index" mode. Which leads me to imagine a future
> direction where we only use stat() and not lstat() in the
> "--no-index" codepath. Having everything including these
> lstat() and stat() calls inside get_mode() will allow such a
> future transition hopefully simpler.
>
> I do not quite see why you decided to move the "is_dir" processing
> up and made the caller responsible.
To avoid a second stat() call in is_directory() after we've already
called stat() to see if the path is a pipe.
> Specifically,
>
>> - fixup_paths(paths, &replacement);
>> + if (!is_pipe[0] && !is_pipe[1])
>> + fixup_paths(paths, is_dir, &replacement);
>
> this seems fishy when one side is pipe and the other one is not.
> When the user says
>
> $ git diff --no-index <(command) path
>
> fixup_paths() are bypassed because one of them is pipe. It makes me
> suspect that it should be an error if "path" is a directory. I do
> not know if fixup_paths() is the best place for doing such checking,
> but somebody should be doing that, no?
I wasn't sure what was best to do in that case. In the end I went with
making the behavior the same as "git diff --no-index -- - directory".
I'm happy to make both an error.
Thanks for your comments, I'll leave it a couple of days to see if there
are any more comments and then re-roll.
Phillip
^ permalink raw reply
* Re: [PATCH v2] fix cherry-pick/revert status when doing multiple commits
From: Phillip Wood @ 2023-06-28 11:11 UTC (permalink / raw)
To: Jacob Keller, git, Phillip Wood; +Cc: Jacob Keller
In-Reply-To: <20230627224230.1951135-1-jacob.e.keller@intel.com>
Hi Jacob
This version looks good to me
Thanks for re-rolling
Phillip
On 27/06/2023 23:41, Jacob Keller wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> The status report for an in-progress cherry-pick does not show the
> current commit if the cherry-pick happens as part of a series of
> multiple commits:
>
> $ git cherry-pick <commit1> <commit2>
> < one of the cherry-picks fails to merge clean >
> Cherry-pick currently in progress.
> (run "git cherry-pick --continue" to continue)
> (use "git cherry-pick --skip" to skip this patch)
> (use "git cherry-pick --abort" to cancel the cherry-pick operation)
>
> $ git status
> On branch <branch>
> Your branch is ahead of '<upstream>' by 1 commit.
> (use "git push" to publish your local commits)
>
> Cherry-pick currently in progress.
> (run "git cherry-pick --continue" to continue)
> (use "git cherry-pick --skip" to skip this patch)
> (use "git cherry-pick --abort" to cancel the cherry-pick operation)
>
> The show_cherry_pick_in_progress() function prints "Cherry-pick
> currently in progress". That function does have a more verbose print
> based on whether the cherry_pick_head_oid is null or not. If it is not
> null, then a more helpful message including which commit is actually
> being picked is displayed.
>
> The introduction of the "Cherry-pick currently in progress" message
> comes from 4a72486de97b ("fix cherry-pick/revert status after commit",
> 2019-04-17). This commit modified wt_status_get_state() in order to
> detect that a cherry-pick was in progress even if the user has used `git
> commit` in the middle of the sequence.
>
> The check used to detect this is the call to sequencer_get_last_command.
> If the sequencer indicates that the lass command was a REPLAY_PICK, then
> the state->cherry_pick_in_progress is set to 1 and the
> cherry_pick_head_oid is initialized to the null_oid. Similar behavior is
> done for the case of REPLAY_REVERT.
>
> It happens that this call of sequencer_get_last_command will always
> report the action even if the user hasn't interrupted anything. Thus,
> during a range of cherry-picks or reverts, the cherry_pick_head_oid and
> revert_head_oid will always be overwritten and initialized to the null
> oid.
>
> This results in status always displaying the terse message which does
> not include commit information.
>
> Fix this by adding an additional check so that we do not re-initialize
> the cherry_pick_head_oid or revert_head_oid if we have already set the
> cherry_pick_in_progress or revert_in_progress bits. This ensures that
> git status will display the more helpful information when its available.
> Add a test case covering this behavior.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>
> Changes since v1:
> * add the missing test case that I had locally but forgot to squash in
> * use else if as suggested by Phillip
>
> t/t7512-status-help.sh | 22 ++++++++++++++++++++++
> wt-status.c | 4 ++--
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index 2f16d5787edf..c2ab8a444a83 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -774,6 +774,28 @@ EOF
> test_cmp expected actual
> '
>
> +test_expect_success 'status when cherry-picking multiple commits' '
> + git reset --hard cherry_branch &&
> + test_when_finished "git cherry-pick --abort" &&
> + test_must_fail git cherry-pick cherry_branch_second one_cherry &&
> + TO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&
> + cat >expected <<EOF &&
> +On branch cherry_branch
> +You are currently cherry-picking commit $TO_CHERRY_PICK.
> + (fix conflicts and run "git cherry-pick --continue")
> + (use "git cherry-pick --skip" to skip this patch)
> + (use "git cherry-pick --abort" to cancel the cherry-pick operation)
> +
> +Unmerged paths:
> + (use "git add <file>..." to mark resolution)
> + both modified: main.txt
> +
> +no changes added to commit (use "git add" and/or "git commit -a")
> +EOF
> + git status --untracked-files=no >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'status when cherry-picking after committing conflict resolution' '
> git reset --hard cherry_branch &&
> test_when_finished "git cherry-pick --abort" &&
> diff --git a/wt-status.c b/wt-status.c
> index 068b76ef6d96..8d23ff8ced23 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1790,10 +1790,10 @@ void wt_status_get_state(struct repository *r,
> oidcpy(&state->revert_head_oid, &oid);
> }
> if (!sequencer_get_last_command(r, &action)) {
> - if (action == REPLAY_PICK) {
> + if (action == REPLAY_PICK && !state->cherry_pick_in_progress) {
> state->cherry_pick_in_progress = 1;
> oidcpy(&state->cherry_pick_head_oid, null_oid());
> - } else {
> + } else if (action == REPLAY_REVERT && !state->revert_in_progress) {
> state->revert_in_progress = 1;
> oidcpy(&state->revert_head_oid, null_oid());
> }
^ permalink raw reply
* git-switch history and checkout compatibility
From: Namikaze Minato @ 2023-06-28 13:03 UTC (permalink / raw)
To: git
In-Reply-To: <CACmJb3yoHagaU1wb4qRT-nZV4Wptao8boaUXCAYrFxfrxcmUYg@mail.gmail.com>
Hello,
I have been waiting for people to report this but it never came so I'm doing it:
I have trouble with getting used to git-switch instead of
git-checkout, but have even more trouble to get people to adopt it.
Please consider the two following git-switch statements:
git switch remote/branch # fatal: a branch is expected, got remote
branch 'remote/branch'
#and
git switch -d remote/branch
git switch master
git switch - # fatal: a branch is expected, got commit 'commit_id_here'
Both as retro-compatibility with checkout and for user-friendliness, I
would expect both to work.
Maybe a setting checkout.autoDetach could control such behavior if the
current implementation should be kept?
What do you think?
Kind regards,
Minato
^ 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