* Re: [PATCH 02/20] t: add library for munging chunk-format files
From: Taylor Blau @ 2023-10-10 23:47 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009205838.GB3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 04:58:38PM -0400, Jeff King wrote:
> ---
> t/lib-chunk.sh | 17 ++++++++
> t/lib-chunk/corrupt-chunk-file.pl | 66 +++++++++++++++++++++++++++++++
> 2 files changed, 83 insertions(+)
> create mode 100644 t/lib-chunk.sh
> create mode 100644 t/lib-chunk/corrupt-chunk-file.pl
I can't claim to be a competent reviewer for the Perl portions of this
patch. But I agree with the goal and am glad to see us getting away from
the brittle implementation that we have been living with in the test
suite.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 03/20] midx: stop ignoring malformed oid fanout chunk
From: Taylor Blau @ 2023-10-10 23:50 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009205919.GC3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 04:59:19PM -0400, Jeff King wrote:
> When we load the oid-fanout chunk, our callback checks that its size is
> reasonable and returns an error if not. However, the caller only checks
> our return value against CHUNK_NOT_FOUND, so we end up ignoring the
> error completely! Using a too-small fanout table means we end up
> accessing random memory for the fanout and segfault.
>
> We can fix this by checking for any non-zero return value, rather than
> just CHUNK_NOT_FOUND, and adjusting our error message to cover both
> cases. We could handle each error code individually, but there's not
> much point for such a rare case. The extra message produced in the
> callback makes it clear what is going on.
>
> The same pattern is used in the adjacent code. Those cases are actually
> OK for now because they do not use a custom callback, so the only error
> they can get is CHUNK_NOT_FOUND. But let's convert them, as this is an
> accident waiting to happen (especially as we convert some of them away
> from pair_chunk). The error messages are more verbose, but it should be
> rare for a user to see these anyway.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> midx.c | 16 ++++++++--------
> t/t5319-multi-pack-index.sh | 20 +++++++++++++++++++-
> 2 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/midx.c b/midx.c
> index 3165218ab5..21d7dd15ef 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -143,14 +143,14 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
> MIDX_HEADER_SIZE, m->num_chunks))
> goto cleanup_fail;
>
> - if (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)
> - die(_("multi-pack-index missing required pack-name chunk"));
> - if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m) == CHUNK_NOT_FOUND)
> - die(_("multi-pack-index missing required OID fanout chunk"));
> - if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)
> - die(_("multi-pack-index missing required OID lookup chunk"));
> - if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)
> - die(_("multi-pack-index missing required object offsets chunk"));
> + if (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names))
> + die(_("multi-pack-index required pack-name chunk missing or corrupted"));
> + if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))
> + die(_("multi-pack-index required OID fanout chunk missing or corrupted"));
> + if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup))
> + die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
> + if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets))
> + die(_("multi-pack-index required object offsets chunk missing or corrupted"));
All makes sense. I have a mild preference for "missing or corrupt" over
"missing or corrupted", but it's mild enough that I wouldn't be sad if
you kept it as-is ;-).
I do wonder if translators would be happy with:
die(_("multi-pack-index required %s chunk missing or corrupt"),
"OID fanout");
or if that is assuming too much about the languages that we translate
into.
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 1bcc02004d..b8fe85aeba 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
The rest of the patch is looking good...
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 04/20] commit-graph: check size of oid fanout chunk
From: Taylor Blau @ 2023-10-11 0:08 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009205951.GD3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 04:59:51PM -0400, Jeff King wrote:
> We load the oid fanout chunk with pair_chunk(), which means we never see
> the size of the chunk. We just assume the on-disk file uses the
> appropriate size, and if it's too small we'll access random memory.
>
> It's easy to check this up-front; the fanout always consists of 256
> uint32's, since it is a fanout of the first byte of the hash pointing
> into the oid index. These parameters can't be changed without
> introducing a new chunk type.
Cool, this is the first patch that should start reducing our usage of
the new pair_chunk_unsafe() and hardening these reads. Let's take a
look...
> This matches the similar check in the midx OIDF chunk (but note that
> rather than checking for the error immediately, the graph code just
> leaves parts of the struct NULL and checks for required fields later).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> commit-graph.c | 13 +++++++++++--
> t/t5318-commit-graph.sh | 26 ++++++++++++++++++++++++++
> 2 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index a689a55b79..9b3b01da61 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -305,6 +305,16 @@ static int verify_commit_graph_lite(struct commit_graph *g)
> return 0;
> }
>
> +static int graph_read_oid_fanout(const unsigned char *chunk_start,
> + size_t chunk_size, void *data)
> +{
> + struct commit_graph *g = data;
> + if (chunk_size != 256 * sizeof(uint32_t))
> + return error("commit-graph oid fanout chunk is wrong size");
Should we mark this string for translation?
> + g->chunk_oid_fanout = (const uint32_t *)chunk_start;
> + return 0;
> +}
> +
Nice. This makes sense and seems like an obvious improvement over the
existing code.
I wonder how common this pattern is. We have read_chunk() which is for
handling more complex scenarios than this. But the safe version of
pair_chunk() really just wants to check that the size of the chunk is as
expected and assign the location in the mmap to some pointer.
Do you think it would be worth changing pair_chunk() to take an expected
size_t and handle this generically? I.e. have a version of
chunk-format::pair_chunk_fn() that looks something like:
static int pair_chunk_fn(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
const unsigned char **p = data;
if (chunk_size != data->size)
return -1;
*p = chunk_start;
return 0;
}
and then our call here would be:
if (pair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,
(const unsigned char **)&graph->chunk_oid_fanout,
256 * sizeof(uint32_t)) < 0)
return error("commit-graph oid fanout chunk is wrong size");
I dunno. It's hard to have a more concrete recomendation without having
read the rest of the series. So it's possible that this is just complete
nonsense ;-). But my hunch is that there are a number of callers that
would benefit from having this built in.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 04/20] commit-graph: check size of oid fanout chunk
From: Taylor Blau @ 2023-10-11 1:24 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <ZSXnZXglgbfK3VYd@nand.local>
On Tue, Oct 10, 2023 at 08:08:05PM -0400, Taylor Blau wrote:
> Do you think it would be worth changing pair_chunk() to take an expected
> size_t and handle this generically? I.e. have a version of
> chunk-format::pair_chunk_fn() that looks something like:
>
> static int pair_chunk_fn(const unsigned char *chunk_start,
> size_t chunk_size, void *data)
> {
> const unsigned char **p = data;
> if (chunk_size != data->size)
> return -1;
> *p = chunk_start;
> return 0;
> }
>
> and then our call here would be:
>
> if (pair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,
> (const unsigned char **)&graph->chunk_oid_fanout,
> 256 * sizeof(uint32_t)) < 0)
> return error("commit-graph oid fanout chunk is wrong size");
I took a brief stab at implementing this tonight and came up with this,
which applies on top of this patch. Looking through the rest of the
series briefly[^1], I think having something like this would be useful:
--- 8< ---
diff --git a/chunk-format.c b/chunk-format.c
index 8d910e23f6..38b0f847be 100644
--- a/chunk-format.c
+++ b/chunk-format.c
@@ -157,6 +157,8 @@ int read_table_of_contents(struct chunkfile *cf,
struct pair_chunk_data {
const unsigned char **p;
size_t *size;
+
+ size_t expected_size;
};
static int pair_chunk_fn(const unsigned char *chunk_start,
@@ -169,6 +171,20 @@ static int pair_chunk_fn(const unsigned char *chunk_start,
return 0;
}
+static int pair_chunk_expect_fn(const unsigned char *chunk_start,
+ size_t chunk_size,
+ void *data)
+{
+ struct pair_chunk_data *pcd = data;
+ if (pcd->expected_size != chunk_size)
+ return error(_("mismatched chunk size, got: %"PRIuMAX", wanted:"
+ " %"PRIuMAX),
+ (uintmax_t)chunk_size,
+ (uintmax_t)pcd->expected_size);
+ *pcd->p = chunk_start;
+ return 0;
+}
+
int pair_chunk(struct chunkfile *cf,
uint32_t chunk_id,
const unsigned char **p,
@@ -178,6 +194,14 @@ int pair_chunk(struct chunkfile *cf,
return read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);
}
+int pair_chunk_expect(struct chunkfile *cf,
+ uint32_t chunk_id, const unsigned char **p,
+ size_t expected_size)
+{
+ struct pair_chunk_data pcd = { .p = p, .expected_size = expected_size };
+ return read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);
+}
+
int pair_chunk_unsafe(struct chunkfile *cf,
uint32_t chunk_id,
const unsigned char **p)
diff --git a/chunk-format.h b/chunk-format.h
index 8dce7967f4..778f81f555 100644
--- a/chunk-format.h
+++ b/chunk-format.h
@@ -53,6 +53,10 @@ int pair_chunk(struct chunkfile *cf,
const unsigned char **p,
size_t *size);
+int pair_chunk_expect(struct chunkfile *cf,
+ uint32_t chunk_id, const unsigned char **p,
+ size_t expected_size);
+
/*
* Unsafe version of pair_chunk; it does not return the size,
* meaning that the caller cannot possibly be careful about
diff --git a/commit-graph.c b/commit-graph.c
index 9b3b01da61..ed85161fdb 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -305,16 +305,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)
return 0;
}
-static int graph_read_oid_fanout(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size != 256 * sizeof(uint32_t))
- return error("commit-graph oid fanout chunk is wrong size");
- g->chunk_oid_fanout = (const uint32_t *)chunk_start;
- return 0;
-}
-
static int graph_read_oid_lookup(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -404,7 +394,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
GRAPH_HEADER_SIZE, graph->num_chunks))
goto free_and_return;
- read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT,
+ (const unsigned char **)&graph->chunk_oid_fanout,
+ 256 * sizeof(uint32_t)) < 0)
+ error(_("commit-graph oid fanout chunk is wrong size"));
read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
pair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);
pair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d25bea3ec5..467b5f5e9c 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -841,6 +841,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
# otherwise we hit an earlier check
check_corrupt_chunk OIDF clear $(printf "000000%02x" $(test_seq 250)) &&
cat >expect.err <<-\EOF &&
+ error: mismatched chunk size, got: 1000, wanted: 1024
error: commit-graph oid fanout chunk is wrong size
error: commit-graph is missing the OID Fanout chunk
EOF
--- >8 ---
Or, quite honestly, having the "expected_size" parameter be required in
the safe version of `pair_chunk()`.
Thanks,
Taylor
[^1]: A non-brief review is on my to-do list for tomorrow.
^ permalink raw reply related
* What's cooking in git.git (Oct 2023, #04; Tue, 10)
From: Junio C Hamano @ 2023-10-11 1:32 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']
* cc/repack-sift-filtered-objects-to-separate-pack (2023-10-02) 9 commits
(merged to 'next' on 2023-10-03 at e5a4824609)
+ 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
+ pack-bitmap-write: rebuild using new bitmap when remapping
+ repack: refactor finding pack prefix
+ repack: refactor finishing pack-objects command
+ t/helper: add 'find-pack' test-tool
+ pack-objects: allow `--filter` without `--stdout`
"git repack" machinery learns to pay attention to the "--filter="
option.
cf. <ZRsknb4NxNHTR21E@nand.local>
source: <20231002165504.1325153-1-christian.couder@gmail.com>
* cw/prelim-cleanup (2023-09-29) 4 commits
(merged to 'next' on 2023-10-03 at 5985929612)
+ parse: separate out parsing functions from config.h
+ config: correct bad boolean env value error message
+ wrapper: reduce scope of remove_or_warn()
+ hex-ll: separate out non-hash-algo functions
Shuffle some bits across headers and sources to prepare for
libification effort.
source: <cover.1696021277.git.jonathantanmy@google.com>
* ds/init-diffstat-width (2023-09-29) 1 commit
(merged to 'next' on 2023-10-03 at 18383ac895)
+ diff --stat: set the width defaults in a helper function
Code clean-up.
source: <d45d1dac1a20699e370905b88b6fd0ec296751e7.1695441501.git.dsimic@manjaro.org>
* eb/limit-bulk-checkin-to-blobs (2023-09-26) 1 commit
(merged to 'next' on 2023-10-02 at 89c9c95966)
+ bulk-checkin: only support blobs in index_bulk_checkin
The "streaming" interface used for bulk-checkin codepath has been
narrowed to take only blob objects for now, with no real loss of
functionality.
source: <87msx99b9o.fsf_-_@gmail.froward.int.ebiederm.org>
--------------------------------------------------
[New Topics]
* ak/pretty-decorate-more-fix (2023-10-09) 1 commit
- pretty: fix ref filtering for %(decorate) formats
Unlike "git log --pretty=%D", "git log --pretty="%(decorate)" did
not auto-initialize the decoration subsystem, which has been
corrected.
Will merge to 'next'?
source: <20231008202307.1568477-1-andy.koppe@gmail.com>
* en/docfixes (2023-10-09) 25 commits
- documentation: add missing parenthesis
- documentation: add missing quotes
- documentation: add missing fullstops
- documentation: add some commas where they are helpful
- documentation: fix whitespace issues
- documentation: fix capitalization
- documentation: fix punctuation
- documentation: use clearer prepositions
- documentation: add missing hyphens
- documentation: remove unnecessary hyphens
- documentation: add missing article
- documentation: fix choice of article
- documentation: whitespace is already generally plural
- documentation: fix singular vs. plural
- documentation: fix verb vs. noun
- documentation: fix adjective vs. noun
- documentation: fix verb tense
- documentation: employ consistent verb tense for a list
- documentation: fix subject/verb agreement
- documentation: remove extraneous words
- documentation: add missing words
- documentation: fix apostrophe usage
- documentation: fix typos
- documentation: fix small error
- documentation: wording improvements
Documentation typo and grammo fixes.
Needs review.
source: <pull.1595.git.1696747527.gitgitgadget@gmail.com>
* kn/rev-list-missing-fix (2023-10-09) 3 commits
- rev-list: add commit object support in `--missing` option
- rev-list: move `show_commit()` to the bottom
- revision: rename bit to `do_not_die_on_missing_objects`
"git rev-list --missing" did not work for missing commit objects,
which has been corrected.
Needs review.
source: <20231009105528.17777-1-karthik.188@gmail.com>
* sn/cat-file-doc-update (2023-10-09) 1 commit
(merged to 'next' on 2023-10-10 at 6719ba7bd4)
+ doc/cat-file: make synopsis and description less confusing
"git cat-file" documentation updates.
Will merge to 'master'.
source: <20231009175604.2361700-1-stepnem@smrk.net>
* xz/commit-title-soft-limit-doc (2023-10-09) 1 commit
(merged to 'next' on 2023-10-10 at 0bf3d9ed51)
+ doc: correct the 50 characters soft limit (+)
Doc update.
Will merge to 'master'.
source: <pull.1585.v2.git.git.1696778367151.gitgitgadget@gmail.com>
* jk/chunk-bounds (2023-10-09) 20 commits
(merged to 'next' on 2023-10-10 at 21139603ce)
+ chunk-format: drop pair_chunk_unsafe()
+ commit-graph: detect out-of-order BIDX offsets
+ commit-graph: check bounds when accessing BIDX chunk
+ commit-graph: check bounds when accessing BDAT chunk
+ commit-graph: bounds-check generation overflow chunk
+ commit-graph: check size of generations chunk
+ commit-graph: bounds-check base graphs chunk
+ commit-graph: detect out-of-bounds extra-edges pointers
+ commit-graph: check size of commit data chunk
+ midx: check size of revindex chunk
+ midx: bounds-check large offset chunk
+ midx: check size of object offset chunk
+ midx: enforce chunk alignment on reading
+ midx: check size of pack names chunk
+ commit-graph: check consistency of fanout table
+ midx: check size of oid lookup chunk
+ commit-graph: check size of oid fanout chunk
+ midx: stop ignoring malformed oid fanout chunk
+ t: add library for munging chunk-format files
+ chunk-format: note that pair_chunk() is unsafe
The codepaths that read "chunk" formatted files have been corrected
to pay attention to the chunk size and notice broken files.
Will merge to 'master'.
source: <20231009205544.GA3281950@coredump.intra.peff.net>
* ps/rewritten-is-per-worktree-doc (2023-10-10) 1 commit
- doc/git-worktree: mention "refs/rewritten" as per-worktree refs
Doc update.
Will merge to 'next'.
source: <985ac850eb6e60ae76601acc8bfbcd56f99348b4.1696935657.git.ps@pks.im>
--------------------------------------------------
[Stalled]
* tb/path-filter-fix (2023-08-30) 15 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new filter ver. that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- 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.
Reroll exists, not picked up yet.
cf. <20230830200218.GA5147@szeder.dev>
cf. <20230901205616.3572722-1-jonathantanmy@google.com>
cf. <20230924195900.GA1156862@szeder.dev>
cf. <20231008143523.GA18858@szeder.dev>
source: <cover.1693413637.git.jonathantanmy@google.com>
* pw/rebase-sigint (2023-09-07) 1 commit
- rebase -i: ignore signals when forking subprocesses
If the commit log editor or other external programs (spawned via
"exec" insn in the todo list) receive internactive signal during
"git rebase -i", it caused not just the spawned program but the
"Git" process that spawned them, which is often not what the end
user intended. "git" learned to ignore SIGINT and SIGQUIT while
waiting for these subprocesses.
Expecting a reroll.
cf. <12c956ea-330d-4441-937f-7885ab519e26@gmail.com>
source: <pull.1581.git.1694080982621.gitgitgadget@gmail.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>
* jc/diff-cached-fsmonitor-fix (2023-09-15) 3 commits
- diff-lib: fix check_removed() when fsmonitor is active
- Merge branch 'jc/fake-lstat' into jc/diff-cached-fsmonitor-fix
- Merge branch 'js/diff-cached-fsmonitor-fix' into jc/diff-cached-fsmonitor-fix
(this branch uses jc/fake-lstat.)
The optimization based on fsmonitor in the "diff --cached"
codepath is resurrected with the "fake-lstat" introduced earlier.
It is unknown if the optimization is worth resurrecting, but in case...
source: <xmqqr0n0h0tw.fsf@gitster.g>
--------------------------------------------------
[Cooking]
* jc/merge-ort-attr-index-fix (2023-10-09) 1 commit
(merged to 'next' on 2023-10-10 at b139b87502)
+ merge-ort: initialize repo in index state
Fix "git merge-tree" to stop segfaulting when the --attr-source
option is used.
Will merge to 'master'.
source: <pull.1583.v3.git.git.1696857660374.gitgitgadget@gmail.com>
* jk/decoration-and-other-leak-fixes (2023-10-05) 3 commits
(merged to 'next' on 2023-10-06 at 5fc05c94dc)
+ daemon: free listen_addr before returning
+ revision: clear decoration structs during release_revisions()
+ decorate: add clear_decoration() function
Leakfix.
Will merge to 'master'.
source: <20231005212802.GA982892@coredump.intra.peff.net>
* sn/typo-grammo-phraso-fixes (2023-10-05) 5 commits
- t/README: fix multi-prerequisite example
- doc/gitk: s/sticked/stuck/
- git-jump: admit to passing merge mode args to ls-files
- doc/diff-options: improve wording of the log.diffMerges mention
- doc: fix some typos, grammar and wording issues
Many typos, ungrammatical sentences and wrong phrasing have been
fixed.
Needs review.
source: <20231003082107.3002173-1-stepnem@smrk.net>
* so/diff-merges-dd (2023-10-09) 3 commits
- completion: complete '--dd'
- diff-merges: introduce '--dd' option
- diff-merges: improve --diff-merges documentation
"git log" and friends learned "--dd" that is a short-hand for
"--diff-merges=first-parent -p".
Will merge to 'next'?
source: <20231009160535.236523-1-sorganov@gmail.com>
* vd/loose-ref-iteration-optimization (2023-10-09) 4 commits
- files-backend.c: avoid stat in 'loose_fill_ref_dir'
- dir.[ch]: add 'follow_symlink' arg to 'get_dtype'
- dir.[ch]: expose 'get_dtype'
- ref-cache.c: fix prefix matching in ref iteration
The code to iterate over loose references have been optimized to
reduce the number of lstat() system calls.
Will merge to 'next'?
source: <pull.1594.v2.git.1696888736.gitgitgadget@gmail.com>
* jc/update-list-references-to-lore (2023-10-06) 1 commit
- doc: update list archive reference to use lore.kernel.org
Doc update.
Needs review.
source: <xmqq7cnz741s.fsf@gitster.g>
* cc/git-replay (2023-10-10) 14 commits
- replay: stop assuming replayed branches do not diverge
- replay: add --contained to rebase contained branches
- replay: add --advance or 'cherry-pick' mode
- 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: change rev walking options
- 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
Introduce "git replay", a tool meant on the server side without
working tree to recreate a history.
source: <20231010123847.2777056-1-christian.couder@gmail.com>
* jk/commit-graph-leak-fixes (2023-10-03) 10 commits
(merged to 'next' on 2023-10-06 at 5d202ef8b9)
+ commit-graph: clear oidset after finishing write
+ commit-graph: free write-context base_graph_name during cleanup
+ commit-graph: free write-context entries before overwriting
+ commit-graph: free graph struct that was not added to chain
+ commit-graph: delay base_graph assignment in add_graph_to_chain()
+ commit-graph: free all elements of graph chain
+ commit-graph: move slab-clearing to close_commit_graph()
+ merge: free result of repo_get_merge_bases()
+ commit-reach: free temporary list in get_octopus_merge_bases()
+ t6700: mark test as leak-free
Leakfix.
Will merge to 'master'.
source: <20231003202504.GA7697@coredump.intra.peff.net>
* jm/git-status-submodule-states-docfix (2023-10-04) 1 commit
(merged to 'next' on 2023-10-04 at 520b7711a4)
+ git-status.txt: fix minor asciidoc format issue
Docfix.
Will merge to 'master'.
source: <pull.1591.v3.git.1696386165616.gitgitgadget@gmail.com>
* rs/parse-opt-ctx-cleanup (2023-10-03) 1 commit
(merged to 'next' on 2023-10-04 at d5d0a2ce3b)
+ parse-options: drop unused parse_opt_ctx_t member
Code clean-up.
Will merge to 'master'.
source: <ebcaa9e1-d306-4c93-adec-3f35d7040531@web.de>
* tb/repack-max-cruft-size (2023-10-09) 5 commits
(merged to 'next' on 2023-10-09 at 38f039e880)
+ repack: free existing_cruft array after use
(merged to 'next' on 2023-10-06 at b3ca6df3b9)
+ builtin/repack.c: avoid making cruft packs preferred
+ builtin/repack.c: implement support for `--max-cruft-size`
+ builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
+ t7700: split cruft-related tests to t7704
"git repack" learned "--max-cruft-size" to prevent cruft packs from
growing without bounds.
Will merge to 'master'.
source: <cover.1696293862.git.me@ttaylorr.com>
source: <20231007172031.GA1524950@coredump.intra.peff.net>
source: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
* ak/color-decorate-symbols (2023-10-03) 1 commit
- decorate: add color.decorate.symbols config option
A new config for coloring.
Needs review.
source: <20231003205442.22963-1-andy.koppe@gmail.com>
* jc/attr-tree-config (2023-10-04) 2 commits
- attr: add attr.allowInvalidSource config to allow invalid revision
- attr: add attr.tree for setting the treeish to read attributes from
The attribute subsystem learned to honor `attr.tree` configuration
that specifies which tree to read the .gitattributes files from.
Reroll exists, but not picked up (a review sent).
source: <pull.1577.v2.git.git.1696443502.gitgitgadget@gmail.com>
* js/submodule-fix-misuse-of-path-and-name (2023-10-03) 6 commits
(merged to 'next' on 2023-10-06 at 1054b6e752)
+ t7420: test that we correctly handle renamed submodules
+ t7419: test that we correctly handle renamed submodules
+ t7419, t7420: use test_cmp_config instead of grepping .gitmodules
+ t7419: actually test the branch switching
+ submodule--helper: return error from set-url when modifying failed
+ submodule--helper: use submodule_from_path in set-{url,branch}
In .gitmodules files, submodules are keyed by their names, and the
path to the submodule whose name is $name is specified by the
submodule.$name.path variable. There were a few codepaths that
mixed the name and path up when consulting the submodule database,
which have been corrected. It took long for these bugs to be found
as the name of a submodule initially is the same as its path, and
the problem does not surface until it is moved to a different path,
which apparently happens very rarely.
Will merge to 'master'.
source: <0a0a157f88321d25fdb0be771a454b3410a449f3.camel@archlinux.org>
* ar/diff-index-merge-base-fix (2023-10-02) 1 commit
(merged to 'next' on 2023-10-06 at 0ff4dfc0e1)
+ diff: fix --merge-base with annotated tags
"git diff --merge-base X other args..." insisted that X must be a
commit and errored out when given an annotated tag that peels to a
commit, but we only need it to be a committish. This has been
corrected.
Will merge to 'master'.
source: <20231001151845.3621551-1-hi@alyssa.is>
* js/update-urls-in-doc-and-comment (2023-09-26) 4 commits
- doc: refer to internet archive
- doc: update links for andre-simon.de
- doc: update links to current pages
- doc: switch links to https
Stale URLs have been updated to their current counterparts (or
archive.org) and HTTP links are replaced with working HTTPS links.
Needs review.
source: <pull.1589.v2.git.1695553041.gitgitgadget@gmail.com>
* la/trailer-cleanups (2023-09-26) 4 commits
- trailer: only use trailer_block_* variables if trailers were found
- trailer: use offsets for trailer_start/trailer_end
- trailer: find the end of the log message
- commit: ignore_non_trailer computes number of bytes to ignore
Code clean-up.
Needs review.
source: <pull.1563.v4.git.1695709372.gitgitgadget@gmail.com>
* eb/hash-transition (2023-10-02) 30 commits
- t1016-compatObjectFormat: add tests to verify the conversion between objects
- t1006: test oid compatibility with cat-file
- t1006: rename sha1 to oid
- test-lib: compute the compatibility hash so tests may use it
- builtin/ls-tree: let the oid determine the output algorithm
- object-file: handle compat objects in check_object_signature
- tree-walk: init_tree_desc take an oid to get the hash algorithm
- builtin/cat-file: let the oid determine the output algorithm
- rev-parse: add an --output-object-format parameter
- repository: implement extensions.compatObjectFormat
- object-file: update object_info_extended to reencode objects
- object-file-convert: convert commits that embed signed tags
- object-file-convert: convert commit objects when writing
- object-file-convert: don't leak when converting tag objects
- object-file-convert: convert tag objects when writing
- object-file-convert: add a function to convert trees between algorithms
- object: factor out parse_mode out of fast-import and tree-walk into in object.h
- cache: add a function to read an OID of a specific algorithm
- tag: sign both hashes
- commit: export add_header_signature to support handling signatures on tags
- commit: convert mergetag before computing the signature of a commit
- commit: write commits for both hashes
- object-file: add a compat_oid_in parameter to write_object_file_flags
- object-file: update the loose object map when writing loose objects
- loose: compatibilty short name support
- loose: add a mapping between SHA-1 and SHA-256 for loose objects
- repository: add a compatibility hash algorithm
- object-names: support input of oids in any supported hash
- oid-array: teach oid-array to handle multiple kinds of oids
- object-file-convert: stubs for converting from one object format to another
Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.
Needs review.
source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
* jx/remote-archive-over-smart-http (2023-10-04) 4 commits
- archive: support remote archive from stateless transport
- transport-helper: call do_take_over() in connect_helper
- transport-helper: call do_take_over() in process_connect
- transport-helper: no connection restriction in connect_helper
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
Needs review.
source: <cover.1696432593.git.zhiyou.jx@alibaba-inc.com>
* jx/sideband-chomp-newline-fix (2023-10-04) 3 commits
- pkt-line: do not chomp newlines for sideband messages
- pkt-line: memorize sideband fragment in reader
- test-pkt-line: add option parser for unpack-sideband
Sideband demultiplexer fixes.
Needs review.
source: <cover.1696425168.git.zhiyou.jx@alibaba-inc.com>
* ty/merge-tree-strategy-options (2023-09-25) 1 commit
(merged to 'next' on 2023-09-29 at aa65b54416)
+ merge-tree: add -X strategy option
"git merge-tree" learned to take strategy backend specific options
via the "-X" option, like "git merge" does.
Hold. There is an unwanted structure copying going on.
cf. <a482d047-dd40-436d-8daa-0c74780af11f@gmail.com>
source: <pull.1565.v6.git.1695522222723.gitgitgadget@gmail.com>
* js/ci-coverity (2023-10-05) 6 commits
(merged to 'next' on 2023-10-05 at 253788f0d1)
+ coverity: detect and report when the token or project is incorrect
+ coverity: allow running on macOS
+ coverity: support building on Windows
+ coverity: allow overriding the Coverity project
+ coverity: cache the Coverity Build Tool
+ ci: add a GitHub workflow to submit Coverity scans
GitHub CI workflow has learned to trigger Coverity check.
Will merge to 'master'.
source: <pull.1588.v2.git.1695642662.gitgitgadget@gmail.com>
* js/config-parse (2023-09-21) 5 commits
- config-parse: split library out of config.[c|h]
- config.c: accept config_parse_options in git_config_from_stdin
- config: report config parse errors using cb
- config: split do_event() into start and flush operations
- config: split out config_parse_options
The parsing routines for the configuration files have been split
into a separate file.
Needs review.
source: <cover.1695330852.git.steadmon@google.com>
* jc/fake-lstat (2023-09-15) 1 commit
- cache: add fake_lstat()
(this branch is used by jc/diff-cached-fsmonitor-fix.)
A new helper to let us pretend that we called lstat() when we know
our cache_entry is up-to-date via fsmonitor.
Needs review.
source: <xmqqcyykig1l.fsf@gitster.g>
* rs/parse-options-value-int (2023-09-18) 2 commits
- parse-options: use and require int pointer for OPT_CMDMODE
- parse-options: add int value pointer to struct option
A bit of type safety for the "value" pointer used in the
parse-options API.
Not ready yet.
cf. <4014e490-c6c1-453d-b0ed-645220e3e614@web.de>
source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
* la/trailer-test-and-doc-updates (2023-09-07) 13 commits
(merged to 'next' on 2023-10-06 at 69fef35819)
+ trailer doc: <token> is a <key> or <keyAlias>, not both
+ trailer doc: separator within key suppresses default separator
+ trailer doc: emphasize the effect of configuration variables
+ trailer --unfold help: prefer "reformat" over "join"
+ trailer --parse docs: add explanation for its usefulness
+ trailer --only-input: prefer "configuration variables" over "rules"
+ trailer --parse help: expose aliased options
+ trailer --no-divider help: describe usual "---" meaning
+ trailer: trailer location is a place, not an action
+ trailer doc: narrow down scope of --where and related flags
+ trailer: add tests to check defaulting behavior with --no-* flags
+ trailer test description: this tests --where=after, not --where=before
+ trailer tests: make test cases self-contained
Test coverage for trailers has been improved.
Will merge to 'master'.
source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
* js/doc-unit-tests (2023-10-09) 3 commits
- ci: run unit tests in CI
- unit tests: add TAP unit test framework
- unit tests: add a project plan document
(this branch is used by js/doc-unit-tests-with-cmake.)
Process to add some form of low-level unit tests has started.
Will merge to 'next'?
source: <cover.1696889529.git.steadmon@google.com>
* js/doc-unit-tests-with-cmake (2023-10-09) 7 commits
- cmake: handle also unit tests
- cmake: use test names instead of full paths
- cmake: fix typo in variable name
- artifacts-tar: when including `.dll` files, don't forget the unit-tests
- unit-tests: do show relative file paths
- unit-tests: do not mistake `.pdb` files for being executable
- cmake: also build unit tests
(this branch uses js/doc-unit-tests.)
Update the base topic to work with CMake builds.
Needs review.
source: <pull.1579.v3.git.1695640836.gitgitgadget@gmail.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
Code clean-up.
Not ready to be reviewed yet.
source: <20230824205456.1231371-1-gitster@pobox.com>
* rj/status-bisect-while-rebase (2023-08-01) 1 commit
- status: fix branch shown when not only bisecting
"git status" is taught to show both the branch being bisected and
being rebased when both are in effect at the same time.
Needs review.
cf. <xmqqtttia3vn.fsf@gitster.g>
source: <48745298-f12b-8efb-4e48-90d2c22a8349@gmail.com>
--------------------------------------------------
[Discarded]
* tb/ci-coverity (2023-09-21) 1 commit
. .github/workflows: add coverity action
GitHub CI workflow has learned to trigger Coverity check.
Superseded by the js/ci-coverity topic.
source: <b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com>
* cw/git-std-lib (2023-09-11) 7 commits
. SQUASH???
. git-std-lib: add test file to call git-std-lib.a functions
. git-std-lib: introduce git standard library
. parse: create new library for parsing strings and env values
. config: correct bad boolean env value error message
. wrapper: remove dependency to Git-specific internal file
. hex-ll: split out functionality from hex
Another libification effort.
Superseded by the cw/prelim-cleanup topic.
cf. <xmqqy1hfrk6p.fsf@gitster.g>
cf. <20230915183927.1597414-1-jonathantanmy@google.com>
source: <20230908174134.1026823-1-calvinwan@google.com>
* so/diff-merges-d (2023-09-11) 2 commits
- diff-merges: introduce '-d' option
- diff-merges: improve --diff-merges documentation
Superseded by the so/diff-merges-dd topic.
source: <20230909125446.142715-1-sorganov@gmail.com>
* kn/rev-list-ignore-missing-links (2023-09-20) 1 commit
. revision: add `--ignore-missing-links` user option
Surface the .ignore_missing_links bit that stops the revision
traversal from stopping and dying when encountering a missing
object to a new command line option of "git rev-list", so that the
objects that are required but are missing can be enumerated.
Superseded by kn/rev-list-missing-fix
source: <20230920104507.21664-1-karthik.188@gmail.com>
^ permalink raw reply
* Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from
From: John Cai @ 2023-10-11 2:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, Jeff King, Jonathan Tan
In-Reply-To: <xmqqfs2iqg4k.fsf@gitster.g>
Hi Junio,
On 10 Oct 2023, at 18:14, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> 44451a2 (attr: teach "--attr-source=<tree>" global option to "git",
>> 2023-05-06) provided the ability to pass in a treeish as the attr
>> source. In the context of serving Git repositories as bare repos like we
>> do at GitLab however, it would be easier to point --attr-source to HEAD
>> for all commands by setting it once.
>>
>> Add a new config attr.tree that allows this.
>>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>> ---
>> Documentation/config.txt | 2 ++
>> Documentation/config/attr.txt | 5 +++
>> attr.c | 7 ++++
>> attr.h | 2 ++
>> config.c | 14 ++++++++
>> t/t0003-attributes.sh | 62 +++++++++++++++++++++++++++++++++++
>> 6 files changed, 92 insertions(+)
>> create mode 100644 Documentation/config/attr.txt
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 229b63a454c..b1891c2b5af 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.
>>
>> include::config/advice.txt[]
>>
>> +include::config/attr.txt[]
>> +
>> include::config/core.txt[]
>>
>> include::config/add.txt[]
>> diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
>> new file mode 100644
>> index 00000000000..be882523f8b
>> --- /dev/null
>> +++ b/Documentation/config/attr.txt
>> @@ -0,0 +1,5 @@
>> +attr.tree:
>> + A <tree-ish> to read gitattributes from instead of the worktree. See
>> + linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,
>> + treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take
>> + precedence over attr.tree.
>
> Properly typeset `--attr-source` and `GIT_ATTR_SOURCE`.
>
> A quick "git grep" in Documentation/config/*.txt tells me that
> nobody refers to an object type like <tree-ish>. Imitate what this
> was modeled after, namely Documentation/config/mailmap.txt, which
> says just
>
> ... a reference to a blob in the repository.
>
> without any half mark-up.
sounds good
>
> More importantly, the description makes one wonder what the
> precedence rule between these two (the general rule would be for
> command line parameter to override environment, if I recall
> correctly).
yes, that should be the case.
>
> I think the enumeration header usually is followed by double-colons
> among Documentation/config/*.txt files. Let's be consistent.
>
> In the context of this expression, "worktree" is a wrong noun to
> use---the term of art refers to an instance of "working tree",
> together with some "per worktree" administrative files inside .git/
> directory. On the other hand, "working tree" refers to the "files
> meant to be visible to build tools and editors part of the non-bare
> repository", which is what you want to use here.
>
> attr.tree::
> A tree object to read the attributes from, instead of the
> `.gitattributes` file in the working tree. In a bare
> repository, this defaults to HEAD:.gitattributes". If a
> given value does not resolve to a valid tree object, an
> empty tree is used instead. When `GIT_ATTR_SOURCE`
> environment variable or `--attr-source` command line option
> is used, this configuration variable has no effect.
>
> or something along that line, perhaps.
sounds good to me, thanks
>
>> diff --git a/attr.c b/attr.c
>> index bf2ea1626a6..0ae6852d12b 100644
>> --- a/attr.c
>> +++ b/attr.c
>> @@ -24,6 +24,8 @@
>> #include "tree-walk.h"
>> #include "object-name.h"
>>
>> +const char *git_attr_tree;
>> +
>> const char git_attr__true[] = "(builtin)true";
>> const char git_attr__false[] = "\0(builtin)false";
>> static const char git_attr__unknown[] = "(builtin)unknown";
>> @@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)
>> if (!default_attr_source_tree_object_name)
>> default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
>>
>> + if (!default_attr_source_tree_object_name) {
>> + default_attr_source_tree_object_name = git_attr_tree;
>> + ignore_bad_attr_tree = 1;
>> + }
>
> As long as "attr.tree" was read by calling git_default_attr_config()
> before we come here, git_attr_tree is not NULL and we allow bad attr
> tree in default_attr_source_tree_object_name. But stepping back a
> bit, even if "attr.tree" is unspecified, i.e., git_attr_tree is
> NULL, we set ignore_bad_attr_tree to true here.
>
> What it means is that after the above if() statement, if
> default_attr_source_tree_object_name is still NULL, we know that
> ignore_bad_attr_tree is already set to true.
>
>> if (!default_attr_source_tree_object_name &&
>> startup_info->have_repository &&
>> is_bare_repository()) {
>
> So would it make more sense to remove the assignment to the same
> variable we made in [1/2] around here (not seen in the post
> context)?
>
> Alternatively, even though it makes the code a bit more verbose, the
> logic might become clearer if you wrote the "assign from the config"
> part like so:
>
> if (!default_attr_source_tree_object_name && git_attr_tree) {
> default_attr_source_tree_object_name = git_attr_tree;
> ignore_bad_attr_tree = 1;
> }
>
> It would leave more flexibility to the code around here. You could
> for example add code that assigns a different value, a tree object
> that is required to exist, to default_attr_source_tree_object_name
> after this point, for example, without having to wonder what the
> "current" value of ignore_bad_attr_tree is.
good point--I think this logic is easier to follow.
>
>> +static int git_default_attr_config(const char *var, const char *value)
>> +{
>> + if (!strcmp(var, "attr.tree"))
>> + return git_config_string(&git_attr_tree, var, value);
>> +
>> + /* Add other attribute related config variables here and to
>> + Documentation/config/attr.txt. */
>
> /*
> * Our multi-line comments should look
> * more like this; opening slash-asterisk
> * and closing asterisk-slash sit on a line
> * on its own.
> */
>
>> @@ -342,6 +346,46 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
>> )
>> '
>>
>> +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
>
> Not a fault of this two-patch series, but we probably should refine
> this error reporting so that the reader can tell which one is being
> complained about, and optionally what the offending value was.
>
>
>> +test_expect_success 'attr.tree when HEAD is unborn' '
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> + cd empty &&
>> + echo $bad_attr_source_err >expect_err &&
>
> Let's not rely on words in the error message split with exactly one
> whitespace each and instead quote the variable properly. I.e.,
>
> echo "$bad_attr_source_err" >expect_err &&
>
> But this is not even used, as we do not expect it to fail. Perhaps
> remove it altogether?
>
>> + echo "f/path: test: unspecified" >expect &&
>> + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
>> + test_must_be_empty err &&
>> + test_cmp expect actual
>> + )
>> +'
>> +
>> +test_expect_success 'attr.tree points to non-existing ref' '
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> + cd empty &&
>> + echo $bad_attr_source_err >expect_err &&
>
> Ditto.
>
>> + echo "f/path: test: unspecified" >expect &&
>> + git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
>> + test_must_be_empty err &&
>> + test_cmp expect actual
>> + )
>> +'
>> +
>> +test_expect_success 'bad attr source defaults to reading .gitattributes file' '
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> + cd empty &&
>> + echo "f/path test=val" >.gitattributes &&
>> + echo "f/path: test: val" >expect &&
>> + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
>> + test_must_be_empty err &&
>> + test_cmp expect actual
>> + )
>> +'
>
> In other words, with the additional tests, we do not check error
> cases (which may be perfectly OK, if they are covered by existing
> tests). A bit curious.
Just checked and it does seem like we don't have a test case that covers the
error case. Will add in the next version.
>
>> @@ -356,6 +400,24 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
>
> Do we want to ensure which one takes precedence between the command
> line option and the environment? It's not like the one that is
> given the last takes effect.
yeah, might as well.
>
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> + cd empty &&
>> + git checkout -b attr-source &&
>> + test_commit "val1" .gitattributes "f/path test=val1" &&
>> + git checkout -b attr-tree &&
>> + test_commit "val2" .gitattributes "f/path test=val2" &&
>> + git checkout attr-source &&
>> + echo "f/path: test: val1" >expect &&
>> + git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
>> + test_cmp expect actual &&
>> + GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
>> + test_cmp expect actual
>> + )
>> +'
>> +
>> test_expect_success 'bare repository: with --source' '
>> (
>> cd bare.git &&
>
> Other than that, looking great. Thanks.
thanks!
John
^ permalink raw reply
* Re: [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Isoken Ibizugbe @ 2023-10-11 5:46 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Johannes Schindelin
In-Reply-To: <CAP8UFD3RV-70RG6H86+7E-ZKrqhfgFRfRQdMc6DLGMXPAEf31Q@mail.gmail.com>
Hi Christian,
I hope you're doing well. I did some research, but unfortunately, I
couldn't find any clear documentation stating that setup tests should
be renamed. However, I did find another issue that I think I could
work on - 'Amend error messages and prompts of various (sub)commands
#635'. Should I go on with it?
Thank you for your help and your guidance, I appreciate it.
On Mon, Oct 9, 2023 at 2:38 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Mon, Oct 9, 2023 at 3:33 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
> >
> > Thank you Christian for the help, I have another idea for a micro
> > project "modernizing a test script". I have found test scripts that
> > needs modernizing , which is to rename the setup test 'prepare
> > reference tree' to 'setup'. Is it appropriate for a micro project and
> > should I go ahead with it.
>
> I am not sure these kinds of renames are worth doing, but if you find
> some clear doc or Junio saying that setup tests should be named
> "setup", then that might be worth a try.
^ permalink raw reply
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Naomi Ibe @ 2023-10-11 5:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34yiqfoh.fsf@gitster.g>
Okay then, I'll work with these guidelines in mind. Thank you very much
On Tue, Oct 10, 2023 at 11:24 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Naomi Ibe <naomi.ibeh69@gmail.com> writes:
>
> > Thank you very much! I'd definitely make those changes on my next patch.
>
> [administrivia] do not top post.
>
> > Should I begin work on version 2 or should I still wait for additional
> > input on the version 1?
>
> There is no "rule", but based on observations on how people behave,
> e.g.
>
> * for a small patch like this that can be given a good review in 10
> minutes or so, those who do not do so within the first 3 days
> will probably not do so.
>
> * once a reasonably thorough review is given, those who haven't
> responded to the patch and do not have much else to say are
> unlikely to respond.
>
> * on the other hand, after such a review is given, those who do not
> agree with the review tend to respond rather quickly, to get
> their voice in before it becomes too late.
>
> I would say it would be good to start working on it right away and
> use a couple of days reviewing it yourself before posting it.
^ permalink raw reply
* Re: [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Naomi Ibe @ 2023-10-11 6:06 UTC (permalink / raw)
To: Doreen Wanyama; +Cc: git
In-Reply-To: <CANhBNnvUx=KG2RkkJEamr2KHerXoDrvW2qe5zKq_xiV1t9V92g@mail.gmail.com>
Good morning, I followed this link
https://github.com/gitgitgadget/git/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22
We're not expected to pick something too tasking like working on heavy
code fixes, just pick a project you know you can round up and
contribute to. A project that doesn't involve plenty of major code
manipulations. For task submission, you'd need to learn how to
navigate "git format-patch" and "git send-email" commands(If you read
up on these two and their applications and you should be fine), or how
to use GitGitGadget pull requests to send code patches directly to
this mailing list (https://github.com/gitgitgadget/git)
On Mon, Oct 9, 2023 at 7:32 PM Doreen Wanyama <doreenwanyama20@gmail.com> wrote:
>
> Dear Git community,
>
> I hope you are all doing well. I am writing to show my interest in
> working in the project titled move existing tests to a unit testing
> framework. This is because I have always been intrigued by the work
> the git community does and hence I am interested in being part of
> this. I have gone through the links provided about getting started on
> this. I spent yesterday evening and a better part of today trying to
> understand the resources. As of now I would like to start working on a
> microproject since I understand this is the first step. I am finding
> it difficult though to start. Someone to please help me understand how
> I should go about this or how I should go about finding my first
> microproject. Just a brief explanation will help.
> Thank you in advance.
>
> Best regards,
>
> Doreen Wanyama
^ permalink raw reply
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Dragan Simic @ 2023-10-11 6:17 UTC (permalink / raw)
To: Naomi Ibe; +Cc: Junio C Hamano, git
In-Reply-To: <CACS=G2wXkz9OyR5e0ADzWVy3ejibF-Js=sdJYJDRMh8qskO-9A@mail.gmail.com>
On 2023-10-11 07:50, Naomi Ibe wrote:
> Okay then, I'll work with these guidelines in mind. Thank you very much
If I may notice, avoiding top posting has already been suggested twice,
IIRC.
I think that various webmails have made top posting popular, and
actually forced people a bit to do that through the way their user
interfaces were laid out, which made inline replies kind of a dying art,
which is really sad to see. It is so much more useful when replies are
posted inline, i.e. right below the contents they refer to.
> On Tue, Oct 10, 2023 at 11:24 PM Junio C Hamano <gitster@pobox.com>
> wrote:
>>
>> Naomi Ibe <naomi.ibeh69@gmail.com> writes:
>>
>> > Thank you very much! I'd definitely make those changes on my next patch.
>>
>> [administrivia] do not top post.
>>
>> > Should I begin work on version 2 or should I still wait for additional
>> > input on the version 1?
>>
>> There is no "rule", but based on observations on how people behave,
>> e.g.
>>
>> * for a small patch like this that can be given a good review in 10
>> minutes or so, those who do not do so within the first 3 days
>> will probably not do so.
>>
>> * once a reasonably thorough review is given, those who haven't
>> responded to the patch and do not have much else to say are
>> unlikely to respond.
>>
>> * on the other hand, after such a review is given, those who do not
>> agree with the review tend to respond rather quickly, to get
>> their voice in before it becomes too late.
>>
>> I would say it would be good to start working on it right away and
>> use a couple of days reviewing it yourself before posting it.
^ permalink raw reply
* [PATCH] t-progress.c : unit tests for progress.c
From: Siddharth Singh @ 2023-10-11 8:27 UTC (permalink / raw)
To: git; +Cc: nasamuffin
These tests are directly inspired from the tests inside
t/t0500-progress-display.sh
The existing shell tests for the Git progress library only test the output of the library, not the correctness of the progress struct fields. Unit tests can fill this gap and improve confidence that the library works as expected. For example, unit tests can verify that the progress struct fields are updated correctly when the library is used.
Change-Id: I190522f29fdab9291af71b7788eeee2c0f26282d
Signed-off-by: Siddharth Singh <siddhartth@google.com>
---
Dear Git Community,
As you may be aware, my colleague Josh is proposing a unit testing framework[1] on the mailing list. I attempted to use the latest version of that series to convert t/helper/test-progress.c to unit tests. However, while writing the tests, I realized that the way progress.c is implemented makes it very difficult to test it in units.
Firstly, most unit tests are typically written as a method that takes the expected output and the actual output of the unit being tested, and compares them for equality. However, because it's intended to print user-facing output on an interactive terminal, progress.c prints everything out to stderr, which makes it difficult to unit test.
As written, a unit test for throughput progress printing would have to capture stdout from the library and compare it to the expected output, which is difficult to implement and unusual for unit tests anyway.
There are a few ways to work around this issue in my opinion. One way is to modify the library that does not print to output stream and returns the data to the caller:
static void display(struct progress *progress, uint64_t n, char *done){
…
}
becomes
struct strbuf display(struct progress *progress,uint64_t n,char *done){
…
}
Another way is to capture the output from the library into a file instead of stdout and then read it from the file and compare it to the expected output, However, this is a difficult task, and I recommend against it.
We may need to make some changes to the way these libraries are implemented in order to make them unit testable in the future.
Therefore, i want to ask if it's worth investing time in developing a solution?
I look forward to hearing your thoughts on this.
[1] https://lore.kernel.org/git/cover.1692297001.git.steadmon@google.com/
Makefile | 1 +
t/unit-tests/.gitignore | 1 +
t/unit-tests/t-progress.c | 229 ++++++++++++++++++++++++++++++++++++++
3 files changed, 231 insertions(+)
create mode 100644 t/unit-tests/t-progress.c
diff --git a/Makefile b/Makefile
index 4016da6e39..eabbfe32cf 100644
--- a/Makefile
+++ b/Makefile
@@ -1335,6 +1335,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-progress
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_DIR)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
index e292d58348..cf5eb4803e 100644
--- a/t/unit-tests/.gitignore
+++ b/t/unit-tests/.gitignore
@@ -1,2 +1,3 @@
/t-basic
/t-strbuf
+/t-progress
diff --git a/t/unit-tests/t-progress.c b/t/unit-tests/t-progress.c
new file mode 100644
index 0000000000..437d6acf16
--- /dev/null
+++ b/t/unit-tests/t-progress.c
@@ -0,0 +1,229 @@
+#include "test-lib.h"
+#include "progress.c"
+
+
+static void t_simple_progress()
+{
+ int total = 4;
+ struct progress *progress = NULL;
+ int i;
+ progress = start_progress("Working hard", total);
+ for (i = 1; i <= total; i++) {
+ display_progress(progress, i);
+ check_uint(i, ==, progress->last_value);
+ check_str(progress->title, "Working hard");
+ check_int(progress->last_percent, ==, i * 100 / total);
+ }
+ return;
+}
+
+static void t_simple_progress_percent_text()
+{
+ int total = 4;
+ struct progress *progress = NULL;
+ int i;
+ char *expected[] = {
+ " 0% (0/4)",
+ " 25% (1/4)",
+ " 50% (2/4)",
+ " 75% (3/4)",
+ "100% (4/4)"
+ };
+ char *instructions[] = {
+ "progress",
+ "progress",
+ "progress",
+ "progress",
+ "progress"
+ };
+ int value[] = {
+ 0,
+ 1,
+ 2,
+ 3,
+ 4
+ };
+ progress = start_progress("Working hard", total);
+ for (i = 0; i < 5; i++) {
+ if(strcmp(instructions[i], "progress")==0){
+ display_progress(progress, value[i]);
+ check_str(progress->title, "Working hard");
+ check_str(progress->counters_sb.buf, expected[i]);
+ check_uint(i * (100 / total), ==, progress->last_percent);
+ }
+ }
+ return;
+}
+
+static void t_progress_display_breaks_long_lines_1()
+{
+ int total = 100000;
+ struct progress *progress = NULL;
+ int i;
+ char *expected[4] = {
+ " 0% (100/100000)",
+ " 1% (1000/100000)",
+ " 10% (10000/100000)",
+ "100% (100000/100000)"
+ };
+ char *instructions[] = {
+ "progress",
+ "progress",
+ "progress",
+ "progress"
+ };
+ int value[] = {
+ 100,
+ 1000,
+ 10000,
+ 100000
+ };
+ progress = start_progress(
+ "Working hard.......2.........3.........4.........5.........6",
+ total);
+ for (i = 0; i < 4; i++) {
+ if(strcmp(instructions[i], "progress")==0){
+ display_progress(progress, value[i]);
+ }
+ check_str(progress->title, "Working hard.......2.........3.........4.........5.........6");
+ check_str(progress->counters_sb.buf, expected[i]);
+ }
+ return;
+}
+
+static void t_progress_display_breaks_long_lines_2()
+{
+ int total = 100000;
+ struct progress *progress = NULL;
+ int i;
+ char *expected[] = {
+ "",
+ " 0% (1/100000)",
+ "",
+ " 0% (2/100000)",
+ " 10% (10000/100000)",
+ "100% (100000/100000)"
+ };
+ char *instructions[] = {
+ "update",
+ "progress",
+ "update",
+ "progress",
+ "progress",
+ "progress"
+ };
+ int value[] = {
+ -1,
+ 1,
+ -1,
+ 2,
+ 10000,
+ 100000
+ };
+ progress = start_progress(
+ "Working hard.......2.........3.........4.........5.........6",
+ total);
+ for (i = 0; i < 5; i++) {
+ if(strcmp(instructions[i], "progress")==0){
+ display_progress(progress, value[i]);
+ check_str(progress->title, "Working hard.......2.........3.........4.........5.........6");
+ check_str(progress->counters_sb.buf, expected[i]);
+ }else if(strcmp(instructions[i], "update")==0){
+ progress_test_force_update();
+ }
+ }
+ return;
+}
+
+static void t_progress_display_breaks_long_lines_3()
+{
+ int total = 100000;
+ struct progress *progress = NULL;
+ int i;
+ char *expected[4] = {
+ " 25% (25000/100000)",
+ " 50% (50000/100000)",
+ " 75% (75000/100000)",
+ "100% (100000/100000)"
+ };
+ char *instructions[] = {
+ "progress",
+ "progress",
+ "progress",
+ "progress"
+ };
+ int value[] = {
+ 25000,
+ 50000,
+ 75000,
+ 100000
+ };
+ progress = start_progress(
+ "Working hard.......2.........3.........4.........5.........6",
+ total);
+ for (i = 0; i < 4; i++) {
+ if(strcmp(instructions[i], "progress")==0){
+ display_progress(progress, value[i]);
+ check_str(progress->title, "Working hard.......2.........3.........4.........5.........6");
+ check_str(progress->counters_sb.buf, expected[i]);
+ }else if(strcmp(instructions[i], "update")==0){
+ progress_test_force_update();
+ }
+ }
+ return;
+}
+
+
+static void t_progress_shortens_crazy_caller()
+{
+ int total = 1000;
+ struct progress *progress = NULL;
+ int i;
+ char *expected[4] = {
+ " 10% (100/1000)",
+ " 20% (200/1000)",
+ " 0% (1/1000)",
+ "100% (1000/1000)"
+ };
+ char *instructions[] = {
+ "progress",
+ "progress",
+ "progress",
+ "progress"
+ };
+ int value[] = {
+ 100,
+ 200,
+ 1,
+ 1000
+ };
+ progress = start_progress(
+ "Working hard.......2.........3.........4.........5.........6",
+ total);
+ for (i = 0; i < 4; i++) {
+ if(strcmp(instructions[i], "progress")==0){
+ display_progress(progress, value[i]);
+ check_str(progress->title, "Working hard.......2.........3.........4.........5.........6");
+ check_str(progress->counters_sb.buf, expected[i]);
+ }else if(strcmp(instructions[i], "update")==0){
+ progress_test_force_update();
+ }
+ }
+ return;
+}
+
+int cmd_main(int argc, const char **argv)
+{
+ TEST(t_simple_progress(), "Simple progress upto 3 units");
+ TEST(t_simple_progress_percent_text(),
+ "Simple progress with percent output");
+ TEST(t_progress_display_breaks_long_lines_1(),
+ "progress display breaks long lines #1");
+ TEST(t_progress_display_breaks_long_lines_2(),
+ "progress display breaks long lines #2");
+ TEST(t_progress_display_breaks_long_lines_3(),
+ "progress display breaks long lines #3");
+ TEST(t_progress_shortens_crazy_caller(),
+ "progress shortens - crazy caller");
+ return test_done();
+}
--
2.40.GIT
^ permalink raw reply related
* Bug: git stash store can create stash entries that can't be dropped
From: Erik Cervin Edin @ 2023-10-11 8:42 UTC (permalink / raw)
To: Git Mailing List
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
git stash store HEAD && git stash drop
What did you expect to happen? (Expected behavior)
To either fail storing HEAD or the ability to drop the stash entry, even if it
wasn't created using
git stash create
What happened instead? (Actual behavior)
A stash entry is created that cannot be dropped, because it's not
stash-like commit.
git stash drop
fatal: 'refs/stash@{0}' is not a stash-like commit
What's different between what you expected and what actually happened?
A corrupt entry is added to the stash and it cannot be removed, expect probably
git stash clear
Anything else you want to add:
Any guidance in modifying the stash reflog so that I can remove the stash entry
manually, or other suggestions are appreciated.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27
02:56:13 UTC 2023 x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
pre-commit
^ permalink raw reply
* Re: [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Christian Couder @ 2023-10-11 8:47 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git, Johannes Schindelin
In-Reply-To: <CAJHH8bFLqs7B5UaCFZv+P8yS2zeXLoKoV6YTUB_xFcG8ZLn_WA@mail.gmail.com>
Hi Isoken,
On Wed, Oct 11, 2023 at 7:48 AM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
>
> Hi Christian,
>
> I hope you're doing well. I did some research,
Nice that you did some research and are mentioning it.
However please reply inline instead of top-posting (see
https://en.wikipedia.org/wiki/Posting_style). On
https://git.github.io/General-Microproject-Information/ there is a
section called "Some conventions and tips" which has a "Reply inline"
subsection about this.
> but unfortunately, I
> couldn't find any clear documentation stating that setup tests should
> be renamed.
Ok, so it's indeed better to find something else to work on.
> However, I did find another issue that I think I could
> work on - 'Amend error messages and prompts of various (sub)commands
> #635'. Should I go on with it?
Yeah, I think it's a good idea to work on this. See this discussion
thread where we give advice to Naomi about improving error messages as
microproject:
https://lore.kernel.org/git/CACS=G2zsJxP+NWuosZyrFGctJptHNYTrULErRo_Ns41KeMuMqA@mail.gmail.com/#r
Thanks,
Christian.
^ permalink raw reply
* RE: [RFC] Define "precious" attribute and support it in `git clean`
From: Richard Kerry @ 2023-10-11 10:06 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <xmqqttqytnqb.fsf@gitster.g>
> > I'd like to propose adding a new standard gitattribute "precious".
>
> ;-).
The version of CVS that I used to use, CVSNT, was a lot more careful about the user's files than Git is inclined to be.
If CVSNT, while doing an Update, came across a non-tracked file that was in the way of something that it wanted to write, then the Update would be aborted showing a list of any files that were "in the way". The user could then rename/delete them or redo the Update with a "force" parameter to indicate that such items could be overwritten.
Git has tended to take an approach of "if it's important it'll be tracked by Git - anything else can be trashed with impunity.". Over the years people have been caught out by this and lost work. It may well be that in a Linux development world anything other than tracked source files can be summarily deleted, but in a wider world, like Windows, or environments that are not software development, or that need special files lying around, this is not always an entirely reasonable approach.
> Over the years, I've seen many times scenarios that would have been helped
> if we had not just "tracked? ignored? unignored?" but also the fourth kind
> [*]. The word "ignored" (or "excluded") has always meant "not tracked, not
> to be tracked, and expendable" to Git, and "ignored but unexpendable" class
> was missing. I even used the term "precious" myself in those discussions. At
> the concept level, I support the effort 100%, but as always, the devil will be in
> the details.
>
> Scenarios that people wished for "precious" traditionally have been
>
> * You are working on 'master'. You have in your .gitignore or
> .git/info/exclude a line to ignore path A, and have random
> scribbles in a throw-away file there. There is another branch
> 'seen', where they added some tracked contents at path A/B. You
> do "git checkout seen" and your file A that is an expendable file,
> because it is listed as ignored in .git/info/exclude, is removed
> to make room for creating A/B.
So checkout aborts, saying "A is in the way".
> * Similar situation, but this time, 'seen' branch added a tracked
> contents at path A. Again, "git checkout seen" will discard the
> expendable file A and replace it with tracked contents.
So checkout aborts, saying "A is in the way".
> * Instead of "git checkout", you decide to merge the branch 'seen'
> to the checkout of 'master', where you have an ignored path A.
> Because merging 'seen' would need to bring the tracked contents
> of either A/B (in the first scenario above) or A (in the second
> scenario), your "expendable" A will be removed to make room.
So merge aborts, saying "A is in the way". It is entirely conventional to have merge conflicts that the user needs to resolve. This is just another kind of conflict.
> In previous discussions, nobody was disturbed that "git clean" was unaware
> of the "precious" class, but if we were to have the "precious" class in addition
> to "ignored" aka "expendable", I would not oppose to teach "git clean" about
> it, too.
Indeed, if something is explicitly precious then nothing should summarily delete it.
I know this goes against some stated design decisions of early Git, but in the CVSNT world *all* files were considered precious and would always cause an update to be aborted if there were any inclination to replace them.
An option might be to state, in config, whether a project, or everything, should be managed on the basis of "all untracked files are precious" or "files may be explicitly marked precious", or, as now, "nothing is precious".
Regards,
Richard.
PS. I think I've caught all places where my fingers typed "previous" when my brain meant "precious" - apologies if I've missed any.
^ permalink raw reply
* Re: [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Karthik Nayak @ 2023-10-11 10:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
In-Reply-To: <xmqqil7etndo.fsf@gitster.g>
On Tue, Oct 10, 2023 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > I had already reviewed the patches internally at GitLab, so for what
> > it's worth please feel free to add my Reviewed-by.
>
> Great. It seems that 'seen' with this series fails to pass the
> tests, though.
>
> https://github.com/git/git/actions/runs/6462854176/job/17545104753
Seems like this is because of commit-graph being enabled, I think the
best thing to do here
would be to disable the commit graph of these tests.
This should do:
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index bbff66e4fc..39a8402682 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -5,6 +5,11 @@ test_description='handling of missing objects in rev-list'
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
+# Disable writing the commit graph as the tests depend on making particular
+# commits hidden, the graph is created before that and rev-list
would default
+# to using the commit graph in such instances.
+GIT_TEST_COMMIT_GRAPH=0
+
# We setup the repository with two commits, this way HEAD is always
# available and we can hide commit 1.
test_expect_success 'create repository and alternate directory' '
Thanks for reporting. I'll wait for a day or two (for reviews) and
will add this to the second version
of this series.
^ permalink raw reply
* Re: [PATCH 0/3] Add `-p' option to `git-mv', inspired by `mkdir'
From: Hugo Sales @ 2023-10-11 12:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee, Shaoxuan Yuan
In-Reply-To: <xmqq1qe3wbt1.fsf@gitster.g>
> On 10/10/2023 12:39 AM GMT Junio C Hamano <gitster@pobox.com> wrote:
>
> Is there a reason why somebody benefits by us retaining the current
> behaviour, where
>
> $ git mv file no/such/dir/yet/file
>
> fails with "No such file or directory", and they are forced to say
> either
>
> $ mkdir -p no/such/dir/yet
> $ git mv file no/such/dir/yet/file
>
> or
>
> $ git mv -p file no/such/dir/yet/file
Somewhat, as it could be a typo, so if you actually want to create a new folder, you have to be explicit. I wouldn't be opposed to removing the need for the flag, but if we did, I think we should warn the user that a new directory was created.
> Imagine there is no "no" directory (so nothing under it
> exists), and you did this (you do have a regular file "file").
>
> $ git mv [-p] file no/such/dir/yet
>
> What should happen? Would we do the equivalent of
>
> $ mkdir -p no/such/dir && git mv file no/such/dir/yet
>
> or are we doing
>
> $ mkdir -p no/such/dir/yet && git mv file no/such/dir/yet/file
>
> Both are plausible, and "mkdir -p" does not have such a nasty
> ambiguity. That is what makes me unsure about the new feature
> (again, either with required "-p" or with implied "-p").
I think the ambiguity is resolved by the inclusion of lack thereof of a trailing `/`. This is what the implementation already does, too, before this patch. So
$ git mv [-p] file no/such/dir/yet
would be the same as
$ mkdir -p no/such/dir && git mv file no/such/dir/yet
and
$ git mv [-p] file no/such/dir/yet/
would be the same as
$ mkdir -p no/such/dir/yet && git mv file no/such/dir/yet/
Moving `file` to `no/such/dir/yet/file`
Thanks for the feedback,
Hugo
P.S. Apologies for the duplicated email, Junio, I did a Reply instead of a Reply All
^ permalink raw reply
* [PATCH] revision: Don't queue uninteresting commits
From: Øystein Walle @ 2023-10-11 12:35 UTC (permalink / raw)
To: git; +Cc: derrickstolee, Øystein Walle
Currently all given commits are added to the topo_queue during
init_topo_walk(). Later on in get_revision_1() the uninteresting ones
are skipped because simplify_commit() tells it to.
Let's not add them to the topo_queue in the first place.
Signed-off-by: Øystein Walle <oystwa@gmail.com>
---
I noticed this while trying to understand the generation based algorithm
introduced in b45424181e (revision.c: generation-based topo-order
algorithm, 2018-11-01) in an attempt to write a similar one for
gitoxide. Comparing my solution to git's output I fixed a mismatch by
essentially doing this, and it turns out it works in git too. I am not
extremely confident, but all the tests pass...
For fun I also tried removing the UNINTERESTING check from
get_commit_action() altogether but then a lot of tests fail. I expected
that because both the flag and function predate the new algorithm.
revision.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 2f4c53ea20..deeab813c7 100644
--- a/revision.c
+++ b/revision.c
@@ -3681,7 +3681,8 @@ static void init_topo_walk(struct rev_info *revs)
for (list = revs->commits; list; list = list->next) {
struct commit *c = list->item;
- if (*(indegree_slab_at(&info->indegree, c)) == 1)
+ if (*(indegree_slab_at(&info->indegree, c)) == 1 &&
+ !(c->object.flags & UNINTERESTING))
prio_queue_put(&info->topo_queue, c);
}
--
2.34.1
^ permalink raw reply related
* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
From: Bagas Sanjaya @ 2023-10-11 13:41 UTC (permalink / raw)
To: Jeff King, Todd Zullinger
Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <20230925161748.GA2149383@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 3282 bytes --]
On Mon, Sep 25, 2023 at 12:17:48PM -0400, Jeff King wrote:
> On Mon, Sep 25, 2023 at 10:48:29AM -0400, Todd Zullinger wrote:
>
> > From the peanut gallery... could the presence or lack of the
> > Email::Valid perl module be a factor?
>
> Ah, thanks! The thought of differing modules even occurred to me, since
> I know we have a few optimistic dependencies, but when I looked I didn't
> manage to find that one (somehow I thought Mail::Address was the
> interesting one here; I think I might be getting senile).
>
> With Email::Valid installed, I can reproduce with just (in git.git, but
> I think it would work in any repo):
>
> $ echo "exit 0" >.git/hooks/sendemail-validate
> $ chmod +x .git/hooks/sendemail-validate
> $ git send-email --dry-run -1 --to=foo@example.com,bar@example.com
> error: unable to extract a valid address from: foo@example.com,bar@example.com
>
> Disabling the hook with "chmod -x" makes the problem go away (and this
> is with current "master", hence the more readable error message).
>
> I think the issue is that a8022c5f7b ends up in extract_valid_address()
> via this call stack:
>
> $ = main::extract_valid_address('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1161
> $ = main::extract_valid_address_or_die('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 2087
> @ = main::unique_email_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1507
> @ = main::gen_header() called from file '/home/peff/compile/git/git-send-email' line 2113
> . = main::validate_patch('/tmp/WfoPQSKCUa/0001-The-twelfth-batch.patch', 'auto') called from file '/home/peff/compile/git/git-send-email' line 815
>
> whereas prior to that commit, we hit it later:
>
> $ = main::extract_valid_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1166
> @ = main::validate_address('foo@example.com') called from file '/home/peff/compile/git/git-send-email' line 1189
> @ = main::validate_address_list('foo@example.com', 'bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1348
> @ = main::process_address_list('foo@example.com,bar@example.com') called from file '/home/peff/compile/git/git-send-email' line 1091
>
> So the issue is the call to gen_header() added in validate_patch(). We
> won't yet have processed the address lists by that point. We can move
> those calls up, but it requires moving a bit of extra code, too (like
> the parts prompting for the "to" list if it isn't filled in).
>
> Possibly the validation checks need to be moved down, if they want to
> see a more complete view of the emails. But now we're doing more work
> (like asking the user to write the cover letter!) before we do
> validation, which is probably bad.
>
> So I dunno. Maybe gen_header() should be lazily doing this
> process_address_list() stuff? I'm not very familiar with the send-email
> code, so I'm not sure what secondary effects that could have.
>
Michael, did you look into this since you authored the culprit?
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 06/20] commit-graph: check consistency of fanout table
From: Taylor Blau @ 2023-10-11 14:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210458.GF3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:04:58PM -0400, Jeff King wrote:
> We use bsearch_hash() to look up items in the oid index of a
> commit-graph. It also has a fanout table to reduce the initial range in
> which we'll search. But since the fanout comes from the on-disk file, a
> corrupted or malicious file can cause us to look outside of the
> allocated index memory.
This is all very well written and explained. The patch LGTM.
> ---
> So I actually implemented the bsearch_hash() bounds checks and wrote
> tests for midx and idx files before realizing how they handle this. ;)
> Which makes sense, because the usual outcome for a corrupted idx file is
> for it to say "non-monotonic index", which I have seen lead to user
> confusion. Arguably we should have it say something about "hey, your idx
> file seems to be corrupted, because...". But that can be its own topic.
Yeah, I definitely agree that that is out of scope here, and can be left
as #leftoverbits.
Thanks,
Taylor
^ permalink raw reply
* Re: [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Doreen Wanyama @ 2023-10-11 14:48 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2w+_o_85DBrv2vpR6Ym8R7XeijJDbaB1y8a5xkNztyoRA@mail.gmail.com>
Thank you for this. Let me check them out
On Wed, Oct 11, 2023 at 9:07 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> Good morning, I followed this link
> https://github.com/gitgitgadget/git/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22
> We're not expected to pick something too tasking like working on heavy
> code fixes, just pick a project you know you can round up and
> contribute to. A project that doesn't involve plenty of major code
> manipulations. For task submission, you'd need to learn how to
> navigate "git format-patch" and "git send-email" commands(If you read
> up on these two and their applications and you should be fine), or how
> to use GitGitGadget pull requests to send code patches directly to
> this mailing list (https://github.com/gitgitgadget/git)
>
> On Mon, Oct 9, 2023 at 7:32 PM Doreen Wanyama <doreenwanyama20@gmail.com> wrote:
> >
> > Dear Git community,
> >
> > I hope you are all doing well. I am writing to show my interest in
> > working in the project titled move existing tests to a unit testing
> > framework. This is because I have always been intrigued by the work
> > the git community does and hence I am interested in being part of
> > this. I have gone through the links provided about getting started on
> > this. I spent yesterday evening and a better part of today trying to
> > understand the resources. As of now I would like to start working on a
> > microproject since I understand this is the first step. I am finding
> > it difficult though to start. Someone to please help me understand how
> > I should go about this or how I should go about finding my first
> > microproject. Just a brief explanation will help.
> > Thank you in advance.
> >
> > Best regards,
> >
> > Doreen Wanyama
^ permalink raw reply
* Re: [PATCH 07/20] midx: check size of pack names chunk
From: Taylor Blau @ 2023-10-11 14:52 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210514.GG3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:14PM -0400, Jeff King wrote:
> @@ -176,9 +176,16 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
>
> cur_pack_name = (const char *)m->chunk_pack_names;
> for (i = 0; i < m->num_packs; i++) {
> + const char *end;
> + size_t avail = m->chunk_pack_names_len -
> + (cur_pack_name - (const char *)m->chunk_pack_names);
> +
This patch all looks good to me, but reading this hunk gave me a little
bit of pause. I was wondering what might happen if chunk_pack_names_len
was zero, and subtracting some other non-zero size_t from it might cause
us to wrap around.
But I think that's a non-issue here, since we'd set cur_pack_name to the
beginning of the chunk, and compute avail as 0 - (m->chunk_pack_names -
m->chunk_pack_names), and get 0 back, causing our memchr() lookup below
to fail, and for us to report this chunk is garbage.
And since cur_pack_name monotonically increases, I think that there is
an inductive argument to be made that this subtraction is always safe to
do. But it couldn't hurt to do something like:
size_t read = cur_pack_name - (const char *)m->chunk_pack_names;
size_t avail = m->chunk_pack_names_len;
if (read > avail)
die("...");
avail -= read;
to make absolutely sure that we would never underflow here.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 08/20] midx: enforce chunk alignment on reading
From: Taylor Blau @ 2023-10-11 14:56 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210523.GH3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:23PM -0400, Jeff King wrote:
> ---
> chunk-format.c | 8 +++++++-
> chunk-format.h | 3 ++-
> commit-graph.c | 2 +-
> midx.c | 3 ++-
> t/t5319-multi-pack-index.sh | 14 ++++++++++++++
> 5 files changed, 26 insertions(+), 4 deletions(-)
Very nicely written and explained. I agree that the choice you made here
(to validate the alignment in the chunk-format API itself when reading
the table of contents) feels more sensible and places less burden on the
callers.
LGTM, let's keep reading...
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 08/20] midx: enforce chunk alignment on reading
From: Taylor Blau @ 2023-10-11 15:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009210523.GH3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 05:05:23PM -0400, Jeff King wrote:
> @@ -120,6 +121,11 @@ int read_table_of_contents(struct chunkfile *cf,
> error(_("terminating chunk id appears earlier than expected"));
> return 1;
> }
> + if (chunk_offset % expected_alignment != 0) {
Oops, I missed this in my first read. I'm definitely nit-picking here,
but this should probably be:
if (chunk_offset % expected_alignment)
without the trailing "!= 0".
I don't have a strong preference here, since we are doing a comparison
of an arithmetic operation against an (un-)expected value. But I think
the CodingGuidelines would technically call this out of style...
> + error(_("chunk id %"PRIx32" not %d-byte aligned"),
> + chunk_id, expected_alignment);
> + return 1;
> + }
>
> table_of_contents += CHUNK_TOC_ENTRY_SIZE;
> next_chunk_offset = get_be64(table_of_contents + 4);
Thanks,
Taylor
^ permalink raw reply
* [Outreachy][PATCH 0/1] builtin/branch.c: ammend die() error message
From: Isoken June Ibizugbe @ 2023-10-11 15:24 UTC (permalink / raw)
To: git
This patch improves the consistency and clarity of error messages across Git commands. It adheres to Git's Coding Guidelines for error messages:
- Error messages no longer end with a full stop.
- Capitalization is avoided in error messages.
- Error messages lead with a description of the issue, enhancing readability.
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
Isoken June Ibizugbe (1):
branch.c: ammend error messages for die()
builtin/branch.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
--
2.42.0.325.g3a06386e31
^ permalink raw reply
* [PATCH 1/1] branch.c: ammend error messages for die()
From: Isoken June Ibizugbe @ 2023-10-11 15:24 UTC (permalink / raw)
To: git
In-Reply-To: <20231011152424.6957-1-isokenjune@gmail.com>
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..a756543d64 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch %s is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch %s is being bisected at %s"),
target, wt->path);
}
}
@@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (ref_exists(oldref.buf))
recovery = 1;
else
- die(_("Invalid branch name: '%s'"), oldname);
+ die(_("invalid branch name: '%s'"), oldname);
}
for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
if (oldref_usage & IS_HEAD)
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -624,9 +624,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && !(oldref_usage & IS_ORPHAN) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
@@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated!"), newname);
strbuf_release(&logmsg);
strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldref);
strbuf_release(&newref);
strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
@@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -892,7 +892,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
+ die(copy? _("cannot copy the current branch while not on any")
: _("cannot rename the current branch while not on any."));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
@@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
--
2.42.0.325.g3a06386e31
^ permalink raw reply related
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