* 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
* 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: [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
* [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
* [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 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
* 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
* 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
* 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: 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
* [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
* [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 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 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 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
* 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] 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
* [[Outreachy]PATCH v2] t/t7601: Modernize test scripts using functions
From: Dorcas AnonoLitunya @ 2023-10-16 15:50 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>
---
Changes in v2:
- Add Outreachy to subject line
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
* (no subject)
From: Dorcas Litunya @ 2023-10-16 15:57 UTC (permalink / raw)
To: christian.couder; +Cc: anonolitunya, git, gitster
Bcc:
Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
Reply-To:
In-Reply-To: <20231016152113.135970-1-anonolitunya@gmail.com>
On Mon, Oct 16, 2023 at 06:21:00PM +0300, Dorcas AnonoLitunya wrote:
> 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
>
Please ignore this version as I had not indicated Outreachy in the
subject header. Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/3] rev-list: add support for commits in `--missing`
From: Junio C Hamano @ 2023-10-16 16:24 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps
In-Reply-To: <20231016103830.56486-1-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> ... some extra checks here, especially because we don't want to
> allow missing commit's tree and parent to be parsed.
Good changes relative to the previous round.
A bad news is that with this series (especially [PATCH 3/3]) applied
on top of Patrick's "always make sure the commit we found from the
commit-graph at least exists" change, t5310 and t5320 seem to
consistently fail for me. They pass with the first two steps
applied without [3/3] but that is to be expected as they are both
no-ops.
The change in 3/3 to list-objects.c::do_traverse() seems to be the
culprit. Reverting that single hunk makes t5310 and t5320 pass
again. What I find alarming is that two tests that are touched by
this series, t5318 and t6022, do not seem to fail with the hunk
reverted, which means the hunk has no meaningful test coverage for
the purpose of this series.
I didn't even try to understand what is going on, so there may be a
very good reason that the change must be there for do_traverse() to
work correctly. I dunno.
> +- if (commit->object.flags & ADDED)
> ++ if (commit->object.flags & ADDED || commit->object.flags & MISSING)
This and others are syntactically correct C, but some folks may find
it more readable to see each of the bitwise operations enclosed in a
pair of parentheses when combined by logical operations, i.e.,
if ((commit->object.flags & ADDED) || (commit->object.flags & MISSING))
In this particular case, we can even say
if (commit->object.flags & (ADDED | MISSING))
meaning, "if we have either the ADDED or the MISSING bit set", which
may make it even clearer.
Thanks.
^ permalink raw reply
* Re: [PATCH v8 2.5/3] fixup! unit tests: add TAP unit test framework
From: Junio C Hamano @ 2023-10-16 16:41 UTC (permalink / raw)
To: steadmon; +Cc: Phillip Wood, calvinwan, git, linusa, rsbecker
In-Reply-To: <20231016134421.21659-1-phillip.wood123@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Here are a couple of cleanups for the unit test framework that I
> noticed.
Thanks. I trust that this will be squashed into the next update,
but in the meantime, I'll include it in the copy of the series I
have (without squashing). Here is another one I noticed.
----- >8 --------- >8 --------- >8 -----
Subject: [PATCH] fixup! ci: run unit tests in CI
A CI job failed due to contrib/coccinelle/equals-null.cocci
and suggested this change, which seems sensible.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/unit-tests/t-strbuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
index c2fcb0cbd6..8388442426 100644
--- a/t/unit-tests/t-strbuf.c
+++ b/t/unit-tests/t-strbuf.c
@@ -28,7 +28,7 @@ static void setup_populated(void (*f)(struct strbuf*, void*), char *init_str, vo
static int assert_sane_strbuf(struct strbuf *buf)
{
/* Initialized strbufs should always have a non-NULL buffer */
- if (buf->buf == NULL)
+ if (!buf->buf)
return 0;
/* Buffers should always be NUL-terminated */
if (buf->buf[buf->len] != '\0')
--
2.42.0-398-ga9ecda2788
^ permalink raw reply related
* Re: [PATCH] t/t7601: Modernize test scripts using functions
From: Junio C Hamano @ 2023-10-16 16:53 UTC (permalink / raw)
To: Dorcas AnonoLitunya; +Cc: christian.couder, git
In-Reply-To: <20231016152113.135970-1-anonolitunya@gmail.com>
Dorcas AnonoLitunya <anonolitunya@gmail.com> writes:
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
Let's try if we can pack a bit more information. For example
Subject: [PATCH] t7601: use "test_path_is_file" etc. instead of "test -f"
would clarify what kind of modernization is done by this patch.
> The test script is currently using the command format 'test -f' to
> check for existence or absence of files.
"is currently using" -> "uses".
> Replace it with new helper functions following the format
> 'test_path_is_file'.
I am not sure what role "the format" plays in this picture.
test_path_is_file is not new---it has been around for quite a while.
> Consequently, the patch also replaces the inverse command '! test -f' or
> 'test ! -f' with new helper function following the format
> 'test_path_is_missing'
A bit more on this later.
> This adjustment using helper functions makes the code more readable and
> easier to understand.
Looking good. If I were writing this, I'll make the whole thing
more like this, though:
t7601: use "test_path_is_file" etc. instead of "test -f"
Some tests in t7601 use "test -f" and "test ! -f" to see if a
path exists or is missing. Use test_path_is_file and
test_path_is_missing helper functions to clarify these tests a
bit better. This especially matters for the "missing" case,
because "test ! -f F" will be happy if "F" exists as a
directory, but the intent of the test is that "F" should not
exist, even as a directory.
> 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 &&
The original says "We are happy if c2.c is not a file", so it would
have been happy if by some mistake "git reset" created a directory
there. But the _intent_ of the test is that we do not have anything
at c2.c, and the updated code expresses it better.
^ permalink raw reply
* (no subject)
From: Dorcas Litunya @ 2023-10-16 18:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: christian.couder, git
Bcc:
Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
Reply-To:
In-Reply-To: <xmqq1qdumrto.fsf@gitster.g>
On Mon, Oct 16, 2023 at 09:53:55AM -0700, Junio C Hamano wrote:
> Dorcas AnonoLitunya <anonolitunya@gmail.com> writes:
>
> > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
>
> Let's try if we can pack a bit more information. For example
>
> Subject: [PATCH] t7601: use "test_path_is_file" etc. instead of "test -f"
>
> would clarify what kind of modernization is done by this patch.
>
> > The test script is currently using the command format 'test -f' to
> > check for existence or absence of files.
>
> "is currently using" -> "uses".
>
> > Replace it with new helper functions following the format
> > 'test_path_is_file'.
>
> I am not sure what role "the format" plays in this picture.
> test_path_is_file is not new---it has been around for quite a while.
>
> > Consequently, the patch also replaces the inverse command '! test -f' or
> > 'test ! -f' with new helper function following the format
> > 'test_path_is_missing'
>
> A bit more on this later.
>
So should I replace this in the next version or leave this as is?
> > This adjustment using helper functions makes the code more readable and
> > easier to understand.
>
> Looking good. If I were writing this, I'll make the whole thing
> more like this, though:
>
> t7601: use "test_path_is_file" etc. instead of "test -f"
>
> Some tests in t7601 use "test -f" and "test ! -f" to see if a
> path exists or is missing. Use test_path_is_file and
> test_path_is_missing helper functions to clarify these tests a
> bit better. This especially matters for the "missing" case,
> because "test ! -f F" will be happy if "F" exists as a
> directory, but the intent of the test is that "F" should not
> exist, even as a directory.
>
>
> > 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 &&
>
> The original says "We are happy if c2.c is not a file", so it would
> have been happy if by some mistake "git reset" created a directory
> there. But the _intent_ of the test is that we do not have anything
> at c2.c, and the updated code expresses it better.
^ permalink raw reply
* Re: [PATCH v2 0/3] rev-list: add support for commits in `--missing`
From: Karthik Nayak @ 2023-10-16 19:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqqbkcyo7ro.fsf@gitster.g>
On Mon, Oct 16, 2023 at 6:24 PM Junio C Hamano <gitster@pobox.com> wrote:
> Good changes relative to the previous round.
>
> A bad news is that with this series (especially [PATCH 3/3]) applied
> on top of Patrick's "always make sure the commit we found from the
> commit-graph at least exists" change, t5310 and t5320 seem to
> consistently fail for me. They pass with the first two steps
> applied without [3/3] but that is to be expected as they are both
> no-ops.
>
> The change in 3/3 to list-objects.c::do_traverse() seems to be the
> culprit. Reverting that single hunk makes t5310 and t5320 pass
> again.
It seems that the new chunk introduced now causes collision because of
the bit field of the MISSING flag being the same as the pre-existing
NEEDS_BITMAP flag. Earlier this wasn't a concern, but now, their paths
have converged at this change.
So changing the bit field for the MISSING flag from `22` to an unused `28`
fixes the issue. I should have run the whole test suite again. Apologies.
> What I find alarming is that two tests that are touched by
> this series, t5318 and t6022, do not seem to fail with the hunk
> reverted, which means the hunk has no meaningful test coverage for
> the purpose of this series.
>
Well, actually the newly introduced tests t6022 require this block,
but this is specific
to when commit graph is enabled. So what you did see was correct, but
the test would
also fail with `GIT_TEST_COMMIT_GRAPH=1` if the block was removed. That's
exactly why in this series I increased the number of commits in the
test block from 2->3.
> > +- if (commit->object.flags & ADDED)
> > ++ if (commit->object.flags & ADDED || commit->object.flags & MISSING)
>
> This and others are syntactically correct C, but some folks may find
> it more readable to see each of the bitwise operations enclosed in a
> pair of parentheses when combined by logical operations, i.e.,
>
> if ((commit->object.flags & ADDED) || (commit->object.flags & MISSING))
>
> In this particular case, we can even say
>
> if (commit->object.flags & (ADDED | MISSING))
>
> meaning, "if we have either the ADDED or the MISSING bit set", which
> may make it even clearer.
I agree with this, I'll add it in the next block.
Thanks for the quick review, I'll wait a day/two and send v3 with the
changes to fix tests
and cleaner code.
^ permalink raw reply
* Git does not work on my machine!!!
From: jtrites @ 2023-10-16 20:20 UTC (permalink / raw)
To: git; +Cc: John Trites
I would appreciate some help resolving this issue!!!
JohnTrites@MSI MINGW64 /c/Users/JohnTrites
$ git config --global user.name "JohnTrites"
error: could not lock config file C:/Users/John
Trites/AppData/Roaming/SPB_16.6/.gitconfig: No such file or directory
^ 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