* [PATCH] t/t7601: Modernize test scripts using functions
From: Dorcas AnonoLitunya @ 2023-10-16 15:21 UTC (permalink / raw)
To: christian.couder; +Cc: anonolitunya, git, gitster
The test script is currently using the command format 'test -f' to
check for existence or absence of files.
Replace it with new helper functions following the format
'test_path_is_file'.
Consequently, the patch also replaces the inverse command '! test -f' or
'test ! -f' with new helper function following the format
'test_path_is_missing'
This adjustment using helper functions makes the code more readable and
easier to understand.
Signed-off-by: Dorcas AnonoLitunya <anonolitunya@gmail.com>
---
t/t7601-merge-pull-config.sh | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index bd238d89b0..e08767df66 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' '
test_expect_success 'merge c1 with c2' '
git reset --hard c1 &&
- test -f c0.c &&
- test -f c1.c &&
- test ! -f c2.c &&
- test ! -f c3.c &&
+ test_path_is_file c0.c &&
+ test_path_is_file c1.c &&
+ test_path_is_missing c2.c &&
+ test_path_is_missing c3.c &&
git merge c2 &&
- test -f c1.c &&
- test -f c2.c
+ test_path_is_file c1.c &&
+ test_path_is_file c2.c
'
test_expect_success 'fast-forward pull succeeds with "true" in pull.ff' '
@@ -411,8 +411,8 @@ test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
git reset --hard c1 &&
git config pull.twohead ours &&
git merge c2 &&
- test -f c1.c &&
- ! test -f c2.c
+ test_path_is_file c1.c &&
+ test_path_is_missing c2.c
'
test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' '
@@ -431,10 +431,10 @@ test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octo
test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
test "$(git rev-parse c3)" = "$(git rev-parse HEAD^3)" &&
git diff --exit-code &&
- test -f c0.c &&
- test -f c1.c &&
- test -f c2.c &&
- test -f c3.c
+ test_path_is_file c0.c &&
+ test_path_is_file c1.c &&
+ test_path_is_file c2.c &&
+ test_path_is_file c3.c
'
conflict_count()
--
2.42.0.345.gaab89be2eb
^ permalink raw reply related
* Method for Calculating Statistics of Developer Contribution to a Specified Branch.
From: Hongyi Zhao @ 2023-10-16 14:10 UTC (permalink / raw)
To: Git List
Dear Git Mailing List,
I am a developer currently working on a project and I wanted to
establish statistics for each team member's contribution to a specific
branch.
Say, for a user "JianboLin", I am currently using the following method:
$ git clone https://github.com/OrderN/CONQUEST-release.git
$ cd CONQUEST-release
$ git log --author="JianboLin" --stat --summary origin/f-mlff | awk
'NF ==4 && $2 =="|" && $3 ~/[0-9]+/ && $4 ~/[+-]+|[+]+|[-]+/ {s+=$3}
END {print s}'
Using the above command, I am able to calculate the number of lines
contributed by a specific author on a specific branch, which allows me
to quantify the contribution to a branch by each team member.
However, I would like to know if a more efficient or accurate method
exists to carry out this task. Are there any other parameters,
commands, or aspects I need to consider to get a more comprehensive
measure of contribution?
I greatly appreciate your expertise and look forward to your valuable input.
Best Regards,
Zhao
--
Assoc. Prof. Hongsheng Zhao <hongyi.zhao@gmail.com>
Theory and Simulation of Materials
Hebei Vocational University of Technology and Engineering
No. 473, Quannan West Street, Xindu District, Xingtai, Hebei province
^ permalink raw reply
* [PATCH v8 2.5/3] fixup! unit tests: add TAP unit test framework
From: Phillip Wood @ 2023-10-16 13:43 UTC (permalink / raw)
To: steadmon; +Cc: calvinwan, git, gitster, linusa, phillip.wood123, rsbecker
In-Reply-To: <00d3c95a81449bf49c4ce992d862d7a858691840.1696889530.git.steadmon@google.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Here are a couple of cleanups for the unit test framework that I
noticed.
Update the documentation of the example custom check to reflect the
change in return value of test_assert() and mention that
checks should be careful when dereferencing pointer arguments.
Also avoid evaluating macro augments twice in check_int() and
friends. The global variable test__tmp was introduced to avoid
evaluating the arguments to these macros more than once but the macros
failed to use it when passing the values being compared to
check_int_loc().
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
t/unit-tests/test-lib.h | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/t/unit-tests/test-lib.h b/t/unit-tests/test-lib.h
index 8df3804914..a8f07ae0b7 100644
--- a/t/unit-tests/test-lib.h
+++ b/t/unit-tests/test-lib.h
@@ -42,18 +42,21 @@ void test_msg(const char *format, ...);
/*
* Test checks are built around test_assert(). checks return 1 on
- * success, 0 on failure. If any check fails then the test will
- * fail. To create a custom check define a function that wraps
- * test_assert() and a macro to wrap that function. For example:
+ * success, 0 on failure. If any check fails then the test will fail. To
+ * create a custom check define a function that wraps test_assert() and
+ * a macro to wrap that function to provide a source location and
+ * stringified arguments. Custom checks that take pointer arguments
+ * should be careful to check that they are non-NULL before
+ * dereferencing them. For example:
*
* static int check_oid_loc(const char *loc, const char *check,
* struct object_id *a, struct object_id *b)
* {
- * int res = test_assert(loc, check, oideq(a, b));
+ * int res = test_assert(loc, check, a && b && oideq(a, b));
*
- * if (res) {
- * test_msg(" left: %s", oid_to_hex(a);
- * test_msg(" right: %s", oid_to_hex(a);
+ * if (!res) {
+ * test_msg(" left: %s", a ? oid_to_hex(a) : "NULL";
+ * test_msg(" right: %s", b ? oid_to_hex(a) : "NULL";
*
* }
* return res;
@@ -79,7 +82,8 @@ int check_bool_loc(const char *loc, const char *check, int ok);
#define check_int(a, op, b) \
(test__tmp[0].i = (a), test__tmp[1].i = (b), \
check_int_loc(TEST_LOCATION(), #a" "#op" "#b, \
- test__tmp[0].i op test__tmp[1].i, a, b))
+ test__tmp[0].i op test__tmp[1].i, \
+ test__tmp[0].i, test__tmp[1].i))
int check_int_loc(const char *loc, const char *check, int ok,
intmax_t a, intmax_t b);
@@ -90,7 +94,8 @@ int check_int_loc(const char *loc, const char *check, int ok,
#define check_uint(a, op, b) \
(test__tmp[0].u = (a), test__tmp[1].u = (b), \
check_uint_loc(TEST_LOCATION(), #a" "#op" "#b, \
- test__tmp[0].u op test__tmp[1].u, a, b))
+ test__tmp[0].u op test__tmp[1].u, \
+ test__tmp[0].u, test__tmp[1].u))
int check_uint_loc(const char *loc, const char *check, int ok,
uintmax_t a, uintmax_t b);
@@ -101,7 +106,8 @@ int check_uint_loc(const char *loc, const char *check, int ok,
#define check_char(a, op, b) \
(test__tmp[0].c = (a), test__tmp[1].c = (b), \
check_char_loc(TEST_LOCATION(), #a" "#op" "#b, \
- test__tmp[0].c op test__tmp[1].c, a, b))
+ test__tmp[0].c op test__tmp[1].c, \
+ test__tmp[0].c, test__tmp[1].c))
int check_char_loc(const char *loc, const char *check, int ok,
char a, char b);
--
2.42.0.506.g0dd4464cfd3
^ permalink raw reply related
* [PATCH v2 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-16 10:38 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231016103830.56486-1-karthik.188@gmail.com>
The `--missing` object option in rev-list currently works only with
missing blobs/trees. For missing commits the revision walker fails with
a fatal error.
Let's extend the functionality of `--missing` option to also support
commit objects. This is done by adding a new `MISSING` flag that the
revision walker sets whenever it encounters a missing tree/commit. The
revision walker will now continue the traversal and call `show_commit()`
even for missing commits. In rev-list we can then check for this flag
and call the existing code for parsing `--missing` objects.
A scenario where this option would be used is to find the boundary
objects between different object directories. Consider a repository with
a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
repository, using the `--missing=print` option while disabling the
alternate object directory allows us to find the boundary objects
between the main and alternate object directory.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 6 +++
list-objects.c | 2 +-
object.h | 2 +-
revision.c | 11 ++++--
revision.h | 3 ++
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++
6 files changed, 93 insertions(+), 5 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 98542e8b3c..604ae01d0c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)
display_progress(progress, ++progress_counter);
+ if (revs->do_not_die_on_missing_objects &&
+ commit->object.flags & MISSING) {
+ finish_object__ma(&commit->object);
+ return;
+ }
+
if (show_disk_usage)
total_disk_usage += get_object_disk_usage(&commit->object);
diff --git a/list-objects.c b/list-objects.c
index 47296dff2f..60c5533721 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -387,7 +387,7 @@ static void do_traverse(struct traversal_context *ctx)
* an uninteresting boundary commit may not have its tree
* parsed yet, but we are not going to show them anyway
*/
- if (!ctx->revs->tree_objects)
+ if (!ctx->revs->tree_objects || commit->object.flags & MISSING)
; /* do not bother loading tree */
else if (repo_get_commit_tree(the_repository, commit)) {
struct tree *tree = repo_get_commit_tree(the_repository,
diff --git a/object.h b/object.h
index 114d45954d..12913e6116 100644
--- a/object.h
+++ b/object.h
@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);
/*
* object flag allocation:
- * revision.h: 0---------10 15 23------27
+ * revision.h: 0---------10 15 22------27
* fetch-pack.c: 01 67
* negotiator/default.c: 2--5
* walker.c: 0-2
diff --git a/revision.c b/revision.c
index e789834dd1..d147e0f611 100644
--- a/revision.c
+++ b/revision.c
@@ -1110,7 +1110,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
struct commit_list *parent = commit->parents;
unsigned pass_flags;
- if (commit->object.flags & ADDED)
+ if (commit->object.flags & ADDED || commit->object.flags & MISSING)
return 0;
commit->object.flags |= ADDED;
@@ -1168,7 +1168,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
for (parent = commit->parents; parent; parent = parent->next) {
struct commit *p = parent->item;
int gently = revs->ignore_missing_links ||
- revs->exclude_promisor_objects;
+ revs->exclude_promisor_objects ||
+ revs->do_not_die_on_missing_objects;
if (repo_parse_commit_gently(revs->repo, p, gently) < 0) {
if (revs->exclude_promisor_objects &&
is_promisor_object(&p->object.oid)) {
@@ -1176,7 +1177,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
break;
continue;
}
- return -1;
+
+ if (!revs->do_not_die_on_missing_objects)
+ return -1;
+ else
+ p->object.flags |= MISSING;
}
if (revs->sources) {
char **slot = revision_sources_at(revs->sources, p);
diff --git a/revision.h b/revision.h
index c73c92ef40..07906bc3bc 100644
--- a/revision.h
+++ b/revision.h
@@ -41,6 +41,9 @@
/* WARNING: This is also used as REACHABLE in commit-graph.c. */
#define PULL_MERGE (1u<<15)
+/* WARNING: This is also used as NEEDS_BITMAP in pack-bitmap.c. */
+#define MISSING (1u<<22)
+
#define TOPO_WALK_EXPLORED (1u<<23)
#define TOPO_WALK_INDEGREE (1u<<24)
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
new file mode 100755
index 0000000000..40265a4f66
--- /dev/null
+++ b/t/t6022-rev-list-missing.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='handling of missing objects in rev-list'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# 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' '
+ test_commit 1 &&
+ test_commit 2 &&
+ test_commit 3
+'
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ test_expect_success "rev-list --missing=error fails with missing object $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ test_must_fail git rev-list --missing=error --objects \
+ --no-object-names HEAD
+ '
+done
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ for action in "allow-any" "print"
+ do
+ test_expect_success "rev-list --missing=$action with missing $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ # Before the object is made missing, we use rev-list to
+ # get the expected oids.
+ git rev-list --objects --no-object-names \
+ HEAD ^$obj >expect.raw &&
+
+ # Blobs are shared by all commits, so evethough a commit/tree
+ # might be skipped, its blob must be accounted for.
+ if [ $obj != "HEAD:1.t" ]; then
+ echo $(git rev-parse HEAD:1.t) >>expect.raw &&
+ echo $(git rev-parse HEAD:2.t) >>expect.raw
+ fi &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ git rev-list --missing=$action --objects --no-object-names \
+ HEAD >actual.raw &&
+
+ # When the action is to print, we should also add the missing
+ # oid to the expect list.
+ case $action in
+ allow-any)
+ ;;
+ print)
+ grep ?$oid actual.raw &&
+ echo ?$oid >>expect.raw
+ ;;
+ esac &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+ '
+ done
+done
+
+test_done
--
2.41.0
^ permalink raw reply related
* [PATCH v2 2/3] rev-list: move `show_commit()` to the bottom
From: Karthik Nayak @ 2023-10-16 10:38 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231016103830.56486-1-karthik.188@gmail.com>
The `show_commit()` function already depends on `finish_commit()`, and
in the upcoming commit, we'll also add a dependency on
`finish_object__ma()`. Since in C symbols must be declared before
they're used, let's move `show_commit()` below both `finish_commit()`
and `finish_object__ma()`, so the code is cleaner as a whole without the
need for declarations.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ea77489c38..98542e8b3c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)
return size;
}
-static void finish_commit(struct commit *commit);
+static inline void finish_object__ma(struct object *obj)
+{
+ /*
+ * Whether or not we try to dynamically fetch missing objects
+ * from the server, we currently DO NOT have the object. We
+ * can either print, allow (ignore), or conditionally allow
+ * (ignore) them.
+ */
+ switch (arg_missing_action) {
+ case MA_ERROR:
+ die("missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ case MA_ALLOW_ANY:
+ return;
+
+ case MA_PRINT:
+ oidset_insert(&missing_objects, &obj->oid);
+ return;
+
+ case MA_ALLOW_PROMISOR:
+ if (is_promisor_object(&obj->oid))
+ return;
+ die("unexpected missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ default:
+ BUG("unhandled missing_action");
+ return;
+ }
+}
+
+static void finish_commit(struct commit *commit)
+{
+ free_commit_list(commit->parents);
+ commit->parents = NULL;
+ free_commit_buffer(the_repository->parsed_objects,
+ commit);
+}
+
static void show_commit(struct commit *commit, void *data)
{
struct rev_list_info *info = data;
@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)
finish_commit(commit);
}
-static void finish_commit(struct commit *commit)
-{
- free_commit_list(commit->parents);
- commit->parents = NULL;
- free_commit_buffer(the_repository->parsed_objects,
- commit);
-}
-
-static inline void finish_object__ma(struct object *obj)
-{
- /*
- * Whether or not we try to dynamically fetch missing objects
- * from the server, we currently DO NOT have the object. We
- * can either print, allow (ignore), or conditionally allow
- * (ignore) them.
- */
- switch (arg_missing_action) {
- case MA_ERROR:
- die("missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- case MA_ALLOW_ANY:
- return;
-
- case MA_PRINT:
- oidset_insert(&missing_objects, &obj->oid);
- return;
-
- case MA_ALLOW_PROMISOR:
- if (is_promisor_object(&obj->oid))
- return;
- die("unexpected missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- default:
- BUG("unhandled missing_action");
- return;
- }
-}
-
static int finish_object(struct object *obj, const char *name UNUSED,
void *cb_data)
{
--
2.41.0
^ permalink raw reply related
* [PATCH v2 1/3] revision: rename bit to `do_not_die_on_missing_objects`
From: Karthik Nayak @ 2023-10-16 10:38 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231016103830.56486-1-karthik.188@gmail.com>
The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the
revision walker does not die when encountering a missing tree. This is
currently exclusively set within `builtin/rev-list.c` to ensure the
`--missing` option works with missing trees.
In the upcoming commits, we will extend `--missing` to also support
missing commits. So let's rename the bit to
`do_not_die_on_missing_objects`, which is object type agnostic and can
be used for both trees/commits.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/reflog.c | 2 +-
builtin/rev-list.c | 2 +-
list-objects.c | 2 +-
revision.h | 17 +++++++++--------
4 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index df63a5892e..9e369a5977 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
struct rev_info revs;
repo_init_revisions(the_repository, &revs, prefix);
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
revs.ignore_missing = 1;
revs.ignore_missing_links = 1;
if (verbose)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff715d6918..ea77489c38 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
}
if (arg_missing_action)
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
argc = setup_revisions(argc, argv, &revs, &s_r_opt);
diff --git a/list-objects.c b/list-objects.c
index c25c72b32c..47296dff2f 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,
is_promisor_object(&obj->oid))
return;
- if (!revs->do_not_die_on_missing_tree)
+ if (!revs->do_not_die_on_missing_objects)
die("bad tree object %s", oid_to_hex(&obj->oid));
}
diff --git a/revision.h b/revision.h
index 50091bbd13..c73c92ef40 100644
--- a/revision.h
+++ b/revision.h
@@ -212,18 +212,19 @@ struct rev_info {
/*
* Blobs are shown without regard for their existence.
- * But not so for trees: unless exclude_promisor_objects
+ * But not so for trees/commits: unless exclude_promisor_objects
* is set and the tree in question is a promisor object;
* OR ignore_missing_links is set, the revision walker
- * dies with a "bad tree object HASH" message when
- * encountering a missing tree. For callers that can
- * handle missing trees and want them to be filterable
+ * dies with a "bad <type> object HASH" message when
+ * encountering a missing object. For callers that can
+ * handle missing trees/commits and want them to be filterable
* and showable, set this to true. The revision walker
- * will filter and show such a missing tree as usual,
- * but will not attempt to recurse into this tree
- * object.
+ * will filter and show such a missing object as usual,
+ * but will not attempt to recurse into this tree/commit
+ * object. The revision walker will also set the MISSING
+ * flag for such objects.
*/
- do_not_die_on_missing_tree:1,
+ do_not_die_on_missing_objects:1,
/* for internal use only */
exclude_promisor_objects:1;
--
2.41.0
^ permalink raw reply related
* [PATCH v2 0/3] rev-list: add support for commits in `--missing`
From: Karthik Nayak @ 2023-10-16 10:38 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231009105528.17777-1-karthik.188@gmail.com>
The `--missing` option in git-rev-list(1) was introduced intitally
to deal with missing blobs in the context of promissory notes.
Eventually the option was extended to also support tree objects in
7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).
This patch series extends the `--missing` option to also add support for
commit objects. We do this by introducing a new flag `MISSING` which is
added whenever we encounter a missing commit during traversal. Then in
`builtin/rev-list` we check for this flag and take the appropriate
action based on the `--missing=*` option used.
This series is an alternate to the patch series I had posted earlier:
https://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.
In that patch, we introduced an option `--ignore-missing-links` which
was added to expose the `ignore_missing_links` bit to the user. The
issue in that patch was that, the option `--ignore-missing-links` didn't
play well the pre-existing `--missing` option. This series avoids that
route and just extends the `--missing` option for commits to solve the
same problem.
Changes from v1:
1. The patch series is now rebased on top of Patrick's work to make
commit-graphs more reliable. With this, we need to be more diligant
around which commits are missing and only parse required commits. So
we add some extra checks here, especially because we don't want to
allow missing commit's tree and parent to be parsed.
2. The tests were preivously testing a commit with no parents, add
an additional commit to ensure that the missing commit's parents aren't
parsed.
Range diff:
1: be2ac0a331 = 1: c1c892aa86 revision: rename bit to `do_not_die_on_missing_objects`
2: b44983967f = 2: 67f6bfeaf7 rev-list: move `show_commit()` to the bottom
3: 306d918aef ! 3: 6d8e3c721f rev-list: add commit object support in `--missing` option
@@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)
total_disk_usage += get_object_disk_usage(&commit->object);
+ ## list-objects.c ##
+@@ list-objects.c: static void do_traverse(struct traversal_context *ctx)
+ * an uninteresting boundary commit may not have its tree
+ * parsed yet, but we are not going to show them anyway
+ */
+- if (!ctx->revs->tree_objects)
++ if (!ctx->revs->tree_objects || commit->object.flags & MISSING)
+ ; /* do not bother loading tree */
+ else if (repo_get_commit_tree(the_repository, commit)) {
+ struct tree *tree = repo_get_commit_tree(the_repository,
+
## object.h ##
@@ object.h: void object_array_init(struct object_array *array);
@@ object.h: void object_array_init(struct object_array *array);
* walker.c: 0-2
## revision.c ##
+@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,
+ struct commit_list *parent = commit->parents;
+ unsigned pass_flags;
+
+- if (commit->object.flags & ADDED)
++ if (commit->object.flags & ADDED || commit->object.flags & MISSING)
+ return 0;
+ commit->object.flags |= ADDED;
+
@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,
for (parent = commit->parents; parent; parent = parent->next) {
struct commit *p = parent->item;
@@ t/t6022-rev-list-missing.sh (new)
+# available and we can hide commit 1.
+test_expect_success 'create repository and alternate directory' '
+ test_commit 1 &&
-+ test_commit 2
++ test_commit 2 &&
++ test_commit 3
+'
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
@@ t/t6022-rev-list-missing.sh (new)
+ git rev-list --objects --no-object-names \
+ HEAD ^$obj >expect.raw &&
+
-+ # Since both the commits have the `1.t` blob, rev-list
-+ # will print it since its reachable from either commit. Unless
-+ # the blob itself is missing.
++ # Blobs are shared by all commits, so evethough a commit/tree
++ # might be skipped, its blob must be accounted for.
+ if [ $obj != "HEAD:1.t" ]; then
-+ echo $(git rev-parse HEAD:1.t) >>expect.raw
++ echo $(git rev-parse HEAD:1.t) >>expect.raw &&
++ echo $(git rev-parse HEAD:2.t) >>expect.raw
+ fi &&
+
+ mv "$path" "$path.hidden" &&
@@ t/t6022-rev-list-missing.sh (new)
+ done
+done
+
-+
+test_done
Karthik Nayak (3):
revision: rename bit to `do_not_die_on_missing_objects`
rev-list: move `show_commit()` to the bottom
rev-list: add commit object support in `--missing` option
builtin/reflog.c | 2 +-
builtin/rev-list.c | 93 +++++++++++++++++++------------------
list-objects.c | 4 +-
object.h | 2 +-
revision.c | 11 +++--
revision.h | 20 ++++----
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++
7 files changed, 147 insertions(+), 59 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
--
2.41.0
^ permalink raw reply
* Re: git log --patch for a particular file
From: Kristoffer Haugsbakk @ 2023-10-16 10:29 UTC (permalink / raw)
To: Victor Porton; +Cc: git
In-Reply-To: <75545e1c-ce62-4b49-983e-1e7b1afb2fab@gmail.com>
Hi
On Mon, Oct 16, 2023, at 12:20, Victor Porton wrote:
> I want this:
>
> git log --patch -- server/src/api/docs.py
>
> to print changes only for the file server/src/api/docs.py.
That's how it works for me.
> Currently, it in some reasons outputs nothing (this seems not to be an
> intended behavior and could be counted as a bug).
Does it output nothing on *merge* commits? That's intended.
Try using `-m` as well.
https://stackoverflow.com/a/37801468/1725151
> It is questionable, whether to output commit information (commit hash,
> user email, etc.) for commits that don't change specified file(s). I
> would vote to indeed output this information, not to confuse the user
> about the order of commits.
You can customize the output with `--format=`.
> More generally,
> git log --patch -- A B C
> could log changes in files A, B, C (only).
If I name two files I only get commits that touch any of those files. And
diffs only for them.
--
Kristoffer Haugsbakk
^ permalink raw reply
* git log --patch for a particular file
From: Victor Porton @ 2023-10-16 10:20 UTC (permalink / raw)
To: git
I want this:
git log --patch -- server/src/api/docs.py
to print changes only for the file server/src/api/docs.py.
Currently, it in some reasons outputs nothing (this seems not to be an
intended behavior and could be counted as a bug).
More generally,
git log --patch -- A B C
could log changes in files A, B, C (only).
It is questionable, whether to output commit information (commit hash,
user email, etc.) for commits that don't change specified file(s). I
would vote to indeed output this information, not to confuse the user
about the order of commits.
^ permalink raw reply
* Re: [PATCH v8 0/3] Add unit test framework and project plan
From: phillip.wood123 @ 2023-10-16 10:07 UTC (permalink / raw)
To: Josh Steadmon, git; +Cc: linusa, calvinwan, gitster, rsbecker
In-Reply-To: <cover.1696889529.git.steadmon@google.com>
Hi Josh
Thanks for the update
On 09/10/2023 23:21, Josh Steadmon wrote:
> In addition to reviewing the patches in this series, reviewers can help
> this series progress by chiming in on these remaining TODOs:
> - Figure out if we should split t-basic.c into multiple meta-tests, to
> avoid merge conflicts and changes to expected text in
> t0080-unit-test-output.sh.
I think it depends on how many new tests we think we're going to want to
add here. I can see us adding a few more check_* macros (comparing
object ids and arrays of bytes spring to mind) and wanting to test them
here, but (perhaps naïvely) I don't expect huge amounts of churn here.
> - Figure out if we should de-duplicate assertions in t-strbuf.c at the
> cost of making tests less self-contained and diagnostic output less
> helpful.
In principle we could pass the location information along to any helper
function, I'm not sure how easy that is at the moment. We can get
reasonable error messages by using the check*() macros in the helper and
wrapping the call to the helper with check() as well. For example
static int assert_sane_strbuf(struct strbuf *buf)
{
/* Initialized strbufs should always have a non-NULL buffer */
if (!check(!!buf->buf))
return 0;
/* Buffers should always be NUL-terminated */
if (!check_char(buf->buf[buf->len], ==, '\0'))
return 0;
/*
* Freshly-initialized strbufs may not have a dynamically allocated
* buffer
*/
if (buf->len == 0 && buf->alloc == 0)
return 1;
/* alloc must be at least one byte larger than len */
return check_uint(buf->len, <, buf->alloc);
}
and in the test function call it as
check(assert_sane_strbuf(buf));
which gives error messages like
# check "buf->len < buf->alloc" failed at t/unit-tests/t-strbuf.c:43
# left: 5
# right: 0
# check "assert_sane_strbuf(&buf)" failed at t/unit-tests/t-strbuf.c:60
So we can see where assert_sane_strbuf() was called and which assertion
in assert_sane_strbuf() failed.
> - Figure out if we should collect unit tests statistics similar to the
> "counts" files for shell tests
Unless someone has an immediate need for that I'd be tempted to leave it
wait until someone requests that data.
> - Decide if it's OK to wait on sharding unit tests across "sliced" CI
> instances
Hopefully the unit tests will run fast enough that we don't need to
worry about that in the early stages.
> - Provide guidelines for writing new unit tests
This is not a comprehensive list but we should recommend that
- tests avoid leaking resources so the leak sanitizer see if the code
being tested has a resource leak.
- tests check that pointers are not NULL before deferencing them to
avoid the whole program being taken down with SIGSEGV.
- tests are written with easy debugging in mind - i.e. good diagnostic
messages. Hopefully the check* macros make that easy to do.
> Changes in v8:
> - Flipped return values for TEST, TEST_TODO, and check_* macros &
> functions. This makes it easier to reason about control flow for
> patterns like:
> if (check(some_condition)) { ... } > - Moved unit test binaries to t/unit-tests/bin to simplify .gitignore
> patterns.
Thanks for the updates to the test library, the range diff looks good to me.
> - Removed testing of some strbuf implementation details in t-strbuf.c
I agree that makes sense. I think it would be good to update
assert_sane_strbuf() to use the check* macros as suggest above.
Best Wishes
Phillip
^ permalink raw reply
* Re: You are marked as spam, and therefore cannot authorize a third party application.
From: Konstantin Khomoutov @ 2023-10-16 7:32 UTC (permalink / raw)
To: git
In-Reply-To: <c4880ac5-45b6-90ff-f730-6c66ba59f26f@infohubinnovations.com>
Hi!
On Mon, Oct 16, 2023 at 11:30:05AM +0530, Ramesh wrote:
> Hi GitHub Team,
This mailing list has nothing to do with GitHub - a Git hosting provider, -
as it's dedicated to the developing of Git (a distributed version control
system) itself.
> I am trying to clone my repository from local machine.But i am getting the
> follow exception that "You are marked as spam, and therefore cannot
> authorize a third party application.". Can you please suggest the same.
Please use your favorite internet search engine to look for the exact error
message (that one in double quotes) - you will find lots of relevant material,
for instance this [1]. If you want to actually contact GitHub, use [2] and [3]
for further pointers.
1. https://github.com/orgs/community/discussions/23842
2. https://github.com/orgs/community/discussions/
3. https://support.github.com/
^ permalink raw reply
* [PATCH 2/2] doc/git-repack: don't mention nonexistent "--unpacked" option
From: Patrick Steinhardt @ 2023-10-16 7:19 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <cover.1697440686.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]
The documentation for geometric repacking mentions a "--unpacked" option
that supposedly changes how loose objects are rolled up. This option has
never existed, and the implied behaviour, namely to include all unpacked
objects into the resulting packfile, is in fact the default behaviour.
Correct the documentation to not mention this option.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-repack.txt | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index dfd2a59c50..d61078b697 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -226,11 +226,8 @@ uniquely by the set of packs being "rolled-up"; in other words, the
packs determined to need to be combined in order to restore a geometric
progression.
+
-When `--unpacked` is specified, loose objects are implicitly included in
-this "roll-up", without respect to their reachability. This is subject
-to change in the future. This option (implying a drastically different
-repack mode) is not guaranteed to work with all other combinations of
-option to `git repack`.
+Loose objects are implicitly included in this "roll-up", without respect to
+their reachability. This is subject to change in the future.
+
When writing a multi-pack bitmap, `git repack` selects the largest resulting
pack as the preferred pack for object selection by the MIDX (see
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/2] doc/git-repack: small fixes for geometric repacks
From: Patrick Steinhardt @ 2023-10-16 7:19 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
[-- Attachment #1: Type: text/plain, Size: 406 bytes --]
Hi,
this small series addresses two issues in our documentation for the
git-repack(1) command that relate to geometric repacks.
Patrick
Patrick Steinhardt (2):
doc/git-repack: fix syntax for `-g` shorthand option
doc/git-repack: don't mention nonexistent "--unpacked" option
Documentation/git-repack.txt | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/2] doc/git-repack: fix syntax for `-g` shorthand option
From: Patrick Steinhardt @ 2023-10-16 7:19 UTC (permalink / raw)
To: git; +Cc: Taylor Blau
In-Reply-To: <cover.1697440686.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 981 bytes --]
The `-g` switch is a shorthand for `--geometric=` and allows the user to
specify the geometric. The documentation is wrong though and indicates
that the syntax for the shorthand is `-g=<factor>`. In fact though, the
option must be specified without the equals sign via `-g<factor>`.
Fix the syntax accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-repack.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 8545a32667..dfd2a59c50 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -209,7 +209,7 @@ depth is 4095.
Pass the `--delta-islands` option to `git-pack-objects`, see
linkgit:git-pack-objects[1].
--g=<factor>::
+-g<factor>::
--geometric=<factor>::
Arrange resulting pack structure so that each successive pack
contains at least `<factor>` times the number of objects as the
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Sebastian Thiel @ 2023-10-16 6:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, Josh Triplett, git
In-Reply-To: <xmqqmswjsv8c.fsf@gitster.g>
Thanks a lot, that makes perfect sense!
Thanks to Elijah we may also have discovered why the idea of precious files
didn't get implemented last time it came up: it's too much work to make
all portions of the code aware.
I don't know if this time will be different as I can only offer to implement
the syntax adjustment, whatever that might be (possibly after validating
the candidate against a corpus of repositories), along with the update
to `git clean` so it leaves precious files alone by default and a new flag
to also remove precious files.
Maybe that already is something worth having, but I can also imagine
that ideally there is a plan for retrofitting other portions of git as
well along with the resources to actually do it.
On 15 Oct 2023, at 18:31, Junio C Hamano wrote:
> Sebastian Thiel <sebastian.thiel@icloud.com> writes:
>
>> A particularly interesting question brought up here also was the question
>> of what's more important: untracked files, or precious files? Are they
>> effectively treated the same, or is there a difference?
>
> Think of it this way. There are two orthogonal axes.
>
> (1) Are you a candidate to be tracked, even though you are not
> tracked right now?
>
> (2) Should you be kept and make an operation fail that wants to
> remove you to make room?
>
> For untracked files, both are "Yes". As we already saw in the long
> discussion, precious files are "not to be added and not to be
> clobbered", so you'd answer "No" and "Yes" [*].
>
> In other words, both are equally protected from getting cloberred.
>
> Side note: for completeness, for ignored files, the answers are
> "No", and "No". The introduction of "precious" class makes a
> combination "No-Yes" that hasn't been possible so far.
>
> Elijah, thanks for doing a very good job of creating a catalog of
> kludges we accumulated over the years for the lack of proper support
> for the precious paths. I think they should be kept for backward
> compatibility, but for new users they should not have to learn any
> of them once we have the support for precious paths.
^ permalink raw reply
* You are marked as spam, and therefore cannot authorize a third party application.
From: Ramesh @ 2023-10-16 6:00 UTC (permalink / raw)
To: git
Hi GitHub Team,
I am trying to clone my repository from local machine.But i am getting
the follow exception that "You are marked as spam, and therefore cannot
authorize a third party application.". Can you please suggest the same.
^ permalink raw reply
* Re: [PATCH] grep: die gracefully when outside repository
From: Junio C Hamano @ 2023-10-15 17:57 UTC (permalink / raw)
To: Jeff King; +Cc: Kristoffer Haugsbakk, git, ks1322
In-Reply-To: <20231015032636.GC554702@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Is it even reasonable for "grep --no-index" to care about leaving the
> tree in the first place? That is, is there a reason we should not allow:
>
> git grep --no-index foo ../bar
A huge difference between the bare "grep" and "git grep" is that we
know the scope of the project tree, so it goes recursive by default.
Should the above command line recursively go below ../bar? Would we
allow "/" to be given?
I actually do not think these "we are allowing Git tools to be used
on random garbage" is a good idea to begin with X-<. If we invented
something nice for our variant in "git grep" and wish we can use it
outside the repository, contributing the feature to implementations
of "grep" would have been the right way to move forward, instead of
contaminating the codebase with things that are not related to Git.
Whoever did 59332d13 (Resurrect "git grep --no-index", 2010-02-06)
should be punished X-<.
Anyway.
2e48fcdb (grep docs: document --no-index option, 2010-02-25) seems
to have wanted to explicitly limit the search within the "current
directory", and I am fine to keep the search space limited by the
cwd. On the other hand, of course, the users can shoot themselves
in the foot with "grep -r foo /", so letting them use "git grep" the
same way is perhaps OK. Especially if it simplifies the code if we
lift the limitation, that is a very tempting thing to do.
> If we want to avoid leaving the current directory, then I think we need
> to be checking much sooner (but again, I would argue that it is not
> worth caring about in no-index mode).
^ permalink raw reply
* Re: [PATCH v2 1/3] bugreport: include +i in outfile suffix as needed
From: Junio C Hamano @ 2023-10-15 17:36 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git
In-Reply-To: <20231015034238.100675-2-jacob@initialcommit.io>
Jacob Stopak <jacob@initialcommit.io> writes:
> When the -s flag is absent, git bugreport includes the current hour and
> minute values in the default bugreport filename (and diagnostics zip
> filename if --diagnose is supplied).
>
> If a user runs the bugreport command more than once within a calendar
> minute, a filename conflict with an existing file occurs and the program
> errors, since the new output filename was already used for the previous
> file. If the user waits anywhere from 1 to 60 seconds (depending on
> _when during the calendar minute_ the first command was run) the command
Drop "calendar" here (see below). "If the user waits from running
the command within the same minute" may be easier to understand than
"from 1 to 60 seconds"; after all, the user does not have to wait
for more than 0.5 seconds if the previous attempt was within 0.5
seconds from the minute boundary.
> works again with no error since the default filename is now unique, and
> multiple bug reports are able to be created with default settings.
>
> This is a minor thing but can cause confusion especially for first time
> users of the bugreport command, who are likely to run it multiple times
> in quick succession to learn how it works, (like I did).
Perhaps we should refine the error message we give in this case and
we are done, then?
$ GIT_EDITOR=: git bugreport ; GIT_EDITOR=: git bugreport
Created new report at 'git-bugreport-2023-10-15-1008.txt'.
fatal: unable to create 'git-bugreport-2023-10-15-1008.txt': File exists
The second message can be a bit more friendly and suggest removing
the stale file.
> Add a '+i' into the bugreport filename suffix to make the filename
> unique, where 'i' is an integer starting at 1 and able to grow up to 9
> in the unlikely event a user runs the command 9 times in a single
> minute. This leads to default output filenames like:
What downside do you see in using 2023-10-15-1008+10 after you tried
9 of them? The code to limit the upper bound smells like a wasted
effort that helps nobody in practice because it is "unlikely".
And limiting the upper bound also means you now have to have extra
code to deal with "we ran out---error out and help the user how to
recover" anyway.
Notice that you said "in a single minute" without "calendar" and the
sentence is perfectly understandable? (see above and below).
> This means the user will end up with multiple bugreport files being
> created if they run the command multiple times quickly, but that feels
> more intuitive and consistent than an error arbitrarily occuring within
> a calendar minute, especially given that the time window in which the
> error currently occurs is variable as described above.
And drop "calendar" here, too (see above).
"Within the same minute" is fine.
> @@ -3,7 +3,6 @@
> #include "editor.h"
> #include "gettext.h"
> #include "parse-options.h"
> -#include "strbuf.h"
Looks like an unrelated change here. The updated code does use
strbuf service, so even if other header files happen to include
it, keep including it here is a good discipline for readability.
> @@ -101,12 +101,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buffer = STRBUF_INIT;
> struct strbuf report_path = STRBUF_INIT;
> + struct strbuf option_suffix = STRBUF_INIT;
> + struct strbuf default_option_suffix = STRBUF_INIT;
> int report = -1;
> time_t now = time(NULL);
> struct tm tm;
> enum diagnose_mode diagnose = DIAGNOSE_NONE;
> char *option_output = NULL;
> - char *option_suffix = "%Y-%m-%d-%H%M";
> const char *user_relative_path = NULL;
> char *prefixed_filename;
> size_t output_path_len;
> @@ -118,11 +119,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> PARSE_OPT_OPTARG, option_parse_diagnose),
> OPT_STRING('o', "output-directory", &option_output, N_("path"),
> N_("specify a destination for the bugreport file(s)")),
> - OPT_STRING('s', "suffix", &option_suffix, N_("format"),
> + OPT_STRING('s', "suffix", &option_suffix.buf, N_("format"),
> N_("specify a strftime format suffix for the filename(s)")),
> OPT_END()
> };
>
> + strbuf_addstr(&default_option_suffix, "%Y-%m-%d-%H%M");
> + strbuf_addstr(&option_suffix, default_option_suffix.buf);
It usually pays for a reviewer when two variables, one called
default and the other not, gets initialized to the same value,
because it is a sign that there is something fishy going on. A more
normal pattern is to set up the default, do whatever is needed and
if the non-default one has not been touched, then copy that default
value to the real one, and the code needed deviation from it for
whatever reason. Let's read on.
> @@ -134,9 +138,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> output_path_len = report_path.len;
>
> strbuf_addstr(&report_path, "git-bugreport-");
> - strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
> + strbuf_addftime(&report_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
> strbuf_addstr(&report_path, ".txt");
OK.
> + if (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) {
Style: never compare with 0 or NULL inside a conditional.
if (!compare(...)) {
is the idiom to use.
You may have written it this way to avoid appending after what the
user gave you as a custom pattern, but this if() statement is a
failed way to do so. The user can give what happens to be the same
as the hardcoded default pattern and you cannot tell if it came from
them or your initially hardcoded one by string comparison.
> + int i = 1;
> + int pos = report_path.len - 4;
Totally unclear where the magic "4" comes from.
> + while (file_exists(report_path.buf) && i < 10) {
> + if (i > 1)
> + strbuf_remove(&report_path, pos, 2);
> + strbuf_insertf(&report_path, pos, "+%d", i);
> + i++;
> + }
> + }
We see TOCTOU here.
Do it more like this instead.
* Discard default_option_suffix variable.
* Introduce a boolean option_suffix_is_from_user and
initialize it to false.
* Initialize option_suffix to an empty string.
* After parse_options() returns, if option_suffix is still
empty, then add the default pattern. Otherwise toggle
option_suffix_is_from_user true.
* Prepare the code so that you can recompute report_path as you
need to increment the suffix added to option_suffix.
Then where we do xopen() on report_path.buf, have a fallback loop,
and you can do something like
again:
report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
if (report < 0 && errno == EEXISTS && !option_suffix_is_from_user) {
increment_suffix(&report_path);
goto again;
} else if (report < 0) {
die_errno(_("unable to open '%s'", report_path.buf));
}
to avoid TOCTOU.
HTH.
^ permalink raw reply
* RE: [PATCH] Add x64 SHA-NI implementation to sha1 and sha256 in git ?
From: rsbecker @ 2023-10-15 17:30 UTC (permalink / raw)
To: 'Beat Bolli', 'brian m. carlson',
'Feiyang Xue', git
In-Reply-To: <fd59abd6-49a5-4f6a-8d6c-c608cd3350f7@drbeat.li>
On Sunday, October 15, 2023 12:37 PM, Beat Bolli wrote:
>On 13.10.23 23:05, brian m. carlson wrote:
>> On 2023-10-12 at 20:04:47, Feiyang Xue wrote:
>>> Looking for comments on if this is a good idea to add SHA-NI option
>>> to git, And if so, what'd be a good implementation.
>>>
>>> The attached patch is more of a proof of concept, using a sha-ni
>>> example found on internet and have it tapped into git's "hash-ll.h"
>>> when it sees make flags "SHA1_SHANI=1" and/or "SHA256_SHANI=1" for
>sha1/sha256 respectively.
>>>
>>> "git hash-object" is about 3-ish times faster for me
>>
>> You almost certainly don't want to use SHA-1 using native instructions
>> because it doesn't detect collisions, unlike the default
>> implementation (SHA-1-DC). Since SHA-1 collisions are relatively
>> cheap (less than USD
>> 45,000) to make, not detecting collisions would be a security issue
>> worthy of a CVE.
>>
>> It's fine for SHA-256, but see below.
>>
>>> There are few topics that i'm uncertain about.
>>>
>>> 1. Is it good idea to have machine-specific asm codes in git. I read
>>> there was commit fd1ec82547 which removed assembly implementation for
>>> PPC_SHA1. Does that imply maintainers doesn't want machine-specific assembly
>in source?
>>
>> I'd prefer we didn't.
>>
>> Nettle has SHA-NI extensions on systems that support it, and so does
>> OpenSSL. OpenSSL can't be used by distros for licensing reasons, but
>> Nettle can, and both of those libraries automatically detect the best
>> code to use based on the chip. One of those libraries is almost
>> always going to be linked into Git at some point because of libcurl anyway.
>>
>> If we ship the code, then someone has to maintain it, and I don't know
>> if we have any assembly experts. We might also have a bug that
>> produced incorrect results, which would be pretty disastrous for the
>> affected users. It's much better for maintainability if we push that
>> off onto the cryptographic library maintainers who are much more
>> competent in that regard than I am (I will not speak for others) and
>> let distro maintainers simply link the appropriate library, which, as
>> I said above, they're already effectively doing.
>
>In addition to all the valid points brian makes, the patch uses a non-standard
>assembler (yasm, see http://yasm.tortall.net/) that's not part of the default GNU
>toolchain.
Aside from that, not everyone uses the GNU toolchain. I would suggest that if someone needs ASM support for SHA-1, they could reimplement SHA-1-DC using rotate ASM primitives instead of directly using SHA1 assembly, but I question whether that is significantly faster than having the compiler optimizer generate the equivalent rotates. Still, maintaining any ASM code requires out of box skillsets that are unlikely sustainable.
--Randall
^ permalink raw reply
* Re: [PATCH] bugreport: add 'seconds' to default outfile name
From: Junio C Hamano @ 2023-10-15 17:06 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git
In-Reply-To: <ZStWB1/LX7cTbVGr.jacob@initialcommit.io>
Jacob Stopak <jacob@initialcommit.io> writes:
> Lol! By saying "calendar" I mean "falling on the official boundaries
> of", like 11:15:00 - 11:16:00. Unlike the time between 11:15:30 -
> 11:16:30 which is also a minute, but it's not a "calendar" minute
> because it overlaps into the next minute. I guess in this case it's more
> of a "clock" minute than a "calendar" minute though ':D... I guess
> "calendar" terminology is used more for months/years...
People use "calendar" days usually in order to to differenciate it
with "business" days. It may take your package to come cross
country and take 5 business days, but if you ship it out on Friday,
it will not arrive by Wednesday.
^ permalink raw reply
* Re: [PATCH 21/20] t5319: make corrupted large-offset test more robust
From: Junio C Hamano @ 2023-10-15 17:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Taylor Blau
In-Reply-To: <20231015031732.GB554702@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I had a similar thought process. I sort of assume anybody working
> on git.git is capable of turning to "git blame" in a situation like
> this. But maybe:
>
> # using only %(objectsize) is important here; run "git blame"
> # on these lines for more details
>
> would spell it out more clearly.
The comment was about "the commit" being not so clear which commit.
"see the message of the commit that added this comment" would have
been perfectly fine and they are not required to use "blame" if they
don't like it.
^ permalink raw reply
* Re: [PATCH] Add x64 SHA-NI implementation to sha1 and sha256 in git ?
From: Beat Bolli @ 2023-10-15 16:37 UTC (permalink / raw)
To: brian m. carlson, Feiyang Xue, git
In-Reply-To: <ZSmxFopwJyBGmZY-@tapette.crustytoothpaste.net>
Hi
On 13.10.23 23:05, brian m. carlson wrote:
> On 2023-10-12 at 20:04:47, Feiyang Xue wrote:
>> Looking for comments on if this is a good idea to add SHA-NI option to git,
>> And if so, what'd be a good implementation.
>>
>> The attached patch is more of a proof of concept, using a sha-ni example
>> found on internet and have it tapped into git's "hash-ll.h" when it sees make
>> flags "SHA1_SHANI=1" and/or "SHA256_SHANI=1" for sha1/sha256 respectively.
>>
>> "git hash-object" is about 3-ish times faster for me
>
> You almost certainly don't want to use SHA-1 using native instructions
> because it doesn't detect collisions, unlike the default implementation
> (SHA-1-DC). Since SHA-1 collisions are relatively cheap (less than USD
> 45,000) to make, not detecting collisions would be a security issue
> worthy of a CVE.
>
> It's fine for SHA-256, but see below.
>
>> There are few topics that i'm uncertain about.
>>
>> 1. Is it good idea to have machine-specific asm codes in git. I read there was
>> commit fd1ec82547 which removed assembly implementation for PPC_SHA1. Does that
>> imply maintainers doesn't want machine-specific assembly in source?
>
> I'd prefer we didn't.
>
> Nettle has SHA-NI extensions on systems that support it, and so does
> OpenSSL. OpenSSL can't be used by distros for licensing reasons, but
> Nettle can, and both of those libraries automatically detect the best
> code to use based on the chip. One of those libraries is almost always
> going to be linked into Git at some point because of libcurl anyway.
>
> If we ship the code, then someone has to maintain it, and I don't know
> if we have any assembly experts. We might also have a bug that produced
> incorrect results, which would be pretty disastrous for the affected
> users. It's much better for maintainability if we push that off onto
> the cryptographic library maintainers who are much more competent in
> that regard than I am (I will not speak for others) and let distro
> maintainers simply link the appropriate library, which, as I said above,
> they're already effectively doing.
In addition to all the valid points brian makes, the patch uses a
non-standard assembler (yasm, see http://yasm.tortall.net/) that's not
part of the default GNU toolchain.
Cheers, Beat
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Junio C Hamano @ 2023-10-15 16:31 UTC (permalink / raw)
To: Sebastian Thiel; +Cc: Elijah Newren, Josh Triplett, git
In-Reply-To: <B088FC28-BE30-424D-9CDD-7A53EDFC1710@icloud.com>
Sebastian Thiel <sebastian.thiel@icloud.com> writes:
> A particularly interesting question brought up here also was the question
> of what's more important: untracked files, or precious files? Are they
> effectively treated the same, or is there a difference?
Think of it this way. There are two orthogonal axes.
(1) Are you a candidate to be tracked, even though you are not
tracked right now?
(2) Should you be kept and make an operation fail that wants to
remove you to make room?
For untracked files, both are "Yes". As we already saw in the long
discussion, precious files are "not to be added and not to be
clobbered", so you'd answer "No" and "Yes" [*].
In other words, both are equally protected from getting cloberred.
Side note: for completeness, for ignored files, the answers are
"No", and "No". The introduction of "precious" class makes a
combination "No-Yes" that hasn't been possible so far.
Elijah, thanks for doing a very good job of creating a catalog of
kludges we accumulated over the years for the lack of proper support
for the precious paths. I think they should be kept for backward
compatibility, but for new users they should not have to learn any
of them once we have the support for precious paths.
^ permalink raw reply
* [PATCH] typo: fix the typo 'neeed' into 'needed' in the comment under merge-ort.c
From: Wangchangxin via GitGitGadget @ 2023-10-15 14:08 UTC (permalink / raw)
To: git; +Cc: Wangchangxin, foril
From: foril <1571825323@qq.com>
Signed-off-by: 王常新 (Wang Changxin) <foril@foril.space>
---
typo: fix the typo 'neeed' into 'needed' in the comment under merge-o…
the comments on line 2039 under merge-ort.c should be :
this is needed if we have content merges of content merges rather than
this is neeed if we have content merges of content merges
fix the typo
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1592%2FforiLLL%2Fcomment_patch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1592/foriLLL/comment_patch-v1
Pull-Request: https://github.com/git/git/pull/1592
merge-ort.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..aee6f7d8173 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2036,7 +2036,7 @@ static int handle_content_merge(struct merge_options *opt,
* the three blobs to merge on various sides of history.
*
* extra_marker_size is the amount to extend conflict markers in
- * ll_merge; this is neeed if we have content merges of content
+ * ll_merge; this is needed if we have content merges of content
* merges, which happens for example with rename/rename(2to1) and
* rename/add conflicts.
*/
base-commit: a9ecda2788e229afc9b611acaa26d0d9d4da53ed
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] grep: die gracefully when outside repository
From: Kristoffer Haugsbakk @ 2023-10-15 8:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, ks1322 ks1322
In-Reply-To: <20231015032636.GC554702@coredump.intra.peff.net>
On Sun, Oct 15, 2023, at 05:26, Jeff King wrote:
> Is it even reasonable for "grep --no-index" to care about leaving the
> tree in the first place? That is, is there a reason we should not allow:
>
> git grep --no-index foo ../bar
>
> ?
On second thought yeah, it doesn't make sense. We are outside of any
repository already so what does it matter where the file is relative to
the current working directory?
It seems that `pathspec.c:init_pathspec_item` should let you through in
this case.
--
Kristoffer Haugsbakk
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox