* [PATCH v3 16/17] commit-graph: reuse existing Bloom filters where possible
From: Taylor Blau @ 2023-10-10 20:34 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <cover.1696969994.git.me@ttaylorr.com>
In 9e4df4da07 (commit-graph: new filter ver. that fixes murmur3,
2023-08-01), a bug was described where it's possible for Git to produce
non-murmur3 hashes when the platform's "char" type is signed, and there
are paths with characters whose highest bit is set (i.e. all characters
>= 0x80).
That patch allows the caller to control which version of Bloom filters
are read and written. However, even on platforms with a signed "char"
type, it is possible to reuse existing Bloom filters if and only if
there are no changed paths in any commit's first parent tree-diff whose
characters have their highest bit set.
When this is the case, we can reuse the existing filter without having
to compute a new one. This is done by marking trees which are known to
have (or not have) any such paths. When a commit's root tree is verified
to not have any such paths, we mark it as such and declare that the
commit's Bloom filter is reusable.
Note that this heuristic only goes in one direction. If neither a commit
nor its first parent have any paths in their trees with non-ASCII
characters, then we know for certain that a path with non-ASCII
characters will not appear in a tree-diff against that commit's first
parent. The reverse isn't necessarily true: just because the tree-diff
doesn't contain any such paths does not imply that no such paths exist
in either tree.
So we end up recomputing some Bloom filters that we don't strictly have
to (i.e. their bits are the same no matter which version of murmur3 we
use). But culling these out is impossible, since we'd have to perform
the full tree-diff, which is the same effort as computing the Bloom
filter from scratch.
But because we can cache our results in each tree's flag bits, we can
often avoid recomputing many filters, thereby reducing the time it takes
to run
$ git commit-graph write --changed-paths --reachable
when upgrading from v1 to v2 Bloom filters.
To benchmark this, let's generate a commit-graph in linux.git with v1
changed-paths in generation order[^1]:
$ git clone git@github.com:torvalds/linux.git
$ cd linux
$ git commit-graph write --reachable --changed-paths
$ graph=".git/objects/info/commit-graph"
$ mv $graph{,.bak}
Then let's time how long it takes to go from v1 to v2 filters (with and
without the upgrade path enabled), resetting the state of the
commit-graph each time:
$ git config commitGraph.changedPathsVersion 2
$ hyperfine -p 'cp -f $graph.bak $graph' -L v 0,1 \
'GIT_TEST_UPGRADE_BLOOM_FILTERS={v} git.compile commit-graph write --reachable --changed-paths'
On linux.git (where there aren't any non-ASCII paths), the timings
indicate that this patch represents a speed-up over recomputing all
Bloom filters from scratch:
Benchmark 1: GIT_TEST_UPGRADE_BLOOM_FILTERS=0 git.compile commit-graph write --reachable --changed-paths
Time (mean ± σ): 124.873 s ± 0.316 s [User: 124.081 s, System: 0.643 s]
Range (min … max): 124.621 s … 125.227 s 3 runs
Benchmark 2: GIT_TEST_UPGRADE_BLOOM_FILTERS=1 git.compile commit-graph write --reachable --changed-paths
Time (mean ± σ): 79.271 s ± 0.163 s [User: 74.611 s, System: 4.521 s]
Range (min … max): 79.112 s … 79.437 s 3 runs
Summary
'GIT_TEST_UPGRADE_BLOOM_FILTERS=1 git.compile commit-graph write --reachable --changed-paths' ran
1.58 ± 0.01 times faster than 'GIT_TEST_UPGRADE_BLOOM_FILTERS=0 git.compile commit-graph write --reachable --changed-paths'
On git.git, we do have some non-ASCII paths, giving us a more modest
improvement from 4.163 seconds to 3.348 seconds, for a 1.24x speed-up.
On my machine, the stats for git.git are:
- 8,285 Bloom filters computed from scratch
- 10 Bloom filters generated as empty
- 4 Bloom filters generated as truncated due to too many changed paths
- 65,114 Bloom filters were reused when transitioning from v1 to v2.
[^1]: Note that this is is important, since `--stdin-packs` or
`--stdin-commits` orders commits in the commit-graph by their pack
position (with `--stdin-packs`) or in the raw input (with
`--stdin-commits`).
Since we compute Bloom filters in the same order that commits appear
in the graph, we must see a commit's (first) parent before we process
the commit itself. This is only guaranteed to happen when sorting
commits by their generation number.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bloom.c | 90 ++++++++++++++++++++++++++++++++++++++++++--
bloom.h | 1 +
commit-graph.c | 5 +++
object.h | 1 +
t/t4216-log-bloom.sh | 35 ++++++++++++++++-
5 files changed, 127 insertions(+), 5 deletions(-)
diff --git a/bloom.c b/bloom.c
index 739fa093ba..24dd874e46 100644
--- a/bloom.c
+++ b/bloom.c
@@ -7,6 +7,9 @@
#include "commit-graph.h"
#include "commit.h"
#include "commit-slab.h"
+#include "tree.h"
+#include "tree-walk.h"
+#include "config.h"
define_commit_slab(bloom_filter_slab, struct bloom_filter);
@@ -250,6 +253,73 @@ static void init_truncated_large_filter(struct bloom_filter *filter,
filter->version = version;
}
+#define VISITED (1u<<21)
+#define HIGH_BITS (1u<<22)
+
+static int has_entries_with_high_bit(struct repository *r, struct tree *t)
+{
+ if (parse_tree(t))
+ return 1;
+
+ if (!(t->object.flags & VISITED)) {
+ struct tree_desc desc;
+ struct name_entry entry;
+
+ init_tree_desc(&desc, t->buffer, t->size);
+ while (tree_entry(&desc, &entry)) {
+ size_t i;
+ for (i = 0; i < entry.pathlen; i++) {
+ if (entry.path[i] & 0x80) {
+ t->object.flags |= HIGH_BITS;
+ goto done;
+ }
+ }
+
+ if (S_ISDIR(entry.mode)) {
+ struct tree *sub = lookup_tree(r, &entry.oid);
+ if (sub && has_entries_with_high_bit(r, sub)) {
+ t->object.flags |= HIGH_BITS;
+ goto done;
+ }
+ }
+
+ }
+
+done:
+ t->object.flags |= VISITED;
+ }
+
+ return !!(t->object.flags & HIGH_BITS);
+}
+
+static int commit_tree_has_high_bit_paths(struct repository *r,
+ struct commit *c)
+{
+ struct tree *t;
+ if (repo_parse_commit(r, c))
+ return 1;
+ t = repo_get_commit_tree(r, c);
+ if (!t)
+ return 1;
+ return has_entries_with_high_bit(r, t);
+}
+
+static struct bloom_filter *upgrade_filter(struct repository *r, struct commit *c,
+ struct bloom_filter *filter,
+ int hash_version)
+{
+ struct commit_list *p = c->parents;
+ if (commit_tree_has_high_bit_paths(r, c))
+ return NULL;
+
+ if (p && commit_tree_has_high_bit_paths(r, p->item))
+ return NULL;
+
+ filter->version = hash_version;
+
+ return filter;
+}
+
struct bloom_filter *get_bloom_filter(struct repository *r, struct commit *c)
{
struct bloom_filter *filter;
@@ -292,9 +362,23 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
filter, graph_pos);
}
- if ((filter->data && filter->len) &&
- (!settings || settings->hash_version == filter->version))
- return filter;
+ if (filter->data && filter->len) {
+ struct bloom_filter *upgrade;
+ if (!settings || settings->hash_version == filter->version)
+ return filter;
+
+ /* version mismatch, see if we can upgrade */
+ if (compute_if_not_present &&
+ git_env_bool("GIT_TEST_UPGRADE_BLOOM_FILTERS", 1)) {
+ upgrade = upgrade_filter(r, c, filter,
+ settings->hash_version);
+ if (upgrade) {
+ if (computed)
+ *computed |= BLOOM_UPGRADED;
+ return upgrade;
+ }
+ }
+ }
if (!compute_if_not_present)
return NULL;
diff --git a/bloom.h b/bloom.h
index bfe389e29c..e3a9b68905 100644
--- a/bloom.h
+++ b/bloom.h
@@ -102,6 +102,7 @@ enum bloom_filter_computed {
BLOOM_COMPUTED = (1 << 1),
BLOOM_TRUNC_LARGE = (1 << 2),
BLOOM_TRUNC_EMPTY = (1 << 3),
+ BLOOM_UPGRADED = (1 << 4),
};
struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
diff --git a/commit-graph.c b/commit-graph.c
index e0fc62e110..571f38335a 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1109,6 +1109,7 @@ struct write_commit_graph_context {
int count_bloom_filter_not_computed;
int count_bloom_filter_trunc_empty;
int count_bloom_filter_trunc_large;
+ int count_bloom_filter_upgraded;
};
static int write_graph_chunk_fanout(struct hashfile *f,
@@ -1716,6 +1717,8 @@ static void trace2_bloom_filter_write_statistics(struct write_commit_graph_conte
ctx->count_bloom_filter_trunc_empty);
trace2_data_intmax("commit-graph", ctx->r, "filter-trunc-large",
ctx->count_bloom_filter_trunc_large);
+ trace2_data_intmax("commit-graph", ctx->r, "filter-upgraded",
+ ctx->count_bloom_filter_upgraded);
}
static void compute_bloom_filters(struct write_commit_graph_context *ctx)
@@ -1757,6 +1760,8 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
ctx->count_bloom_filter_trunc_empty++;
if (computed & BLOOM_TRUNC_LARGE)
ctx->count_bloom_filter_trunc_large++;
+ } else if (computed & BLOOM_UPGRADED) {
+ ctx->count_bloom_filter_upgraded++;
} else if (computed & BLOOM_NOT_COMPUTED)
ctx->count_bloom_filter_not_computed++;
ctx->total_bloom_filter_data_size += filter
diff --git a/object.h b/object.h
index db25714b4e..2e5e08725f 100644
--- a/object.h
+++ b/object.h
@@ -75,6 +75,7 @@ void object_array_init(struct object_array *array);
* commit-reach.c: 16-----19
* sha1-name.c: 20
* list-objects-filter.c: 21
+ * bloom.c: 2122
* builtin/fsck.c: 0--3
* builtin/gc.c: 0
* builtin/index-pack.c: 2021
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 8f8b5d4966..a321d8d713 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -221,6 +221,10 @@ test_filter_trunc_large () {
grep "\"key\":\"filter-trunc-large\",\"value\":\"$1\"" $2
}
+test_filter_upgraded () {
+ grep "\"key\":\"filter-upgraded\",\"value\":\"$1\"" $2
+}
+
test_expect_success 'correctly report changes over limit' '
git init limits &&
(
@@ -629,10 +633,19 @@ test_expect_success 'when writing another commit graph, preserve existing versio
test_expect_success 'when writing commit graph, do not reuse changed-path of another version' '
git init doublewrite &&
test_commit -C doublewrite c "$CENT" &&
+
git -C doublewrite config --add commitgraph.changedPathsVersion 1 &&
- git -C doublewrite commit-graph write --reachable --changed-paths &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ test_filter_computed 1 trace2.txt &&
+ test_filter_upgraded 0 trace2.txt &&
+
git -C doublewrite config --add commitgraph.changedPathsVersion 2 &&
- git -C doublewrite commit-graph write --reachable --changed-paths &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C doublewrite commit-graph write --reachable --changed-paths &&
+ test_filter_computed 1 trace2.txt &&
+ test_filter_upgraded 0 trace2.txt &&
+
(
cd doublewrite &&
echo "c01f" >expect &&
@@ -641,4 +654,22 @@ test_expect_success 'when writing commit graph, do not reuse changed-path of ano
)
'
+test_expect_success 'when writing commit graph, reuse changed-path of another version where possible' '
+ git init upgrade &&
+
+ test_commit -C upgrade base no-high-bits &&
+
+ git -C upgrade config --add commitgraph.changedPathsVersion 1 &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C upgrade commit-graph write --reachable --changed-paths &&
+ test_filter_computed 1 trace2.txt &&
+ test_filter_upgraded 0 trace2.txt &&
+
+ git -C upgrade config --add commitgraph.changedPathsVersion 2 &&
+ GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+ git -C upgrade commit-graph write --reachable --changed-paths &&
+ test_filter_computed 0 trace2.txt &&
+ test_filter_upgraded 1 trace2.txt
+'
+
test_done
--
2.42.0.342.g8bb3a896ee
^ permalink raw reply related
* [PATCH v3 17/17] bloom: introduce `deinit_bloom_filters()`
From: Taylor Blau @ 2023-10-10 20:34 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Junio C Hamano, Jeff King, SZEDER Gábor
In-Reply-To: <cover.1696969994.git.me@ttaylorr.com>
After we are done using Bloom filters, we do not currently clean up any
memory allocated by the commit slab used to store those filters in the
first place.
Besides the bloom_filter structures themselves, there is mostly nothing
to free() in the first place, since in the read-only path all Bloom
filter's `data` members point to a memory mapped region in the
commit-graph file itself.
But when generating Bloom filters from scratch (or initializing
truncated filters) we allocate additional memory to store the filter's
data.
Keep track of when we need to free() this additional chunk of memory by
using an extra pointer `to_free`. Most of the time this will be NULL
(indicating that we are representing an existing Bloom filter stored in
a memory mapped region). When it is non-NULL, free it before discarding
the Bloom filters slab.
Suggested-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bloom.c | 16 +++++++++++++++-
bloom.h | 3 +++
commit-graph.c | 4 ++++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/bloom.c b/bloom.c
index 24dd874e46..ff131893cd 100644
--- a/bloom.c
+++ b/bloom.c
@@ -59,6 +59,7 @@ int load_bloom_filter_from_graph(struct commit_graph *g,
sizeof(unsigned char) * start_index +
BLOOMDATA_CHUNK_HEADER_SIZE);
filter->version = g->bloom_filter_settings->hash_version;
+ filter->to_free = NULL;
return 1;
}
@@ -231,6 +232,18 @@ void init_bloom_filters(void)
init_bloom_filter_slab(&bloom_filters);
}
+static void free_one_bloom_filter(struct bloom_filter *filter)
+{
+ if (!filter)
+ return;
+ free(filter->to_free);
+}
+
+void deinit_bloom_filters(void)
+{
+ deep_clear_bloom_filter_slab(&bloom_filters, free_one_bloom_filter);
+}
+
static int pathmap_cmp(const void *hashmap_cmp_fn_data UNUSED,
const struct hashmap_entry *eptr,
const struct hashmap_entry *entry_or_key,
@@ -247,7 +260,7 @@ static int pathmap_cmp(const void *hashmap_cmp_fn_data UNUSED,
static void init_truncated_large_filter(struct bloom_filter *filter,
int version)
{
- filter->data = xmalloc(1);
+ filter->data = filter->to_free = xmalloc(1);
filter->data[0] = 0xFF;
filter->len = 1;
filter->version = version;
@@ -449,6 +462,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
filter->len = 1;
}
CALLOC_ARRAY(filter->data, filter->len);
+ filter->to_free = filter->data;
hashmap_for_each_entry(&pathmap, &iter, e, entry) {
struct bloom_key key;
diff --git a/bloom.h b/bloom.h
index e3a9b68905..d20e64bfbb 100644
--- a/bloom.h
+++ b/bloom.h
@@ -56,6 +56,8 @@ struct bloom_filter {
unsigned char *data;
size_t len;
int version;
+
+ void *to_free;
};
/*
@@ -96,6 +98,7 @@ void add_key_to_filter(const struct bloom_key *key,
const struct bloom_filter_settings *settings);
void init_bloom_filters(void);
+void deinit_bloom_filters(void);
enum bloom_filter_computed {
BLOOM_NOT_COMPUTED = (1 << 0),
diff --git a/commit-graph.c b/commit-graph.c
index 571f38335a..7ccec429cb 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -787,6 +787,7 @@ static void close_commit_graph_one(struct commit_graph *g)
void close_commit_graph(struct raw_object_store *o)
{
close_commit_graph_one(o->commit_graph);
+ deinit_bloom_filters();
o->commit_graph = NULL;
}
@@ -2588,6 +2589,9 @@ int write_commit_graph(struct object_directory *odb,
res = write_commit_graph_file(ctx);
+ if (ctx->changed_paths)
+ deinit_bloom_filters();
+
if (ctx->split)
mark_commit_graphs(ctx);
--
2.42.0.342.g8bb3a896ee
^ permalink raw reply related
* Re: [PATCH 07/15] commit-graph: new filter ver. that fixes murmur3
From: Taylor Blau @ 2023-10-10 20:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: SZEDER Gábor, Jonathan Tan, git, Jeff King
In-Reply-To: <xmqqr0m3y3o2.fsf@gitster.g>
On Mon, Oct 09, 2023 at 12:52:13PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > However, I am not entirely sure I agree with you that this is a "new"
> > issue. At least in the sense that Git (on 'master') does not currently
> > know how to deal with Bloom filters that have different settings across
> > commit-graph layers.
> >
> > IOW, you could produce this problem today using the test you wrote in
> > <20201015132147.GB24954@szeder.dev> using different values of the
> > GIT_BLOOM_SETTINGS environment variables as a proxy for different values
> > of the commitGraph.changedPathsVersion configuration variable introduced
> > in this series.
> >
> > So I think that this series makes it easier to fall into that trap, but
> > the trap itself is not new. I think a reasonable stopgap (which IIUC you
> > have suggested earlier) is to prevent writing a new commit-graph layer
> > with a different hash version than the previous layer.
>
> What we probably want more urgently than that stopgap is to perhaps
> teach the code pretend as if commit-graph did not exist when we
> detect multiple layers use different hash versions (or perhaps only
> use the base layer and ignore the rest as an anti-pessimization), to
> protect correctness first, no?
Very good suggestion, thanks.
Thanks,
Taylor
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: Taylor Blau @ 2023-10-10 20:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh6n24zf1.fsf@gitster.g>
On Sat, Oct 07, 2023 at 01:20:02AM -0700, Junio C Hamano wrote:
> * 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.
>
> What's the status of this thing?
> cf. <20230830200218.GA5147@szeder.dev>
> cf. <20230901205616.3572722-1-jonathantanmy@google.com>
> cf. <20230924195900.GA1156862@szeder.dev>
> source: <cover.1693413637.git.jonathantanmy@google.com>
Good question ;-). There were a couple of outstanding issues pointed out
by SZEDER Gábor that I addressed on top of what's already there.
I tied up what I think are all of the remaining loose ends and put
everything together in a single unified (if a little long) patch series.
This should be ready to go, but I'd love to hear from Jonathan Tan and
SZEDER before declaring victory here.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v4 4/4] parse: separate out parsing functions from config.h
From: Junio C Hamano @ 2023-10-10 20:57 UTC (permalink / raw)
To: Phillip Wood; +Cc: Jonathan Tan, git, Calvin Wan
In-Reply-To: <87c16c09-784f-4818-9231-8023fc2c5a02@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>> I'm open to changing
>> git_config_get_expiry_date_in_days() too, though...we probably don't
>> need so many days.
>
> Indeed, the existing code passes maximum_signed_value_of_type(int) as
> the third argument to limit it to INT_MAX already.
Yeah, in other words, the current implementation does not allow us
to express the days more than an int can, so it is no brainer to
switch to use git_parse_int(). Allowing longer expiration period
is obviously outside the scope of this series.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from
From: Junio C Hamano @ 2023-10-10 22:14 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, Jeff King, Jonathan Tan, John Cai
In-Reply-To: <dadb822da99772cd277417f564cf672f65d1cc24.1696967380.git.gitgitgadget@gmail.com>
"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.
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).
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.
> 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.
> +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.
> @@ -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.
> + 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.
^ permalink raw reply
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Junio C Hamano @ 2023-10-10 22:24 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2yUGGJwD05KOFZK+AV3TSNDvDEfC=pFRsLwKX_-dgt+gA@mail.gmail.com>
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: [PATCH 01/20] chunk-format: note that pair_chunk() is unsafe
From: Taylor Blau @ 2023-10-10 23:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231009205823.GA3282181@coredump.intra.peff.net>
On Mon, Oct 09, 2023 at 04:58:23PM -0400, Jeff King wrote:
> There are no callers of the "safe" version yet, but we'll add some in
> subsequent patches.
Makes sense.
> +int pair_chunk_unsafe(struct chunkfile *cf,
> + uint32_t chunk_id,
> + const unsigned char **p)
> {
> - return read_chunk(cf, chunk_id, pair_chunk_fn, p);
> + size_t dummy;
> + return pair_chunk(cf, chunk_id, p, &dummy);
I have always disliked functions that require you to pass a non-NULL
pointer to some value that you may or may not want to have that function
fill out. So I was going to suggest something along the lines of
"pair_chunk() should tolerate a NULL fourth argument instead of filling
out the size unconditionally".
But that's (a) the whole point of the series ;-), and (b) unnecessary,
since this function is going to go away entirely by the end of the
series, too.
So I think that the call you made here was the right one. The rest of
the patch all looks good to me, let's read on...
Thanks,
Taylor
^ permalink raw reply
* 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
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