* Re: SHA256 support not experimental, or?
From: Junio C Hamano @ 2023-06-29 5:59 UTC (permalink / raw)
To: Adam Majer; +Cc: git
In-Reply-To: <2f5de416-04ba-c23d-1e0b-83bb655829a7@zombino.com>
Adam Majer <adamm@zombino.com> writes:
> Is sha256 still considered experimental or can it be assumed to be stable?
I do not think we would officially label SHA-256 support as "stable"
until we have good interoperability with SHA-1 repositories, but the
expectation is that we will make reasonable effort to keep migration
path for the current SHA-256 repositories, even if it turns out that
its on-disk format need to be updated, to keep the end-user data safe.
So while "no-longer-experimental" patch is probably a bit premature,
the warning in flashing red letters to caution against any use other
than testing may want to be toned down.
Thanks.
^ permalink raw reply
* index.skipHash doesn't work with split index, was Re: Bug Report
From: Jeff King @ 2023-06-29 8:38 UTC (permalink / raw)
To: Tiago d'Almeida; +Cc: Derrick Stolee, git
In-Reply-To: <CAO=RawtAvOna1xrBNY+T-fo4UQe-ipC6OvFODvOSdu4wUML3Ng@mail.gmail.com>
On Tue, Jun 27, 2023 at 05:02:30PM +0100, Tiago d'Almeida wrote:
> Attached to this email follow the `git bugreport` and global `config`
> files, and the git_bug repo.
Thanks for providing your config; it was very important to reproducing.
The bug comes from the combination of "core.splitIndex" and
"index.skipHash" (the latter is triggered in your config by
"feature.manyFiles").
Here's a quick reproduction:
git init repo
cd repo
touch file
git -c core.splitIndex=true -c index.skipHash=true add file
That should add "file" to the index but doesn't. Removing either the
splitIndex option or the skipHash option makes it work. I didn't dig
further than that.
Adding the author of skipHash to the cc.
-Peff
^ permalink raw reply
* Re: [PATCH] t4205: correctly test %(describe:abbrev=...)
From: Kousik Sanagavarapu @ 2023-06-29 9:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma
In-Reply-To: <xmqqv8f7b7h1.fsf@gitster.g>
On Wed, Jun 28, 2023 at 02:30:18PM -0700, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
> > The pretty format %(describe:abbrev=<number>) tells describe to use only
> > <number> characters of the oid to generate the human-readable format of
> > the commit-ish.
>
> Is that *only* correct? I thought it was "at least <number> hexdigits"
> to allow for future growth of the project.
Yeah, this is wrong. It should be "at least" for being more precise as
we may need more than <number> in some cases. Will change that. Thanks
for catching it.
> > This is not apparent in the test for %(describe:abbrev=...) because we
> > directly tag HEAD and use that, in which case the human-readable format
> > is just the tag name. So, create a new commit and use that instead.
>
> Nice. How was this found, I have to wonder, and more importantly,
I was working on duplicating "%(describe)" from pretty, in ref-filter
and was writing tests for it. While going through the trash directory
for some other breakage, I found this. So it was kind of a chance.
> how would we have written this test in the first place to avoid
> testing "the wrong thing", to learn from this experience?
I don't have a clue :).
In this particular test, this is not "the wrong thing" anyways, as you
explain below. We just fail to test it wholly (we missed some cases).
> > test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
> > - test_when_finished "git tag -d tagname" &&
> > - git tag -a -m tagged tagname &&
> > + test_commit --no-tag file &&
> > git describe --abbrev=15 >expect &&
> > git log -1 --format="%(describe:abbrev=15)" >actual &&
> > test_cmp expect actual
>
> The current test checks that the output in the case where the number
> of commits since the tag is 0, and "describe --abbrev=15" and "log
> --format='%(describe:abbrev=15)'" give exactly the same result.
> Which is a good thing to test.
>
> But we *also* want to test a more typical case where there are
> commits between HEAD and the tag that is used to describe it.
>
> And we *also* want to make sure that the hexadecimal object name
> suffix used in the description is at least 15 hexdigits long, if not
> more.
>
> The updated test drops test #1 (which is questionable), adds test #2
> (which is good), and still omits test #3 (which is not so good).
>
> So, perhaps
>
> test_when_finished "git tag -d tagname" &&
> - git tag -a -m tagged tagname &&
> test_commit --no-tag file &&
> git describe --abbrev=15 >expect &&
> git log -1 --format="%(describe:abbrev=15)" >actual &&
> test_cmp expect actual &&
> + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
> + test 16 -le $(wc -c <hexpart) &&
> +
> + git tag -a -m tagged tagname &&
> + git describe --abbrev=15 >expect &&
> + git log -1 --format="%(describe:abbrev=15)" >actual &&
> + test_cmp expect actual &&
> + test tagname = $(cat actual)
>
> or something along the line? First we test with a commit that is
> not tagged at all to have some commits between the tag and HEAD with
> the original comparison (this is for #2), then we make sure the
> length of the hexpart (new---this is for #3), and then we add the
> tag to see the "exact" case also works (this is for #1).
Yeah, makes sense. Thanks for such a nice explanation.
I have applied this and it works. I'll reroll with this change and
also the change in the log message (and also maybe add some comments
about these cases).
I'll make sure to do this in the tests for ref-filter too, about which I
mentioned above.
Thanks
^ permalink raw reply
* Re: [PATCH] repack: only repack .packs that exist
From: Taylor Blau @ 2023-06-29 9:24 UTC (permalink / raw)
To: Jeff King; +Cc: Derrick Stolee via GitGitGadget, git, gitster, Derrick Stolee
In-Reply-To: <20230627075427.GE1226768@coredump.intra.peff.net>
On Tue, Jun 27, 2023 at 03:54:27AM -0400, Jeff King wrote:
> > In other cases, Git prepares its list of packfiles by scanning .idx
> > files and then only adds it to the packfile list if the corresponding
> > .pack file exists. It even does so without a warning! (See
> > add_packed_git() in packfile.c for details.)
>
> Interesting. I'd have expected a warning. ;)
Reading this all over, I was incorrect in my original suggestion that
not checking for the existence of the matching ".pack" file was the
expected thing to do.
We don't try to open the ".pack" file itself in add_packed_git(), that
happens later in open_packed_git() if we end up actually needing to read
objects from the pack, or want to retain a handle on it for later if
we're worried that the ".pack" file itself might get deleted.
But that add_packed_git() silently ignores a pack which has its ".idx"
file and not its ".pack" file is behavior that I wasn't aware of, and
had somehow missed when I reread the function last week.
Stolee: I apologize for having missed this important detail. I think in
that sense matching the behavior of add_packed_git() here is the right
thing to do, and that this patch makes sense to me.
> I also kind of wonder if this repack code should simply be loading and
> iterating the packed_git list, but that is a much bigger change.
I have wanted to do this for a while ;-). The minimal patch is less
invasive than I had thought:
--- 8< ---
diff --git a/builtin/repack.c b/builtin/repack.c
index 1e21a21ea8..e854c832fd 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -104,46 +104,33 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,
struct string_list *fname_kept_list,
const struct string_list *extra_keep)
{
- DIR *dir;
- struct dirent *e;
- char *fname;
+ struct packed_git *p;
struct strbuf buf = STRBUF_INIT;
- if (!(dir = opendir(packdir)))
- return;
-
- while ((e = readdir(dir)) != NULL) {
- size_t len;
+ for (p = get_all_packs(the_repository); p; p = p->next) {
int i;
-
- if (!strip_suffix(e->d_name, ".idx", &len))
- continue;
-
- strbuf_reset(&buf);
- strbuf_add(&buf, e->d_name, len);
- strbuf_addstr(&buf, ".pack");
+ const char *base = basename(p->pack_name);
for (i = 0; i < extra_keep->nr; i++)
- if (!fspathcmp(buf.buf, extra_keep->items[i].string))
+ if (!fspathcmp(base, extra_keep->items[i].string))
break;
- fname = xmemdupz(e->d_name, len);
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, base);
+ strbuf_strip_suffix(&buf, ".pack");
- if ((extra_keep->nr > 0 && i < extra_keep->nr) ||
- (file_exists(mkpath("%s/%s.keep", packdir, fname)))) {
- string_list_append_nodup(fname_kept_list, fname);
- } else {
+ if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)
+ string_list_append(fname_kept_list, buf.buf);
+ else {
struct string_list_item *item;
- item = string_list_append_nodup(fname_nonkept_list,
- fname);
- if (file_exists(mkpath("%s/%s.mtimes", packdir, fname)))
+ item = string_list_append(fname_nonkept_list, buf.buf);
+ if (p->is_cruft)
item->util = (void*)(uintptr_t)CRUFT_PACK;
}
}
- closedir(dir);
- strbuf_release(&buf);
string_list_sort(fname_kept_list);
+ strbuf_release(&buf);
}
static void remove_redundant_pack(const char *dir_name, const char *base_name)
--- >8 ---
I think you could probably go further than this, since having to store
the suffix-less pack names in the fname_kept and fname_nonkept lists is
kind of weird.
It would be nice if we could store pointers to the actual packed_git
structs themselves in place of those lists instead, but I'm not
immediately sure how feasible it would be to do since we re-prepare the
object store between enumerating and then removing these packs.
Thanks,
Taylor
^ permalink raw reply related
* Re: SHA256 support not experimental, or?
From: Adam Majer @ 2023-06-29 10:42 UTC (permalink / raw)
To: brian m. carlson, git
In-Reply-To: <ZJzlezhHk4HpPmRk@tapette.crustytoothpaste.net>
[-- Attachment #1: Type: text/plain, Size: 288 bytes --]
On 6/29/23 03:59, brian m. carlson wrote:
> I have no intention of changing things at this point. I think it should
> be viewed as stable by now, and I'd support this patch, although to get
> it picked up it will need a commit message and a sign-off.
Sounds good. Patch follows.
- Adam
[-- Attachment #2: 0001-doc-sha256-is-no-longer-experimental.patch --]
[-- Type: text/x-patch, Size: 2210 bytes --]
From 90be51143e741053390810720ba4a639c3b0b74c Mon Sep 17 00:00:00 2001
From: Adam Majer <adamm@zombino.com>
Date: Wed, 28 Jun 2023 14:46:02 +0200
Subject: [PATCH] doc: sha256 is no longer experimental
The purpose of this patch is to remove scary wording that basically
stops people using sha256 repositories not because of interoperability
issues with sha1 repositories, but from fear that their work will
suddenly become incompatible in some future version of git.
We should be clear that currently sha256 repositories will not work with
sha1 repositories but stop the scary words.
Signed-off-by: Adam Majer <adamm@zombino.com>
---
Documentation/git.txt | 4 ++--
Documentation/object-format-disclaimer.txt | 8 ++------
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index f0cafa2290..666dbdb55c 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -553,8 +553,8 @@ double-quotes and respecting backslash escapes. E.g., the value
If this variable is set, the default hash algorithm for new
repositories will be set to this value. This value is
ignored when cloning and the setting of the remote repository
- is always used. The default is "sha1". THIS VARIABLE IS
- EXPERIMENTAL! See `--object-format` in linkgit:git-init[1].
+ is always used. The default is "sha1". See `--object-format`
+ in linkgit:git-init[1].
Git Commits
~~~~~~~~~~~
diff --git a/Documentation/object-format-disclaimer.txt b/Documentation/object-format-disclaimer.txt
index 4cb106f0d1..1e976688be 100644
--- a/Documentation/object-format-disclaimer.txt
+++ b/Documentation/object-format-disclaimer.txt
@@ -1,6 +1,2 @@
-THIS OPTION IS EXPERIMENTAL! SHA-256 support is experimental and still
-in an early stage. A SHA-256 repository will in general not be able to
-share work with "regular" SHA-1 repositories. It should be assumed
-that, e.g., Git internal file formats in relation to SHA-256
-repositories may change in backwards-incompatible ways. Only use
-`--object-format=sha256` for testing purposes.
+Note: SHA-256 repositories currently will not be able to share work
+with "regular" SHA-1 repositories.
--
2.41.0
^ permalink raw reply related
* Re: SHA256 support not experimental, or?
From: Adam Majer @ 2023-06-29 10:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqmt0iajww.fsf@gitster.g>
On 6/29/23 07:59, Junio C Hamano wrote:
> Adam Majer <adamm@zombino.com> writes:
>
>> Is sha256 still considered experimental or can it be assumed to be stable?
>
> I do not think we would officially label SHA-256 support as "stable"
> until we have good interoperability with SHA-1 repositories, but the
> expectation is that we will make reasonable effort to keep migration
> path for the current SHA-256 repositories, even if it turns out that
> its on-disk format need to be updated, to keep the end-user data safe.
That could be a different definition of stable. But I'm satisfied that
current sha256 repositories will not end up incompatible with some
future version of git without migration path (talking about on-disk format).
So maybe my question should be reworded to "is sha256 still considered
early stage, for testing purposes only with possible data-loss or can it
be relied on for actual long lived repositories?"
> So while "no-longer-experimental" patch is probably a bit premature,
> the warning in flashing red letters to caution against any use other
> than testing may want to be toned down.
Agreed. I think it should be clear that SHA256 and SHA1 repositories
cannot share data at this point. The scary wording should be removed
though, as currently it sounds like "data loss incoming and it's your
fault" if one chooses sha256
- Adam
^ permalink raw reply
* [PATCH 0/3] commit -a -m: allow the top-level tree to become empty again
From: Johannes Schindelin via GitGitGadget @ 2023-06-29 13:23 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
This patch series is in response to the bug report in
https://github.com/git-for-windows/git/issues/4462 that demonstrates that
git commit -a -m <msg> would no longer always stage all updates to tracked
files. The bug has been introduced in Git v2.40.0.
Johannes Schindelin (3):
do_read_index(): always mark index as initialized unless erroring out
split-index: accept that a base index can be empty
commit -a -m: allow the top-level tree to become empty again
builtin/commit.c | 7 ++-----
read-cache.c | 15 +++++++++------
t/t2200-add-update.sh | 11 +++++++++++
3 files changed, 22 insertions(+), 11 deletions(-)
base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1554%2Fdscho%2Ffix-git-commit-a-m-when-tree-becomes-empty-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1554/dscho/fix-git-commit-a-m-when-tree-becomes-empty-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1554
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/3] do_read_index(): always mark index as initialized unless erroring out
From: Johannes Schindelin via GitGitGadget @ 2023-06-29 13:23 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1554.git.1688044991.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index
from read_cache(), 2008-08-23) a flag was introduced into the
`index_state` structure to indicate whether it had been initialized (or
more correctly: read and parsed).
There was one code path that was not handled, though: when the index
file does not yet exist (but the `must_exist` parameter is set to 0 to
indicate that that's okay). In this instance, Git wants to go forward
with a new, pristine Git index, almost as if the file had existed and
contained no index entries or extensions.
Since Git wants to handle this situation the same as if an "empty" Git
index file existed, let's set the `initialized` flag also in that case.
This is necessary to prepare for fixing the bug where the condition
`cache_nr == 0` is incorrectly used as an indicator that the index was
already read, and the condition `initialized != 0` needs to be used
instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
read-cache.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/read-cache.c b/read-cache.c
index f4c31a68c85..b10caa9831c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2285,6 +2285,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
if (fd < 0) {
if (!must_exist && errno == ENOENT) {
set_new_index_sparsity(istate);
+ istate->initialized = 1;
return 0;
}
die_errno(_("%s: index file open failed"), path);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/3] split-index: accept that a base index can be empty
From: Johannes Schindelin via GitGitGadget @ 2023-06-29 13:23 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1554.git.1688044991.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
We are about to fix an ancient bug where `do_read_index()` pretended
that the index was not initialized when there are no index entries.
Before the `index_state` structure gained the `initialized` flag in
913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from
read_cache(), 2008-08-23), that was the best we could do (even if it was
incorrect: it is totally possible to read a Git index file that contains
no index entries).
This pattern was repeated also in 998330ac2e7 (read-cache: look for
shared index files next to the index, too, 2021-08-26), which we fix
here by _not_ mistaking an empty base index for a missing
`sharedindex.*` file.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
read-cache.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index b10caa9831c..e15a472f54f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2455,12 +2455,14 @@ int read_index_from(struct index_state *istate, const char *path,
base_oid_hex = oid_to_hex(&split_index->base_oid);
base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
- trace2_region_enter_printf("index", "shared/do_read_index",
- the_repository, "%s", base_path);
- ret = do_read_index(split_index->base, base_path, 0);
- trace2_region_leave_printf("index", "shared/do_read_index",
- the_repository, "%s", base_path);
- if (!ret) {
+ if (file_exists(base_path)) {
+ trace2_region_enter_printf("index", "shared/do_read_index",
+ the_repository, "%s", base_path);
+
+ ret = do_read_index(split_index->base, base_path, 0);
+ trace2_region_leave_printf("index", "shared/do_read_index",
+ the_repository, "%s", base_path);
+ } else {
char *path_copy = xstrdup(path);
char *base_path2 = xstrfmt("%s/sharedindex.%s",
dirname(path_copy), base_oid_hex);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/3] commit -a -m: allow the top-level tree to become empty again
From: Johannes Schindelin via GitGitGadget @ 2023-06-29 13:23 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1554.git.1688044991.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In 03267e8656c (commit: discard partial cache before (re-)reading it,
2022-11-08), a memory leak was plugged by discarding any partial index
before re-reading it.
The problem with this memory leak fix is that it was based on an
incomplete understanding of the logic introduced in 7168624c353 (Do not
generate full commit log message if it is not going to be used,
2007-11-28).
That logic was introduced to add a shortcut when committing without
editing the commit message interactively. A part of that logic was to
ensure that the index was read into memory:
if (!active_nr && read_cache() < 0)
die(...)
Translation to English: If the index has not yet been read, read it, and
if that fails, error out.
That logic was incorrect, though: It used `!active_nr` as an indicator
that the index was not yet read. Usually this is not a problem because
in the vast majority of instances, the index contains at least one
entry.
And it was natural to do it this way because at the time that condition
was introduced, the `index_state` structure had no explicit flag to
indicate that it was initialized: This flag was only introduced in
913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from
read_cache(), 2008-08-23), but that commit did not adjust the code path
where no index file was found and a new, pristine index was initialized.
Now, when the index does not contain any entry (which is quite
common in Git's test suite because it starts quite a many repositories
from scratch), subsequent calls to `do_read_index()` will mistake the
index not to be initialized, and read it again unnecessarily.
This is a problem because after initializing the empty index e.g. the
`cache_tree` in that index could have been initialized before a
subsequent call to `do_read_index()` wants to ensure an initialized
index. And if that subsequent call mistakes the index not to have been
initialized, it would lead to leaked memory.
The correct fix for that memory leak is to adjust the condition so that
it does not mistake `active_nr == 0` to mean that the index has not yet
been read.
Using the `initialized` flag instead, we avoid that mistake, and as a
bonus we can fix a bug at the same time that was introduced by the
memory leak fix: When deleting all tracked files and then asking `git
commit -a -m ...` to commit the result, Git would internally update the
index, then discard and re-read the index undoing the update, and fail
to commit anything.
This fixes https://github.com/git-for-windows/git/issues/4462
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/commit.c | 7 ++-----
t/t2200-add-update.sh | 11 +++++++++++
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 65a5c0e29d5..4cf2baaf943 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -998,11 +998,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
struct object_id oid;
const char *parent = "HEAD";
- if (!the_index.cache_nr) {
- discard_index(&the_index);
- if (repo_read_index(the_repository) < 0)
- die(_("Cannot read index"));
- }
+ if (!the_index.initialized && repo_read_index(the_repository) < 0)
+ die(_("Cannot read index"));
if (amend)
parent = "HEAD^1";
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index be394f1131a..c01492f33f8 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -197,4 +197,15 @@ test_expect_success '"add -u non-existent" should fail' '
! grep "non-existent" actual
'
+test_expect_success '"commit -a" implies "add -u" if index becomes empty' '
+ git rm -rf \* &&
+ git commit -m clean-slate &&
+ test_commit file1 &&
+ rm file1.t &&
+ test_tick &&
+ git commit -a -m remove &&
+ git ls-tree HEAD: >out &&
+ test_must_be_empty out
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2] t4205: correctly test %(describe:abbrev=...)
From: Kousik Sanagavarapu @ 2023-06-29 13:18 UTC (permalink / raw)
To: git; +Cc: Kousik Sanagavarapu, Junio C Hamano, Christian Couder,
Hariom Verma
In-Reply-To: <20230628181753.10384-1-five231003@gmail.com>
The pretty format %(describe:abbrev=<number>) tells describe to use
at least <number> digits of the oid to generate the human-readable
format of the commit-ish.
There are three things to test here:
- Check that we can describe a commit that is not tagged (that is,
for example our HEAD is at least one commit ahead of some reachable
commit which is tagged) with at least <number> digits of the oid
being used for describing it.
- Check that when using such a commit-ish, we always use at least
<number> digits of the oid to describe it.
- Check that we can describe a tag. This just gives the name of the
tag irrespective of abbrev (abbrev doesn't make sense here).
Do this, instead of the current test which only tests the last case.
Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
Changes since v1:
- Changed the log message
- Added things to be tested as commented by Junio
Range-diff vs v1:
1: 2c10de6c11 ! 1: 76c3e38033 t4205: correctly test
%(describe:abbrev=...)
@@ Metadata
## Commit message ##
t4205: correctly test %(describe:abbrev=...)
- The pretty format %(describe:abbrev=<number>) tells describe to
use only
- <number> characters of the oid to generate the human-readable
format of
- the commit-ish.
+ The pretty format %(describe:abbrev=<number>) tells describe to
use
+ at least <number> digits of the oid to generate the
human-readable
+ format of the commit-ish.
- This is not apparent in the test for %(describe:abbrev=...)
because we
- directly tag HEAD and use that, in which case the
human-readable format
- is just the tag name. So, create a new commit and use that
instead.
+ There are three things to test here:
+ - Check that we can describe a commit that is not tagged
(that is,
+ for example our HEAD is at least one commit ahead of some
reachable
+ commit which is tagged) with at least <number> digits of
the oid
+ being used for describing it.
+ - Check that when using such a commit-ish, we always use at
least
+ <number> digits of the oid to describe it.
+
+ - Check that we can describe a tag. This just gives the name
of the
+ tag irrespective of abbrev (abbrev doesn't make sense
here).
+
+ Do this, instead of the current test which only tests the last
case.
+
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
## t/t4205-log-pretty-formats.sh ##
@@ t/t4205-log-pretty-formats.sh: test_expect_success
'%(describe:tags) vs git describe --tags' '
- '
test_expect_success '%(describe:abbrev=...) vs git describe
--abbrev=...' '
-- test_when_finished "git tag -d tagname" &&
-- git tag -a -m tagged tagname &&
+ test_when_finished "git tag -d tagname" &&
++
++ # Case 1: We have commits between HEAD and the most recent tag
++ # reachable from it
+ test_commit --no-tag file &&
++ git describe --abbrev=15 >expect &&
++ git log -1 --format="%(describe:abbrev=15)" >actual &&
++ test_cmp expect actual &&
++
++ # Make sure the hash used is at least 15 digits long
++ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
++ test 16 -le $(wc -c <hexpart) &&
++
++ # Case 2: We have a tag at HEAD, describe directly gives the
++ # name of the tag
+ git tag -a -m tagged tagname &&
git describe --abbrev=15 >expect &&
git log -1 --format="%(describe:abbrev=15)" >actual &&
- test_cmp expect actual
+- test_cmp expect actual
++ test_cmp expect actual &&
++ test tagname = $(cat actual)
+ '
+
+ test_expect_success 'log --pretty with space stealing' '
t/t4205-log-pretty-formats.sh | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 4cf8a77667..dd9035aa38 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -1012,10 +1012,25 @@ test_expect_success '%(describe:tags) vs git describe --tags' '
test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
test_when_finished "git tag -d tagname" &&
+
+ # Case 1: We have commits between HEAD and the most recent tag
+ # reachable from it
+ test_commit --no-tag file &&
+ git describe --abbrev=15 >expect &&
+ git log -1 --format="%(describe:abbrev=15)" >actual &&
+ test_cmp expect actual &&
+
+ # Make sure the hash used is at least 15 digits long
+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+ test 16 -le $(wc -c <hexpart) &&
+
+ # Case 2: We have a tag at HEAD, describe directly gives the
+ # name of the tag
git tag -a -m tagged tagname &&
git describe --abbrev=15 >expect &&
git log -1 --format="%(describe:abbrev=15)" >actual &&
- test_cmp expect actual
+ test_cmp expect actual &&
+ test tagname = $(cat actual)
'
test_expect_success 'log --pretty with space stealing' '
--
2.41.0.29.g8148156d44.dirty
^ permalink raw reply related
* Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()
From: Vinayak Dev @ 2023-06-29 16:05 UTC (permalink / raw)
To: git
Hey there!
I was looking through Documentation/MyFirstObjectWalk.txt, and upon
building the branch containing the given code, I find that I get the
error that C99 does not allow implicit function declaration where
trace_printf() is encountered. However, upon including trace.h the
error disappears, and the build proceeds just fine.
I did put DEVELOPER=1 in config.mak before building, but it doesn't
seem to work.
Is the error pointing to a problem, or am I doing something wrong?
If it is the former, I would be very happy to send a patch fixing this.
Thanks a lot!
Vinayak
^ permalink raw reply
* Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()
From: Emily Shaffer @ 2023-06-29 16:33 UTC (permalink / raw)
To: Vinayak Dev; +Cc: git
In-Reply-To: <CADE8Naq5W3Bn=gwV7W-xMvYOMMRO=ZY9Ly6im4Rb_qFjMWTbTg@mail.gmail.com>
On Thu, Jun 29, 2023 at 9:06 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
>
> Hey there!
> I was looking through Documentation/MyFirstObjectWalk.txt, and upon
> building the branch containing the given code, I find that I get the
> error that C99 does not allow implicit function declaration where
> trace_printf() is encountered. However, upon including trace.h the
> error disappears, and the build proceeds just fine.
>
> I did put DEVELOPER=1 in config.mak before building, but it doesn't
> seem to work.
>
> Is the error pointing to a problem, or am I doing something wrong?
> If it is the former, I would be very happy to send a patch fixing this.
Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
very recently a patch to clean up some headers which probably were
implicitly including trace.h when I wrote this walkthrough. Patches
totally welcome - and if you were working from the reference code in
https://github.com/nasamuffin/git/tree/myfirstrevwalk and it's on your
way to rebase and fix that too, I'm happy to update my branch
accordingly too. (If you weren't, don't worry about doing the extra
work, though.)
>
> Thanks a lot!
> Vinayak
^ permalink raw reply
* Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()
From: Emily Shaffer @ 2023-06-29 16:35 UTC (permalink / raw)
To: Vinayak Dev; +Cc: git
In-Reply-To: <CAJoAoZ=OEfsgkqsag926tH4GEuafX26A09SGZ1vR1uLh2W_4TA@mail.gmail.com>
On Thu, Jun 29, 2023 at 9:33 AM Emily Shaffer <nasamuffin@google.com> wrote:
>
> On Thu, Jun 29, 2023 at 9:06 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
> >
> > Hey there!
> > I was looking through Documentation/MyFirstObjectWalk.txt, and upon
> > building the branch containing the given code, I find that I get the
> > error that C99 does not allow implicit function declaration where
> > trace_printf() is encountered. However, upon including trace.h the
> > error disappears, and the build proceeds just fine.
> >
> > I did put DEVELOPER=1 in config.mak before building, but it doesn't
> > seem to work.
> >
> > Is the error pointing to a problem, or am I doing something wrong?
> > If it is the former, I would be very happy to send a patch fixing this.
>
> Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> very recently a patch to clean up some headers which probably were
> implicitly including trace.h when I wrote this walkthrough. Patches
> totally welcome - and if you were working from the reference code in
> https://github.com/nasamuffin/git/tree/myfirstrevwalk
bah, wrong link, the tutorial points to branch `revwalk` instead of
`myfirstrevwalk`, but the offer stands :)
> and it's on your
> way to rebase and fix that too, I'm happy to update my branch
> accordingly too. (If you weren't, don't worry about doing the extra
> work, though.)
>
> >
> > Thanks a lot!
> > Vinayak
^ permalink raw reply
* Re: [PATCH v3] docs: add git hash-object -t option's possible values
From: Junio C Hamano @ 2023-06-29 18:15 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, brian m. carlson, Taylor Blau, John Cai
In-Reply-To: <pull.1533.v3.git.git.1688004473941.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> The summary under the NAME section for git hash-object can mislead
> readers to conclude that the command can only be used to create blobs,
> whereas the description makes it clear that it can be used to create
> objects, not just blobs. Let's clarify the one-line summary.
>
> Further, the description for the option -t does not list out other types
> that can be used when creating objects. Let's make this explicit by
> listing out the different object types.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
Looks good. Queued. Let's merge it down to 'next' and below.
Thanks.
^ permalink raw reply
* [PATCH] fsck: avoid misleading variable name
From: Eric Sunshine @ 2023-06-29 18:13 UTC (permalink / raw)
To: git; +Cc: Jeff King, Eric Sunshine
From: Eric Sunshine <sunshine@sunshineco.com>
When reporting a problem, `git fsck` emits a message such as:
missing blob 1234abcd (:file)
However, this can be ambiguous when the problem is detected in the index
of a worktree other than the one in which `git fsck` was invoked. To
address this shortcoming, 592ec63b38 (fsck: mention file path for index
errors, 2023-02-24) enhanced the output to mention the path of the index
when the problem is detected in some other worktree:
missing blob 1234abcd (.git/worktrees/wt/index:file)
Unfortunately, the variable in fsck_index() which controls whether the
index path should be shown is misleadingly named "is_main_index" which
can be misunderstood as referring to the main worktree (i.e. the one
housing the .git/ repository) rather than to the current worktree (i.e.
the one in which `git fsck` was invoked). Avoid such potential confusion
by choosing a name more reflective of its actual purpose.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
The associated discussion which led to this patch begins at [1].
[1]: https://lore.kernel.org/git/305ccc55-25e3-6b01-cd86-9a9035839d06@sunshineco.com/
builtin/fsck.c | 4 ++--
t/t1450-fsck.sh | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index d9aa4db828..0c00920703 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -808,7 +808,7 @@ static int fsck_resolve_undo(struct index_state *istate,
}
static void fsck_index(struct index_state *istate, const char *index_path,
- int is_main_index)
+ int is_current_worktree)
{
unsigned int i;
@@ -830,7 +830,7 @@ static void fsck_index(struct index_state *istate, const char *index_path,
obj->flags |= USED;
fsck_put_object_name(&fsck_walk_options, &obj->oid,
"%s:%s",
- is_main_index ? "" : index_path,
+ is_current_worktree ? "" : index_path,
istate->cache[i]->name);
mark_object_reachable(obj);
}
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8c442adb1a..5805d47eb9 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -1036,9 +1036,9 @@ test_expect_success 'fsck detects problems in worktree index' '
test_cmp expect actual
'
-test_expect_success 'fsck reports problems in main index without filename' '
+test_expect_success 'fsck reports problems in current worktree index without filename' '
test_when_finished "rm -f .git/index && git read-tree HEAD" &&
- echo "this object will be removed to break the main index" >file &&
+ echo "this object will be removed to break current worktree index" >file &&
git add file &&
blob=$(git rev-parse :file) &&
remove_object $blob &&
--
2.41.0.362.gccff93557d
^ permalink raw reply related
* Re: [PATCH 3/3] fsck: mention file path for index errors
From: Eric Sunshine @ 2023-06-29 18:21 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Johannes Sixt
In-Reply-To: <20230511170133.GA1977634@coredump.intra.peff.net>
On Thu, May 11, 2023 at 1:01 PM Jeff King <peff@peff.net> wrote:
> On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:
> > Yes, s/main/current/ probably would be helpful for future readers of
> > the code. It's unfortunate that the term "current" can ambiguously
> > also be read as meaning "the up-to-date index" or "the present-time
> > index" as opposed to "the index in this directory/worktree", which is
> > the intention here. But "current" is consistent with the existing
> > `struct worktree.is_current`, so hopefully should not be too
> > confusing.
>
> I think in this context it should be pretty clear. Do you want to
> prepare a patch?
Done. As usual, I forgot to use --in-reply-to=<this-thread> when
sending the patch despite having gone through the effort of looking up
the relevant message-ID of this thread. Oh well. The patch is here[1].
[1]: https://lore.kernel.org/git/20230629181333.87465-1-ericsunshine@charter.net/
^ permalink raw reply
* Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()
From: Vinayak Dev @ 2023-06-29 18:45 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
In-Reply-To: <CAJoAoZnVAe3kvUdPZmanbKffG7cx3Tc-==H4+FH=L5qQP2smEg@mail.gmail.com>
> On Thu, Jun 29, 2023 at 9:33 AM Emily Shaffer <nasamuffin@google.com> wrote:
Hi, thanks for replying!
> > Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> > very recently a patch to clean up some headers which probably were
> > implicitly including trace.h when I wrote this walkthrough. Patches
> > totally welcome - and if you were working from the reference code in
> > https://github.com/nasamuffin/git/tree/myfirstrevwalk
>
> bah, wrong link, the tutorial points to branch `revwalk` instead of
> `myfirstrevwalk`, but the offer stands :)
>
> > and it's on your
> > way to rebase and fix that too, I'm happy to update my branch
> > accordingly too. (If you weren't, don't worry about doing the extra
> > work, though.)
Sure will! But do you mean open a PR on your fork? I have the patch ready,
and would be very happy to do so, if it is accepted!
Thanks a lot!
Vinayak
^ permalink raw reply
* [PATCH] docs: include "trace.h" in MyFirstObjectWalk.txt
From: Vinayak Dev @ 2023-06-29 18:52 UTC (permalink / raw)
To: nasamuffin; +Cc: git, Vinayak Dev
In Documentation/MyFirstObjectWalk.txt, the function
trace_printf() is used to enable trace output.
However, the file "trace.h" is not included, which
produces an error when the code from the tutorial is
compiled, with the compiler complaining that the
function is not defined before usage. Therefore, add
an include for "trace.h" in the tutorial.
Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>
---
Documentation/MyFirstObjectWalk.txt | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index eee513e86f..c3a23eb100 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -41,6 +41,7 @@ Open up a new file `builtin/walken.c` and set up the command handler:
*/
#include "builtin.h"
+#include "trace.h"
int cmd_walken(int argc, const char **argv, const char *prefix)
{
@@ -49,12 +50,13 @@ int cmd_walken(int argc, const char **argv, const char *prefix)
}
----
-NOTE: `trace_printf()` differs from `printf()` in that it can be turned on or
-off at runtime. For the purposes of this tutorial, we will write `walken` as
-though it is intended for use as a "plumbing" command: that is, a command which
-is used primarily in scripts, rather than interactively by humans (a "porcelain"
-command). So we will send our debug output to `trace_printf()` instead. When
-running, enable trace output by setting the environment variable `GIT_TRACE`.
+NOTE: `trace_printf()`, defined in `trace.h`, differs from `printf()` in
+that it can be turned on or off at runtime. For the purposes of this
+tutorial, we will write `walken` as though it is intended for use as
+a "plumbing" command: that is, a command which is used primarily in
+scripts, rather than interactively by humans (a "porcelain" command).
+So we will send our debug output to `trace_printf()` instead.
+When running, enable trace output by setting the environment variable `GIT_TRACE`.
Add usage text and `-h` handling, like all subcommands should consistently do
(our test suite will notice and complain if you fail to do so).
base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 2/3] split-index: accept that a base index can be empty
From: Junio C Hamano @ 2023-06-29 19:02 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <81118ce106222993ef17586fb0f249d8319f3b90.1688044991.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We are about to fix an ancient bug where `do_read_index()` pretended
> that the index was not initialized when there are no index entries.
>
> Before the `index_state` structure gained the `initialized` flag in
> 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from
> read_cache(), 2008-08-23), that was the best we could do (even if it was
> incorrect: it is totally possible to read a Git index file that contains
> no index entries).
Yeah, I very much remember how that single bit made our live much
easier.
> This pattern was repeated also in 998330ac2e7 (read-cache: look for
> shared index files next to the index, too, 2021-08-26), which we fix
> here by _not_ mistaking an empty base index for a missing
> `sharedindex.*` file.
Ahh, this is in the codepath to deal with a separate worktree. We
allow sharing of the "sharedindex.*" file across worktrees and
entries read from the "index" files from individual worktrees to
overlay it. But we also do allow worktrees to have their own
"sharedindex.*" file, which is what the commit in question wanted to
do, and the way it (wanted to) implement was
- check the "gitdir" version first, as before
- if that did not exist, then look at the one next to "index"
but "if that did not exist" was implemented incorrectly and did not
account for the case where that "gitdir" version was an empty index.
So, instead, updated code checks and reads the "gitdir" version *if*
the file exists, regardless of how many entries there are in it.
Makes sense.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> read-cache.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index b10caa9831c..e15a472f54f 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2455,12 +2455,14 @@ int read_index_from(struct index_state *istate, const char *path,
>
> base_oid_hex = oid_to_hex(&split_index->base_oid);
> base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
> - trace2_region_enter_printf("index", "shared/do_read_index",
> - the_repository, "%s", base_path);
> - ret = do_read_index(split_index->base, base_path, 0);
> - trace2_region_leave_printf("index", "shared/do_read_index",
> - the_repository, "%s", base_path);
> - if (!ret) {
> + if (file_exists(base_path)) {
> + trace2_region_enter_printf("index", "shared/do_read_index",
> + the_repository, "%s", base_path);
> +
> + ret = do_read_index(split_index->base, base_path, 0);
> + trace2_region_leave_printf("index", "shared/do_read_index",
> + the_repository, "%s", base_path);
> + } else {
> char *path_copy = xstrdup(path);
> char *base_path2 = xstrfmt("%s/sharedindex.%s",
> dirname(path_copy), base_oid_hex);
^ permalink raw reply
* Re: [PATCH] fsck: avoid misleading variable name
From: Jeff King @ 2023-06-29 19:04 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Eric Sunshine
In-Reply-To: <20230629181333.87465-1-ericsunshine@charter.net>
On Thu, Jun 29, 2023 at 02:13:33PM -0400, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> When reporting a problem, `git fsck` emits a message such as:
>
> missing blob 1234abcd (:file)
>
> However, this can be ambiguous when the problem is detected in the index
> of a worktree other than the one in which `git fsck` was invoked. To
> address this shortcoming, 592ec63b38 (fsck: mention file path for index
> errors, 2023-02-24) enhanced the output to mention the path of the index
> when the problem is detected in some other worktree:
>
> missing blob 1234abcd (.git/worktrees/wt/index:file)
>
> Unfortunately, the variable in fsck_index() which controls whether the
> index path should be shown is misleadingly named "is_main_index" which
> can be misunderstood as referring to the main worktree (i.e. the one
> housing the .git/ repository) rather than to the current worktree (i.e.
> the one in which `git fsck` was invoked). Avoid such potential confusion
> by choosing a name more reflective of its actual purpose.
This looks good to me. Thanks for following up!
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] commit -a -m: allow the top-level tree to become empty again
From: Junio C Hamano @ 2023-06-29 19:17 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <08c50b64e2a93300eed196505936e58ce8bb639b.1688044991.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> That logic was introduced to add a shortcut when committing without
> editing the commit message interactively. A part of that logic was to
> ensure that the index was read into memory:
>
> if (!active_nr && read_cache() < 0)
> die(...)
>
> Translation to English: If the index has not yet been read, read it, and
> if that fails, error out.
Well described. It does make sense to turn !active_nr used here
into a check on the .initialized member.
> And it was natural to do it this way because at the time that condition
> was introduced, the `index_state` structure had no explicit flag to
> indicate that it was initialized: This flag was only introduced in
> 913e0e99b6a (unpack_trees(): protect the handcrafted in-core index from
> read_cache(), 2008-08-23), but that commit did not adjust the code path
> where no index file was found and a new, pristine index was initialized.
My mistake, but after 15 years it probably is beyond statute of
limitations ;-)
> Using the `initialized` flag instead, we avoid that mistake, and as a
> bonus we can fix a bug at the same time that was introduced by the
> memory leak fix: When deleting all tracked files and then asking `git
> commit -a -m ...` to commit the result, Git would internally update the
> index, then discard and re-read the index undoing the update, and fail
> to commit anything.
That does sound like the primary bug fixed with this change, not a
bonus, but anyway, the change is very sensible and clearly described
with a good test. Will queue.
Thanks.
^ permalink raw reply
* Re: Documentation/MyFirstObjectWalk: add #include "trace.h" to use trace_printf()
From: Junio C Hamano @ 2023-06-29 19:28 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Vinayak Dev, git
In-Reply-To: <CAJoAoZ=OEfsgkqsag926tH4GEuafX26A09SGZ1vR1uLh2W_4TA@mail.gmail.com>
Emily Shaffer <nasamuffin@google.com> writes:
> Yeah, it's almost certainly stale in MyFirstObjectWalk - there was
> very recently a patch to clean up some headers which probably were
> implicitly including trace.h when I wrote this walkthrough.
We are lucky that we have folks like Vinayak who tried out the
examples and then bothered to spend time reporting the failure
discovered. What does it take, however, for us to have a bit more
automated way to prevent such a breakage that comes from API
changes? Is it feasible, for example, to add a test that extracts
code snippets from the MyFirstObjectWalk document and try to build
the result? Alternatively, we can ship such a set of sample source
files somewhere in our tree (e.g. contrib/examples?) and have such
a test try to build using the current set of source files, but then
we need a mechansim to ensure that the sample source files will not
go out of sync with the document.
Thoughts?
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] fsck: mention file path for index errors
From: Junio C Hamano @ 2023-06-29 19:37 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Jeff King, Git Mailing List, Johannes Sixt
In-Reply-To: <CAPig+cSeQKr-MNN7_44wuGBCYDMm8H+1mi+X6dd-0p2DkFY2sg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Thu, May 11, 2023 at 1:01 PM Jeff King <peff@peff.net> wrote:
>> On Thu, May 11, 2023 at 12:28:45PM -0400, Eric Sunshine wrote:
>> > Yes, s/main/current/ probably would be helpful for future readers of
>> > the code. It's unfortunate that the term "current" can ambiguously
>> > also be read as meaning "the up-to-date index" or "the present-time
>> > index" as opposed to "the index in this directory/worktree", which is
>> > the intention here. But "current" is consistent with the existing
>> > `struct worktree.is_current`, so hopefully should not be too
>> > confusing.
>>
>> I think in this context it should be pretty clear. Do you want to
>> prepare a patch?
>
> Done. As usual, I forgot to use --in-reply-to=<this-thread> when
> sending the patch despite having gone through the effort of looking up
> the relevant message-ID of this thread. Oh well. The patch is here[1].
>
> [1]: https://lore.kernel.org/git/20230629181333.87465-1-ericsunshine@charter.net/
I've queued your patch on top of the jk/fsck-indices-in-worktrees
topic as-is, but the earlier discussion in the thread shows that
Peff already is in agreement with the change, so I would not mind
amending in his Acked-by: later.
Thanks, anyway.
^ permalink raw reply
* Re: [RFC PATCH v3 1/1] unit tests: Add a project plan document
From: Linus Arver @ 2023-06-29 19:42 UTC (permalink / raw)
To: Josh Steadmon, git
Cc: calvinwan, szeder.dev, phillip.wood123, chooglen, avarab, gitster,
sandals
In-Reply-To: <8afdb215d7e10ca16a2ce8226b4127b3d8a2d971.1686352386.git.steadmon@google.com>
Hello,
Josh Steadmon <steadmon@google.com> writes:
> In our current testing environment, we spend a significant amount of
> effort crafting end-to-end tests for error conditions that could easily
> be captured by unit tests (or we simply forgo some hard-to-setup and
> rare error conditions).Describe what we hope to accomplish by
I see a minor typo (no space before the word "Describe").
> +=== Comparison
> +
> +[format="csv",options="header",width="75%"]
> +|=====
> +Framework,"TAP support","Diagnostic output","Parallel execution","Vendorable / ubiquitous","Maintainable / extensible","Major platform support","Lazy test planning","Runtime- skippable tests","Scheduling / re-running",Mocks,"Signal & exception handling","Coverage reports"
> +https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[Custom Git impl.],[lime-background]#True#,[lime-background]#True#,?,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,?,?,[red-background]#False#,?,?
> +https://cmocka.org/[cmocka],[lime-background]#True#,[lime-background]#True#,?,[red-background]#False#,[yellow-background]#Partial#,[yellow-background]#Partial#,[yellow-background]#Partial#,?,?,[lime-background]#True#,?,?
> +https://libcheck.github.io/check/[Check],[lime-background]#True#,[lime-background]#True#,?,[red-background]#False#,[yellow-background]#Partial#,[lime-background]#True#,[yellow-background]#Partial#,?,?,[red-background]#False#,?,?
> +https://github.com/rra/c-tap-harness/[C TAP],[lime-background]#True#,[red-background]#False#,?,[lime-background]#True#,[yellow-background]#Partial#,[yellow-background]#Partial#,[yellow-background]#Partial#,?,?,[red-background]#False#,?,?
> +https://github.com/silentbicycle/greatest[Greatest],[yellow-background]#Partial#,?,?,[lime-background]#True#,[yellow-background]#Partial#,?,[yellow-background]#Partial#,?,?,[red-background]#False#,?,?
> +https://github.com/Snaipe/Criterion[Criterion],[lime-background]#True#,?,?,[red-background]#False#,?,[lime-background]#True#,?,?,?,[red-background]#False#,?,?
> +https://github.com/zorgnax/libtap[libtap],[lime-background]#True#,?,?,?,?,?,?,?,?,?,?,?
> +https://nemequ.github.io/munit/[µnit],?,?,?,?,?,?,?,?,?,?,?,?
> +https://github.com/google/cmockery[cmockery],?,?,?,?,?,?,?,?,?,[lime-background]#True#,?,?
> +https://github.com/lpabon/cmockery2[cmockery2],?,?,?,?,?,?,?,?,?,[lime-background]#True#,?,?
> +https://github.com/ThrowTheSwitch/Unity[Unity],?,?,?,?,?,?,?,?,?,?,?,?
> +https://github.com/siu/minunit[minunit],?,?,?,?,?,?,?,?,?,?,?,?
> +https://cunit.sourceforge.net/[CUnit],?,?,?,?,?,?,?,?,?,?,?,?
> +https://www.kindahl.net/mytap/doc/index.html[MyTAP],[lime-background]#True#,?,?,?,?,?,?,?,?,?,?,?
> +|=====
This table is a little hard to read. Do you have your patch on GitHub or
somewhere else where this table is rendered with HTML?
It would help to explain each of the answers that are filled in
with the word "Partial", to better understand why it is the case. I
suspect this might get a little verbose, in which case I suggest just
giving each framework its own heading.
The column names here are slightly different from the headings used
under "Desired features"; I suggest making them the same.
Also, how about grouping some of these together? For example "Diagnostic
output" and "Coverage reports" feel like they could be grouped under
"Output formats". Here's one way to group these:
1. Output formats
TAP support
Diagnostic output
Coverage reports
2. Cost of adoption
Vendorable / ubiquitous
Maintainable / extensible
Major platform support
3. Performance flexibility
Parallel execution
Lazy test planning
Runtime-skippable tests
Scheduling / re-running
4. Developer experience
Mocks
Signal & exception handling
I can think of some other metrics to add to the comparison, namely:
1. Age (how old is the framework)
2. Size in KLOC (thousands of lines of code)
3. Adoption rate (which notable C projects already use this framework?)
4. Project health (how active are its developers?)
I think for 3 and 4, we could probably mine some data out of GitHub
itself.
Lastly it would be helpful if we can mark some of these categories as
must-haves. For example would lack of "Major platform support" alone
disqualify a test framework? This would help fill in the empty bits in
the comparison table because we could skip looking too deeply into a
framework if it fails to meet a must-have requirement.
Thanks,
Linus
^ 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