* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Konstantin Khomoutov @ 2023-01-13 15:30 UTC (permalink / raw)
To: git; +Cc: Hans Petter Selasky
In-Reply-To: <446984f6-0d2e-04da-11a3-8b1481fac953@selasky.org>
On Fri, Jan 13, 2023 at 02:39:37PM +0100, Hans Petter Selasky wrote:
[...]
> > It's not clear why are you referring to Gitorious in your mail's subject and
> > then talk about Git.
[...]
> I thought that Git was short for Gitorious? My bad.
No, unless you're late to the Git party ;-)
Old-timers do remember Gitorious as a software project [1] which is closely
related to Git but was a totally separate project.
1. https://en.wikipedia.org/wiki/Gitorious
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 16:06 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: rsbecker, git
In-Reply-To: <20230113160218.d3nsoxpbrxrrszhz@meerkat.local>
On 1/13/23 17:02, Konstantin Ryabitsev wrote:
> On Fri, Jan 13, 2023 at 04:54:39PM +0100, Hans Petter Selasky wrote:
>> On 1/13/23 16:45, Konstantin Ryabitsev wrote:
>>> If, for some reason, Linus ever needs to remove something
>>> from linux.git, he will do it and just give a heads-up why and for what
>>> reason.
>>
>> This gotta be a joke.
>>
>> There are 46K forks of Linus Torvalds Linux kernel on GitHUB, and if Linus
>> Torvalds one day decides to do a forced push, it will for sure be a
>> disaster!
>
> No it won't, and I speak from some position of authority on this subject (I'm
> responsible for git.kernel.org).
>
> If Linus has to alter the history of linux.git, it will for sure be an
> extraordinary event -- it's never happened yet. However, it will be widely
> publicised, the reasons for it will be made clear, and everyone will just
> accept it and move on.
>
> Git history edits occur all the time. Most tooling expects this to
> occasionally happen and deals with it correctly.
>
OK, if you say so. Though in my mind 46K rebases of millions of commits
seem a lot overhead.
However, if history can be edited anyway, why do you need the
cryptographic hash algorithm. Why not use a non-cryptographic one?
What's the point? Only so that one party can stay in control?
--HPS
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 16:02 UTC (permalink / raw)
To: rsbecker, 'Konstantin Ryabitsev'; +Cc: git
In-Reply-To: <00b201d92767$a0403ee0$e0c0bca0$@nexbridge.com>
On 1/13/23 16:56, rsbecker@nexbridge.com wrote:
> git is using SHA1/SHA256 (which happen to be coincidentally cryptographic) as message digests with a very low probability of collisions when the hashes are computed. There is never a situation, implied by cryptography, where there is a decode of a git hash. In order to make git a blockchain, you would need to implement central signing authorities, which would require a fork if the signature mechanism changes. The signature mechanism (SSH, GPG) is distinct from hash computation in git's trees, but depends on hash integrity.
I see.
But at the same time any unique enough hash, identifies a specific piece
of code or checkout, even though it is not under a specific signing
authority. And that is the problem, that authorities may distribute
allowed-only-hashes for their hardware ...
--HPS
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Konstantin Ryabitsev @ 2023-01-13 16:02 UTC (permalink / raw)
To: Hans Petter Selasky; +Cc: rsbecker, git
In-Reply-To: <d087568b-919e-61f8-c203-e59a2e0572c6@selasky.org>
On Fri, Jan 13, 2023 at 04:54:39PM +0100, Hans Petter Selasky wrote:
> On 1/13/23 16:45, Konstantin Ryabitsev wrote:
> > If, for some reason, Linus ever needs to remove something
> > from linux.git, he will do it and just give a heads-up why and for what
> > reason.
>
> This gotta be a joke.
>
> There are 46K forks of Linus Torvalds Linux kernel on GitHUB, and if Linus
> Torvalds one day decides to do a forced push, it will for sure be a
> disaster!
No it won't, and I speak from some position of authority on this subject (I'm
responsible for git.kernel.org).
If Linus has to alter the history of linux.git, it will for sure be an
extraordinary event -- it's never happened yet. However, it will be widely
publicised, the reasons for it will be made clear, and everyone will just
accept it and move on.
Git history edits occur all the time. Most tooling expects this to
occasionally happen and deals with it correctly.
-K
^ permalink raw reply
* RE: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: rsbecker @ 2023-01-13 15:56 UTC (permalink / raw)
To: 'Hans Petter Selasky', 'Konstantin Ryabitsev'; +Cc: git
In-Reply-To: <6061d012-13b7-ca4b-5556-70875b65c887@selasky.org>
On January 13, 2023 10:50 AM, Hans Petter Selasky wrote:
>On 1/13/23 16:45, Konstantin Ryabitsev wrote:
>> I think you're misunderstanding some of the core principles of git.
>
>Maybe, I'm usually commandering git via the terminal.
>
>But if you say you can already edit stuff, why does the commit hash need to be
>cryptographic? I don't get that part. Yeah, I think of git commits like blockchain.
git is using SHA1/SHA256 (which happen to be coincidentally cryptographic) as message digests with a very low probability of collisions when the hashes are computed. There is never a situation, implied by cryptography, where there is a decode of a git hash. In order to make git a blockchain, you would need to implement central signing authorities, which would require a fork if the signature mechanism changes. The signature mechanism (SSH, GPG) is distinct from hash computation in git's trees, but depends on hash integrity.
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 15:54 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: rsbecker, git
In-Reply-To: <20230113154516.jxm2cer4sogatayp@meerkat.local>
On 1/13/23 16:45, Konstantin Ryabitsev wrote:
> If, for some reason, Linus ever needs to remove something
> from linux.git, he will do it and just give a heads-up why and for what
> reason.
This gotta be a joke.
There are 46K forks of Linus Torvalds Linux kernel on GitHUB, and if
Linus Torvalds one day decides to do a forced push, it will for sure be
a disaster!
--HPS
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 15:50 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: rsbecker, git
In-Reply-To: <20230113154516.jxm2cer4sogatayp@meerkat.local>
On 1/13/23 16:45, Konstantin Ryabitsev wrote:
> I think you're misunderstanding some of the core principles of git.
Maybe, I'm usually commandering git via the terminal.
But if you say you can already edit stuff, why does the commit hash need
to be cryptographic? I don't get that part. Yeah, I think of git commits
like blockchain.
--HPS
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Konstantin Ryabitsev @ 2023-01-13 15:45 UTC (permalink / raw)
To: Hans Petter Selasky; +Cc: rsbecker, git
In-Reply-To: <8a8fbe42-7809-f3e7-b233-6bef790254e1@selasky.org>
On Fri, Jan 13, 2023 at 03:42:48PM +0100, Hans Petter Selasky wrote:
> > I do not understand the goal of this request. If it is possible to forge
> > hashes, then nothing in a git repository can ever be trusted. Signed
> > content will no longer be verifiable. The whole Merkel Tree representing
> > the commit history becomes easily corruptible by hackers and no upstream
> > remote repository can ever be trusted - or someone's own if someone
> > targets a repo with malware that rewrites hashes. Imagine a scenario when
> > malware replaces a blob in a repo and then forges the hash to pretend that
> > the replacement never occurred. Using git as a supply chain audit trail
> > becomes impossible. This is a potential vector for ransomware invading the
> > git ecosystem. This seems like a really fatal path to take for the
> > product.
>
> If a hacker replaces a blob, everyone on the project will see it, because
> such changes typically generate a commit e-mail.
I don't think you have a very clear picture of how git works.
> And then an action will be made to revoke the access of that hacker. Now a
> clever hacker wouldn't do that. A clever hacker would just flip one bit
> somewhere in a random blob, looking like a hardware fault, and then force
> the project to rewind to backups every day, because the repository can no
> longer be verified.
That's not how it works at all. If there is a corrupted object, the admins of
the repository just put the correct object into place either from a backup or
from another copy of the repository. There is no rewinding required.
> There is no advantage from protecting from hardware errors, unless you can
> recover from them! Cryptographic hash algorithms are not suitable to recover
> bits. They only tell data is OK or NOK, and if there is no backup, you loose
> it!
This is true about all digital media.
> It is no solution for big repositories to rewind to backups just because
> of bit-flips. Such problems should be fixed w/o the need to roll-back,
> because that stops the entire production!
No it doesn't.
> > it can be repaired by a push --force
>
> Hobby projects can do that, but not big projects like FreeBSD and the Linux
> kernel.
Sure they can, but not due to missing objects (a corrupted object is just a
missing object). If, for some reason, Linus ever needs to remove something
from linux.git, he will do it and just give a heads-up why and for what
reason.
I think you're misunderstanding some of the core principles of git.
-K
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Konstantin Ryabitsev @ 2023-01-13 15:39 UTC (permalink / raw)
To: Hans Petter Selasky; +Cc: git
In-Reply-To: <446984f6-0d2e-04da-11a3-8b1481fac953@selasky.org>
On Fri, Jan 13, 2023 at 02:39:37PM +0100, Hans Petter Selasky wrote:
> Just imagine the consequences of finding child porn inside a 10-year old
> firmware binary blob in the Linux kernel. Will you just ignore it, or will
> you fix it?
How do you expect something like this would happen? A much more likely
scenario would be someone contributing a binary blob that doesn't actually
allow redistribution, and therefore would need to be purged from the
repository.
When something like this happens, everyone is given a heads-up, the history is
rewritten, and everyone moves on. It's a fairly routine procedure -- ask
anyone who's ever committed an API key into their repo.
Git supports history edits and everyone lives with it just fine -- I think you
are under the impression that git is some kind of globally distributed
blockchain where any history edit requires a consensus fork. It's not at all
the case.
-K
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 15:15 UTC (permalink / raw)
To: rsbecker, git
In-Reply-To: <009701d9275a$678416b0$368c4410$@nexbridge.com>
On 1/13/23 15:21, rsbecker@nexbridge.com wrote:
> Signed content will no longer be verifiable. The whole Merkel Tree representing the commit history becomes easily corruptible by hackers
Hi,
As a long time open sourcer and hacker, I'm totally against signing
software. Is the GIT project going to build the new infrastructure for
John-Deers new tractor firmware adventure? It is totally against the
values of open source craftmanship.
I don't think any of you crypto-enthusiasts understand how propritary
companies use signed software to keep their power intact.
That's also an argument for using a non-crypto hash.
--HPS
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 14:42 UTC (permalink / raw)
To: rsbecker, git
In-Reply-To: <009701d9275a$678416b0$368c4410$@nexbridge.com>
On 1/13/23 15:21, rsbecker@nexbridge.com wrote:
> On January 13, 2023 8:40 AM, Hans Petter Selasky wrote:
>> On 1/13/23 14:30, Konstantin Khomoutov wrote:
>>> On Fri, Jan 13, 2023 at 01:59:44PM +0100, Hans Petter Selasky wrote:
>>>
>>>> Currently GIT only supports cryptographic hashes for its commit tags.
>>> [...]
>>>
>>> https://github.com/git/git/blob/9bf691b78cf906751e65d65ba0c6ffdcd9a5a1
>>> 2c/Documentation/technical/hash-function-transition.txt
>>>
>>> It's not clear why are you referring to Gitorious in your mail's
>>> subject and then talk about Git.
>>>
>>
>> Hi,
>>
>> I thought that Git was short for Gitorious? My bad.
>>
>> The document you refer to really highlights my concerns, that a strong
>> cryptographic hash algorithm is the highway to hell.
>>
>> Do _not_ use a cryptographic hash for Git. Use plain good old CRC hashes.
>>
>> Just imagine the consequences of finding child porn inside a 10-year old firmware
>> binary blob in the Linux kernel. Will you just ignore it, or will you fix it?
>>
>> That's why I say, that it must be possible to forge the hashes by default.
>
Hi,
> I do not understand the goal of this request. If it is possible to forge hashes, then nothing in a git repository can ever be trusted. Signed content will no longer be verifiable. The whole Merkel Tree representing the commit history becomes easily corruptible by hackers and no upstream remote repository can ever be trusted - or someone's own if someone targets a repo with malware that rewrites hashes. Imagine a scenario when malware replaces a blob in a repo and then forges the hash to pretend that the replacement never occurred. Using git as a supply chain audit trail becomes impossible. This is a potential vector for ransomware invading the git ecosystem. This seems like a really fatal path to take for the product.
If a hacker replaces a blob, everyone on the project will see it,
because such changes typically generate a commit e-mail. And then an
action will be made to revoke the access of that hacker. Now a clever
hacker wouldn't do that. A clever hacker would just flip one bit
somewhere in a random blob, looking like a hardware fault, and then
force the project to rewind to backups every day, because the repository
can no longer be verified.
> The advantage of how git functions is that it is possible to mirror or clone repositories, protecting from hardware errors. Repositories exist in distributed form, so there may be hundreds or thousands of copies in case someone's copy is corrupted by a disk or memory write error - so that takes hash reconstruction out of the requirement set. If the git architecture was based on a central repository model only, then this might be a reasonable request, but that is not how git works. If, for instance, a main GitHub repo is somehow corrupted, it can be repaired by a push --force or a clone from a different instance.
>
There is no advantage from protecting from hardware errors, unless you
can recover from them! Cryptographic hash algorithms are not suitable to
recover bits. They only tell data is OK or NOK, and if there is no
backup, you loose it! It is no solution for big repositories to rewind
to backups just because of bit-flips. Such problems should be fixed w/o
the need to roll-back, because that stops the entire production!
> it can be repaired by a push --force
Hobby projects can do that, but not big projects like FreeBSD and the
Linux kernel.
> Unless I am missing your point.
Yes, a little bit :-)
--HPS
^ permalink raw reply
* RE: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: rsbecker @ 2023-01-13 14:21 UTC (permalink / raw)
To: 'Hans Petter Selasky', git
In-Reply-To: <446984f6-0d2e-04da-11a3-8b1481fac953@selasky.org>
On January 13, 2023 8:40 AM, Hans Petter Selasky wrote:
>On 1/13/23 14:30, Konstantin Khomoutov wrote:
>> On Fri, Jan 13, 2023 at 01:59:44PM +0100, Hans Petter Selasky wrote:
>>
>>> Currently GIT only supports cryptographic hashes for its commit tags.
>> [...]
>>
>> https://github.com/git/git/blob/9bf691b78cf906751e65d65ba0c6ffdcd9a5a1
>> 2c/Documentation/technical/hash-function-transition.txt
>>
>> It's not clear why are you referring to Gitorious in your mail's
>> subject and then talk about Git.
>>
>
>Hi,
>
>I thought that Git was short for Gitorious? My bad.
>
>The document you refer to really highlights my concerns, that a strong
>cryptographic hash algorithm is the highway to hell.
>
>Do _not_ use a cryptographic hash for Git. Use plain good old CRC hashes.
>
>Just imagine the consequences of finding child porn inside a 10-year old firmware
>binary blob in the Linux kernel. Will you just ignore it, or will you fix it?
>
>That's why I say, that it must be possible to forge the hashes by default.
I do not understand the goal of this request. If it is possible to forge hashes, then nothing in a git repository can ever be trusted. Signed content will no longer be verifiable. The whole Merkel Tree representing the commit history becomes easily corruptible by hackers and no upstream remote repository can ever be trusted - or someone's own if someone targets a repo with malware that rewrites hashes. Imagine a scenario when malware replaces a blob in a repo and then forges the hash to pretend that the replacement never occurred. Using git as a supply chain audit trail becomes impossible. This is a potential vector for ransomware invading the git ecosystem. This seems like a really fatal path to take for the product.
The advantage of how git functions is that it is possible to mirror or clone repositories, protecting from hardware errors. Repositories exist in distributed form, so there may be hundreds or thousands of copies in case someone's copy is corrupted by a disk or memory write error - so that takes hash reconstruction out of the requirement set. If the git architecture was based on a central repository model only, then this might be a reasonable request, but that is not how git works. If, for instance, a main GitHub repo is somehow corrupted, it can be repaired by a push --force or a clone from a different instance.
Unless I am missing your point.
--Randall
^ permalink raw reply
* Re: [PATCH] ls-tree: add --sparse-filter-oid argument
From: Eric Sunshine @ 2023-01-13 14:17 UTC (permalink / raw)
To: William Sprent via GitGitGadget; +Cc: git, William Sprent
In-Reply-To: <pull.1459.git.1673456518993.gitgitgadget@gmail.com>
On Wed, Jan 11, 2023 at 12:05 PM William Sprent via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> To fill this gap, add a new argument to ls-tree '--sparse-filter-oid'
> which takes the object id of a blob containing sparse checkout patterns
> that filters the output of 'ls-tree'. When filtering with given sparsity
> patterns, 'ls-tree' only outputs blobs and commit objects that
> match the given patterns.
> [...]
> Signed-off-by: William Sprent <williams@unity3d.com>
This is not a proper review, but rather just some comments about
issues I noticed while quickly running my eye over the patch. Many are
just micro-nits about style; a few regarding the tests are probably
actionable.
> Note that one of the tests only pass when run on top of commit
> 5842710dc2 (dir: check for single file cone patterns, 2023-01-03), which
> was submitted separately and is currently is merged to 'next'.
Thanks for mentioning this. It's exactly the sort of information the
maintainer needs when applying your patch to his tree. And it can be
helpful for reviewers too.
> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
> @@ -48,6 +48,11 @@ OPTIONS
> +--sparse-filter-oid=<blob-ish>::
> + Omit showing tree objects and paths that do not match the
> + sparse-checkout specification contained in the blob
> + (or blob-expression) <blob-ish>.
Good to see a documentation update. The SYNOPSIS probably deserves an
update too.
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> @@ -329,12 +331,79 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {
> +static void sparse_filter_data__init(
> + struct sparse_filter_data **d,
> + const char *sparse_oid_name, read_tree_fn_t fn)
> +{
> + struct object_id sparse_oid;
> + struct object_context oc;
> +
> + (*d) = xcalloc(1, sizeof(**d));
> + (*d)->fn = fn;
> + (*d)->pl.use_cone_patterns = core_sparse_checkout_cone;
> +
> + strbuf_init(&(*d)->full_path_buf, 0);
> +
> +
Nit: too many blank lines.
> + if (get_oid_with_context(the_repository,
> + sparse_oid_name,
> + GET_OID_BLOB, &sparse_oid, &oc))
Pure nit: somewhat odd choice of wrapping; I'd have expected something
along the lines of:
if (get_oid_with_context(the_repository, sparse_oid_name, GET_OID_BLOB,
&sparse_oid, &oc))
or:
if (get_oid_with_context(the_repository, sparse_oid_name,
GET_OID_BLOB, &sparse_oid, &oc))
> +static void sparse_filter_data__free(struct sparse_filter_data *d)
> +{
> + clear_pattern_list(&d->pl);
> + strbuf_release(&d->full_path_buf);
> + free(d);
> +}
Is the double-underscore convention in function names imported from
elsewhere? I don't recall seeing it used in this codebase.
> +static int path_matches_sparse_checkout_patterns(struct strbuf *full_path, struct pattern_list *pl, int dtype)
> +{
> + enum pattern_match_result match = recursively_match_path_with_sparse_patterns(full_path->buf, the_repository->index, dtype, pl);
> + return match > 0;
> +}
> +
> +
Nit: too many blank lines
> +static int filter_sparse(const struct object_id *oid, struct strbuf *base,
> + const char *pathname, unsigned mode, void *context)
> +{
> +
> + struct sparse_filter_data *data = context;
Nit: unnecessary blank line after "{"
> + int do_recurse = show_recursive(base->buf, base->len, pathname);
> + if (object_type(mode) == OBJ_TREE)
> + return do_recurse;
> +
> + strbuf_reset(&data->full_path_buf);
> + strbuf_addbuf(&data->full_path_buf, base);
> + strbuf_addstr(&data->full_path_buf, pathname);
> +
> + if (!path_matches_sparse_checkout_patterns(&data->full_path_buf, &data->pl, DT_REG))
> + return 0;
> +
> + return data->fn(oid, base, pathname, mode, context);
> +}
> +
> +
Nit: too many blank lines
> diff --git a/dir.c b/dir.c
> @@ -1450,45 +1450,51 @@ int init_sparse_checkout_patterns(struct index_state *istate)
> +static int path_in_sparse_checkout_1(const char *path,
> + struct index_state *istate,
> + int require_cone_mode)
> +{
> +
> + /*
Nit: unnecessary blank line after "{"
> diff --git a/t/t3106-ls-tree-sparse-checkout.sh b/t/t3106-ls-tree-sparse-checkout.sh
> new file mode 100755
We often try to avoid introducing a new test script if the tests being
added can fit well into an existing script. If you didn't find any
existing script where these tests would fit, then creating a new
script may be fine.
> @@ -0,0 +1,103 @@
> +check_agrees_with_ls_files () {
> + REPO=repo
> + git -C $REPO submodule deinit -f --all
> + git -C $REPO cat-file -p ${filter_oid} >${REPO}/.git/info/sparse-checkout
> + git -C $REPO sparse-checkout init --cone 2>err
> + git -C $REPO submodule init
> + git -C $REPO ls-files -t| grep -v "^S "|cut -d" " -f2 >ls-files
> + test_cmp ls-files actual
> +}
Several comments:
Since the return code of this function is significant and callers care
about it, you should &&-chain all of the code in the function itself
(just like you do within a test) so that failure of any command in the
function is noticed and causes the calling test to fail. It's a good
idea to &&-chain the variable assignments at the top of the function
too (just in case someone later inserts code above the assignments).
It's odd to see a mixture of $VAR and ${VAR}. It's better to be
consistent. We typically use the $VAR form (though it's not exclusive
by any means).
Some shells complain when the pathname following ">" redirection
operator is not quoted, so:
git -C $REPO cat-file -p ${filter_oid} >"$REPO/.git/info/sparse-checkout" &&
Style: add space around "|" pipe operator
We usually avoid having a Git command upstream of the pipe since its
exit code gets swallowed by the pipe, so we usually do this instead:
git -C $REPO ls-files -t >out &&
grep -v "^S " out | cut -d" " -f2 >ls-files &&
Minor: The two-command pipeline `grep -v "^S " | cut -d" " -f2
>ls-files` could be expressed via a single `sed` invocation:
sed -n "/^S /!s/^. //p" out &&
Nit: The first argument to test_cmp() is typically named "expect".
Error output is captured to file "err" but that file is never consulted.
> +check_same_result_in_bare_repo () {
> + FULL=repo
> + BARE=bare
> + FILTER=$1
> + git -C repo cat-file -p ${filter_oid}| git -C bare hash-object -w --stdin
> + git -C bare ls-tree --name-only --filter-sparse-oid=${filter_oid} -r HEAD >bare-result
> + test_cmp expect bare-result
> +}
Same comments as above, plus:
What is the purpose of the variables FULL, BARE, FILTER? They don't
seem to be used in the function.
I suspect that the function should be using $FILTER internally rather
than $filter_oid.
> +test_expect_success 'setup' '
> + git init submodule &&
> + (
> + cd submodule &&
> + test_commit file
> + ) &&
Minor: test_commit() accepts a -C option, so this could be done
without the subshell:
test_commit -C submodule file
> + git init repo &&
> + (
> + cd repo &&
> + mkdir dir &&
> + test_commit dir/sub-file &&
> + test_commit dir/sub-file2 &&
> + mkdir dir2 &&
> + test_commit dir2/sub-file1 &&
> + test_commit dir2/sub-file2 &&
> + test_commit top-file &&
> + git clone ../submodule submodule &&
> + git submodule add ./submodule &&
> + git submodule absorbgitdirs &&
> + git commit -m"add submodule" &&
> + git sparse-checkout init --cone
> + ) &&
Here the subshell makes sense since so many commands are run in
directory "repo." Fine.
^ permalink raw reply
* Re: Bug report: checkout --recurse-submodules failing
From: Philippe Blain @ 2023-01-13 13:58 UTC (permalink / raw)
To: Carlos Gonzalez, git@vger.kernel.org
In-Reply-To: <CWXP265MB3688191D75A56F27A0C4DDB69CFD9@CWXP265MB3688.GBRP265.PROD.OUTLOOK.COM>
Hi Carlos,
Le 2023-01-12 à 07:17, Carlos Gonzalez a écrit :
> What did you do before the bug happened?
>
> Checking out a custom commit or tag using `--recurse-submodules`, where the specific commit contained a submodule
> not included in the main branch. The checkout command failed. (Detailed steps below)
>
> What did you expect to happen?
>
> The repository should be checked out to the specified commit and submodules updated, and the one missing in the main
> branch, cloned.
It's a reasonable expectation, but the current 'checkout' code does not
know how to clone missing submodules.
>
> What happened instead?
>
> git checkout --recurse-submodules submodule
> fatal: not a git repository: ../.git/modules/sycl-blas
> fatal: could not reset submodule index
>
> What's different between what you expected and what actually happened?
>
> This only fails when the repository was cloned with --recursive flag.
>
> Anything else you want to add:
>
> I wrote these simple steps to reproduce:
>
> mkdir repo1 && cd repo1
> git init
> git submodule add --name sycl-blas https://github.com/codeplaysoftware/sycl-blas.git
> git commit -m "Adding submodule"
> git tag -a submodule -m submodule
> git submodule deinit sycl-blas
> truncate -s 0 .gitmodules
> rm -rf sycl-blas/
> git add .gitmodules
> git rm sycl-blas
> git commit -m "Remove submodule"
> cd ../
>
> # When repository is cloned without `--recursive`, checkout works
> git clone repo1 cloned-repo1
> cd cloned-repo1
> git checkout --recurse-submodules submodule # ok
> cd ../
> # When repository is cloned with `--recursive`, checkout fails
> git clone --recursive repo1 cloned-repo2
> cd cloned-repo2
> git checkout --recurse-submodules submodule
> fatal: not a git repository: ../.git/modules/sycl-blas
> fatal: could not reset submodule index
Thanks for a complete reproducer. This has been reported before,
see the threads linked at [1].
I was working on that issue last month, so that at least the checkout fails
in a clean way, but have not gone back to it yet.
In the meantime, as you've discovered, a workaround is to clone without
'--recurse-submodules', then manually run 'git submodule update --init --recursive'
after switching branches.
Cheers,
Philippe.
[1] https://github.com/gitgitgadget/git/issues/752
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 13:39 UTC (permalink / raw)
To: git
In-Reply-To: <20230113133059.snyjblh3sz2wzcnd@carbon>
On 1/13/23 14:30, Konstantin Khomoutov wrote:
> On Fri, Jan 13, 2023 at 01:59:44PM +0100, Hans Petter Selasky wrote:
>
>> Currently GIT only supports cryptographic hashes for its commit tags.
> [...]
>
> https://github.com/git/git/blob/9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c/Documentation/technical/hash-function-transition.txt
>
> It's not clear why are you referring to Gitorious in your mail's subject and
> then talk about Git.
>
Hi,
I thought that Git was short for Gitorious? My bad.
The document you refer to really highlights my concerns, that a strong
cryptographic hash algorithm is the highway to hell.
Do _not_ use a cryptographic hash for Git. Use plain good old CRC hashes.
Just imagine the consequences of finding child porn inside a 10-year old
firmware binary blob in the Linux kernel. Will you just ignore it, or
will you fix it?
That's why I say, that it must be possible to forge the hashes by default.
--HPS
^ permalink raw reply
* Re: Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Konstantin Khomoutov @ 2023-01-13 13:30 UTC (permalink / raw)
To: git; +Cc: Hans Petter Selasky
In-Reply-To: <39dd1a00-786b-acf5-8a40-2425f7dab6cc@selasky.org>
On Fri, Jan 13, 2023 at 01:59:44PM +0100, Hans Petter Selasky wrote:
> Currently GIT only supports cryptographic hashes for its commit tags.
[...]
https://github.com/git/git/blob/9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c/Documentation/technical/hash-function-transition.txt
It's not clear why are you referring to Gitorious in your mail's subject and
then talk about Git.
^ permalink raw reply
* Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 13:23 UTC (permalink / raw)
To: git
Hi,
Currently GIT only supports cryptographic hashes for its commit tags.
That means:
1) It's very difficult to edit the history without also recomputing the
hash tags for all commits after the needed change-point, which then
means references to a repository is broken.
2) Only a single bit error in the main repository can break everything!
3) Illicit contents may be present in binary blobs, which in the future
may be need to be removed without warrant and the only way to do that is
by rebasing and force pushing, which will break "everything". It can be
everything from child-porn to expired distribution licenses.
Many people think that bit errors cannot happen because the memory uses
ECC and the file system uses cryptographic hashes to verify the
integrity of the data. But what many people forget about is that when
copying data from memory to disk, typically using a DMA channel data is
copied w/o any kind of integrity protection, because the integrity
protection is not end-to-end. The integrity protection is only per-link.
Therefore I propose the following changes to GIT.
1) Use a CRC128 / 256 or 512 non-cryptographic based hashing algorithm
as default.
2) Add support for a CRC fixup field, which usually is zero, but when
merges are needed, it can be non-zero, to allow the hash-tag-value to
remain the same! This also allows for easy conversion of existing GIT
repositories to the new scheme.
3) All git objects should be uncompressed.
CRC-XXX can easily be used to correct multiple bit errors without any
performance overhead.
--HPS
^ permalink raw reply
* Gitorious should use CRC128 / 256 / 512 instead of SHA-1
From: Hans Petter Selasky @ 2023-01-13 12:59 UTC (permalink / raw)
To: git
Hi,
Currently GIT only supports cryptographic hashes for its commit tags.
That means:
1) It's very difficult to edit the history without also recomputing the
hash tags for all commits after the needed change-point, which then
means references to a repository is broken.
2) Only a single bit error in the main repository can break everything!
3) Illicit contents may be present in binary blobs, which in the future
may be need to be removed without warrant and the only way to do that is
by rebasing and force pushing, which will break "everything". It can be
everything from child-porn to expired distribution licenses.
Many people think that bit errors cannot happen because the memory uses
ECC and the file system uses cryptographic hashes to verify the
integrity of the data. But what many people forget about is that when
copying data from memory to disk, typically using a DMA channel data is
copied w/o any kind of integrity protection, because the integrity
protection is not end-to-end. The integrity protection is only per-link.
Therefore I propose the following changes to GIT.
1) Use a CRC128 / 256 or 512 non-cryptographic based hashing algorithm
as default.
2) Add support for a CRC fixup field, which usually is zero, but when
merges are needed, it can be non-zero, to allow the hash-tag-value to
remain the same! This also allows for easy conversion of existing GIT
repositories to the new scheme.
3) All git objects should be uncompressed.
CRC-XXX can easily be used to correct multiple bit errors without any
performance overhead.
Please CC me. I'm not subscribed to this list.
--HPS
^ permalink raw reply
* RE: Parallelism for submodule update
From: Zitzmann, Christian @ 2023-01-13 10:49 UTC (permalink / raw)
To: rsbecker@nexbridge.com, git@vger.kernel.org
In-Reply-To: <009801d91eca$e1646360$a42d2a20$@nexbridge.com>
Hello Randall,
Yes, I guess this is a quite common that the harddisk is much faster than the Network Services.
With the scalar strategy (e.g. blobless clones) the checkout phase does not involve mainly harddisk activity anymore, but includes fetching sources from the remote. So it consumes a lot of Network Services.
Especially with parallelism we would gain a lot of performance here, as network and harddisk are utilized in parallel.
This could be even a general strategy (without using blobless clones) to have fetch and checkout done together, but both in an parallel Scheme
Currently it's like this:
Multithreading (mainly network utilization, only small amount of data - Commits and Trees -)
Thread1: Fetch submodule1 -> NETWORK
Thread2: Fetch submodule2 -> NETWORK
---
Thread<x>: Fetch Submodule<n> -> NETWORK
Sequential (alternating harddisk and network utilization)
Loop1
Try to Checkout Submodule1 commit -> HARDDISK
Fetch missing objects (e.g. Blobs - big amount of Data) -> NETWORK
Checkout Submodule commit -> HARDDISK
Loop2
Try to Checkout Submodule1 commit -> HARDDISK
Fetch missing objects (e.g. Blobs - big amount of Data) -> NETWORK
Checkout Submodule commit -> HARDDISK
...
Loop<n>
Try to Checkout Submodule<n> commit -> HARDDISK
Fetch missing objects (e.g. Blobs - big amount of Data) -> NETWORK
Checkout Submodule commit -> HARDDISK
Here the Network accesses in the sequential part really have significant waiting times (e.g. name service) with low local resources utilization
The proposal is to change it to a full parallel flow!
Multithreading (both network and harddisk are utilized all the time)
Thread1:
Fetch submodule1 (blobless) -> NETWORK
Try to Checkout Submodule commit -> HARDDISK
Fetch missing objects -> NETWORK
Checkout Submodule commit -> HARDDISK
Thread2:
Fetch submodule2 (blobless) -> NETWORK
Try to Checkout Submodule commit -> HARDDISK
Fetch missing objects -> NETWORK
Checkout Submodule commit... -> HARDDISK
...
Thread<x>
The only negative effect I'd see when having very slow harddisks, or disks that suffer significantly from parallel access, the overall performance could also suffer.
In general in the partial clone, but even in the full clone approach, network and harddisk utilization will be in parallel, and therefore performance can increase.
Best regards
Christian
-----Original Message-----
From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
Sent: Montag, 2. Januar 2023 17:54
To: Zitzmann, Christian <Christian.Zitzmann@vitesco.com>; git@vger.kernel.org
Subject: RE: Parallelism for submodule update
[Sie erhalten nicht häufig E-Mails von rsbecker@nexbridge.com. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
>-----Original Message-----
>From: <Christian.Zitzmann@vitesco.com>
On January 2, 2023 11:45 AM Christian Zitzmann wrote:
>we are using git since many years with also heavily using submodules.
>
>When updating the submodules, only the fetching part is done in
>parallel (with config submodule.fetchjobs or --jobs) but the checkout
>is done sequentially
>
>What I’ve recognized when cloning with
>- scalar clone --full-clone --recurse-submodules <URL> or
>- git clone --filter=blob:none --also-filter-submodules
>--recurse-submodules <URL>
>
>We loose performance, as the fetch of the blobs is done in the
>sequential checkout part, instead of in the parallel part.
>
>Furthermore, the utilization - without partial clone - of network and
>harddisk is not always good, as first the network is utilized (fetch)
>and then the harddisk
>(checkout)
>
>As the checkout part is local to the submodule (no shared resources to
>block), it would be great if we could move the checkout into the parallelized part.
>E.g. by doing fetch and checkout (with blob fetching) in one step with e.g.
>run_processes_parallel_tr2
>
>I expect that this significantly improves the performance, especially
>when using partial clones.
>
>Do you think this is possible? Do I miss anything in my thoughts?
Since this is a platform-specific request, if it happens, this should be a configuration switch that defaults off. On my platform, the file system itself is fairly fast, but the name service traversals and resolutions (what happens in the name service) is a performance problem. Doing the checkout/switch in parallel would actually be counter-productive in my case. So I would keep it off, but I get that other platforms could benefit.
Regards,
Randall
--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.
^ permalink raw reply
* Re: [PATCH v2 0/9] sequencer API & users: fix widespread leaks
From: Phillip Wood @ 2023-01-13 10:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
In-Reply-To: <cover-v2-0.9-00000000000-20230112T124201Z-avarab@gmail.com>
Hi Ævar
The code changes in this version look good modulo a couple of minor
comments, however some of the commit messages need to be updated to
reflect the (very welcome) changes you've made in v2.
Thanks again for working on this
Phillip
On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> This series fixes various widespread leaks in the sequencer and its
> users (rebase, revert, cherry-pick). As a result 18 tests become
> leak-free in their entirety.
>
> See the v1 for a longer general summary:
> https://lore.kernel.org/git/cover-00.10-00000000000-20221230T071741Z-avarab@gmail.com/
>
> Changes since v1:
>
> * I think this addresses all the outstanding feedback, thanks all.
> * Most significantly, the replay_opts_release() is now moved out of
> sequencer_remove_state() as suggested.
> * There's a prep change for renaming "squash_onto_name", per the
> discussion in v1.
> * Just do "goto leave" rather than being paranoid and introdungi
> "goto cleanup", thanks Phillip!
> * Various other small changes, see the range-diff.
>
> A branch & passing CI for this are at:
> https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-2
>
> Ævar Arnfjörð Bjarmason (9):
> rebase: use "cleanup" pattern in do_interactive_rebase()
> sequencer.c: split up sequencer_remove_state()
> rebase & sequencer API: fix get_replay_opts() leak in "rebase"
> builtin/revert.c: move free-ing of "revs" to replay_opts_release()
> builtin/rebase.c: rename "squash_onto_name" to "to_free"
> builtin/rebase.c: fix "options.onto_name" leak
> sequencer.c: always free() the "msgbuf" in do_pick_commit()
> builtin/rebase.c: free() "options.strategy_opts"
> commit.c: free() revs.commit in get_fork_point()
>
> builtin/rebase.c | 27 +++++++++-------
> builtin/revert.c | 8 ++---
> commit.c | 1 +
> sequencer.c | 43 ++++++++++++++++----------
> sequencer.h | 1 +
> t/t3405-rebase-malformed.sh | 1 +
> t/t3412-rebase-root.sh | 1 +
> t/t3416-rebase-onto-threedots.sh | 1 +
> t/t3419-rebase-patch-id.sh | 1 +
> t/t3423-rebase-reword.sh | 1 +
> t/t3425-rebase-topology-merges.sh | 2 ++
> t/t3431-rebase-fork-point.sh | 1 +
> t/t3432-rebase-fast-forward.sh | 1 +
> t/t3437-rebase-fixup-options.sh | 1 +
> t/t3438-rebase-broken-files.sh | 2 ++
> t/t3501-revert-cherry-pick.sh | 1 +
> t/t3502-cherry-pick-merge.sh | 1 +
> t/t3503-cherry-pick-root.sh | 1 +
> t/t3506-cherry-pick-ff.sh | 1 +
> t/t3511-cherry-pick-x.sh | 1 +
> t/t7402-submodule-rebase.sh | 1 +
> t/t9106-git-svn-commit-diff-clobber.sh | 1 -
> t/t9164-git-svn-dcommit-concurrent.sh | 1 -
> 23 files changed, 64 insertions(+), 36 deletions(-)
>
> Range-diff against v1:
> 1: f3a4ed79c7d ! 1: d0a0524f3d4 rebase: use "cleanup" pattern in do_interactive_rebase()
> @@ Commit message
> rebase: use "cleanup" pattern in do_interactive_rebase()
>
> Use a "goto cleanup" pattern in do_interactive_rebase(). This
> - eliminates some duplicated free() code added in 0609b741a43 (rebase
> - -i: combine rebase--interactive.c with rebase.c, 2019-04-17), and sets
> - us up for a subsequent commit which'll make further use of the
> - "cleanup" label.
> + eliminates some duplicated free() code added in 53bbcfbde7c (rebase
> + -i: implement the main part of interactive rebase as a builtin,
> + 2018-09-27), and sets us up for a subsequent commit which'll make
> + further use of the "cleanup" label.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> 2: 4994940a0a9 ! 2: c4eaa8dfef4 sequencer.c: split up sequencer_remove_state()
> @@ Commit message
> function, which will be adjusted and called independent of the other
> code in sequencer_remove_state() in a subsequent commit.
>
> - The only functional changes here are:
> -
> - * Changing the "int" to a "size_t", which is the correct type, as
> - "xopts_nr" is a "size_t".
> -
> - * Calling the free() before the "if (is_rebase_i(opts) && ...)",
> - which is OK, and makes a subsequent change smaller.
> + The only functional change here is changing the "int" to a "size_t",
> + which is the correct type, as "xopts_nr" is a "size_t".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
> struct strbuf buf = STRBUF_INIT;
> - int i, ret = 0;
> + int ret = 0;
> -+
> -+ replay_opts_release(opts);
>
> if (is_rebase_i(opts) &&
> strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
> @@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
> - free(opts->xopts[i]);
> - free(opts->xopts);
> - strbuf_release(&opts->current_fixups);
> --
> ++ replay_opts_release(opts);
> +
> strbuf_reset(&buf);
> strbuf_addstr(&buf, get_dir(opts));
> - if (remove_dir_recursively(&buf, 0))
> 3: 3e9c4df61fe ! 3: f06f565ceaf rebase & sequencer API: fix get_replay_opts() leak in "rebase"
> @@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts)
> break;
> }
> case ACTION_EDIT_TODO:
> +@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)
> +
> + replay.action = REPLAY_INTERACTIVE_REBASE;
> + ret = sequencer_remove_state(&replay);
> ++ replay_opts_release(&replay);
> + } else {
> + strbuf_addstr(&dir, opts->state_dir);
> + if (remove_dir_recursively(&dir, 0))
> +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
> +
> + replay.action = REPLAY_INTERACTIVE_REBASE;
> + ret = sequencer_remove_state(&replay);
> ++ replay_opts_release(&replay);
> + } else {
> + strbuf_reset(&buf);
> + strbuf_addstr(&buf, options.state_dir);
> +
> + ## builtin/revert.c ##
> +@@ builtin/revert.c: int cmd_revert(int argc, const char **argv, const char *prefix)
> + if (opts.revs)
> + release_revisions(opts.revs);
> + free(opts.revs);
> ++ replay_opts_release(&opts);
> + return res;
> + }
> +
> +@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
> + free(opts.revs);
> + if (res < 0)
> + die(_("cherry-pick failed"));
> ++ replay_opts_release(&opts);
> + return res;
> + }
>
> ## sequencer.c ##
> @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
> @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
> -static void replay_opts_release(struct replay_opts *opts)
> +void replay_opts_release(struct replay_opts *opts)
> {
> -- free(opts->gpg_sign);
> -- free(opts->reflog_action);
> -- free(opts->default_strategy);
> -- free(opts->strategy);
> -+ FREE_AND_NULL(opts->gpg_sign);
> -+ FREE_AND_NULL(opts->reflog_action);
> -+ FREE_AND_NULL(opts->default_strategy);
> -+ FREE_AND_NULL(opts->strategy);
> + free(opts->gpg_sign);
> + free(opts->reflog_action);
> +@@ sequencer.c: static void replay_opts_release(struct replay_opts *opts)
> + free(opts->strategy);
> for (size_t i = 0; i < opts->xopts_nr; i++)
> free(opts->xopts[i]);
> -- free(opts->xopts);
> + opts->xopts_nr = 0;
> -+ FREE_AND_NULL(opts->xopts);
> + free(opts->xopts);
> strbuf_release(&opts->current_fixups);
> }
> +@@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
> + }
> + }
>
> +- replay_opts_release(opts);
> +-
> + strbuf_reset(&buf);
> + strbuf_addstr(&buf, get_dir(opts));
> + if (remove_dir_recursively(&buf, 0))
>
> ## sequencer.h ##
> @@ sequencer.h: int sequencer_pick_revisions(struct repository *repo,
> @@ t/t3412-rebase-root.sh: Tests if git rebase --root --onto <newparent> can rebase
>
> log_with_names () {
>
> + ## t/t3419-rebase-patch-id.sh ##
> +@@ t/t3419-rebase-patch-id.sh: test_description='git rebase - test patch id computation'
> + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> ++TEST_PASSES_SANITIZE_LEAK=true
> + . ./test-lib.sh
> +
> + scramble () {
> +
> ## t/t3423-rebase-reword.sh ##
> @@
>
> @@ t/t3423-rebase-reword.sh
>
> . "$TEST_DIRECTORY"/lib-rebase.sh
>
> + ## t/t3425-rebase-topology-merges.sh ##
> +@@
> + #!/bin/sh
> +
> + test_description='rebase topology tests with merges'
> ++
> ++TEST_PASSES_SANITIZE_LEAK=true
> + . ./test-lib.sh
> + . "$TEST_DIRECTORY"/lib-rebase.sh
> +
> +
> ## t/t3437-rebase-fixup-options.sh ##
> @@ t/t3437-rebase-fixup-options.sh: to the "fixup" command that works with "fixup!", "fixup -C" works with
> "amend!" upon --autosquash.
> @@ t/t3438-rebase-broken-files.sh
>
> test_expect_success 'set up conflicting branches' '
>
> + ## t/t3501-revert-cherry-pick.sh ##
> +@@ t/t3501-revert-cherry-pick.sh: test_description='test cherry-pick and revert with renames
> + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> ++TEST_PASSES_SANITIZE_LEAK=true
> + . ./test-lib.sh
> +
> + test_expect_success setup '
> +
> + ## t/t3502-cherry-pick-merge.sh ##
> +@@ t/t3502-cherry-pick-merge.sh: test_description='cherry picking and reverting a merge
> + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> ++TEST_PASSES_SANITIZE_LEAK=true
> + . ./test-lib.sh
> +
> + test_expect_success setup '
> +
> + ## t/t3503-cherry-pick-root.sh ##
> +@@ t/t3503-cherry-pick-root.sh: test_description='test cherry-picking (and reverting) a root commit'
> + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> ++TEST_PASSES_SANITIZE_LEAK=true
> + . ./test-lib.sh
> +
> + test_expect_success setup '
> +
> + ## t/t3506-cherry-pick-ff.sh ##
> +@@ t/t3506-cherry-pick-ff.sh: test_description='test cherry-picking with --ff option'
> + GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> + export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> ++TEST_PASSES_SANITIZE_LEAK=true
> + . ./test-lib.sh
> +
> + test_expect_success setup '
> +
> + ## t/t3511-cherry-pick-x.sh ##
> +@@
> +
> + test_description='Test cherry-pick -x and -s'
> +
> ++TEST_PASSES_SANITIZE_LEAK=true
> + . ./test-lib.sh
> +
> + pristine_detach () {
> +
> ## t/t7402-submodule-rebase.sh ##
> @@
>
> 4: 1e4e504c533 < -: ----------- builtin/revert.c: refactor run_sequencer() return pattern
> 5: e2895bb9795 < -: ----------- builtin/revert.c: fix common leak by using replay_opts_release()
> 6: 21eea8eb802 ! 4: e83bdfab046 builtin/revert.c: move free-ing of "revs" to replay_opts_release()
> @@ builtin/revert.c: int cmd_revert(int argc, const char **argv, const char *prefix
> - if (opts.revs)
> - release_revisions(opts.revs);
> - free(opts.revs);
> -+ replay_opts_release(&opts);
> + replay_opts_release(&opts);
> return res;
> }
> -
> @@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
> opts.action = REPLAY_PICK;
> sequencer_init_config(&opts);
> @@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *p
> - if (opts.revs)
> - release_revisions(opts.revs);
> - free(opts.revs);
> -+ replay_opts_release(&opts);
> if (res < 0)
> die(_("cherry-pick failed"));
> - return res;
> + replay_opts_release(&opts);
>
> ## sequencer.c ##
> @@ sequencer.c: void replay_opts_release(struct replay_opts *opts)
> opts->xopts_nr = 0;
> - FREE_AND_NULL(opts->xopts);
> + free(opts->xopts);
> strbuf_release(&opts->current_fixups);
> + if (opts->revs)
> + release_revisions(opts->revs);
> -+ FREE_AND_NULL(opts->revs);
> ++ free(opts->revs);
> }
>
> int sequencer_remove_state(struct replay_opts *opts)
> -: ----------- > 5: 4fea2b77c6d builtin/rebase.c: rename "squash_onto_name" to "to_free"
> 7: 484ebbfd6d1 ! 6: 898bb7698fc builtin/rebase.c: fix "options.onto_name" leak
> @@ Commit message
>
> In [1] we started saving away the earlier xstrdup()'d
> "options.onto_name" assignment to free() it, but when [2] added this
> - "keep_base" branch it didn't free() the already assigned
> - "squash_onto_name" before re-assigning to "options.onto_name". Let's
> - do that, and fix the memory leak.
> + "keep_base" branch it didn't free() the already assigned value before
> + re-assigning to "options.onto_name". Let's do that, and fix the memory
> + leak.
>
> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> strbuf_addstr(&buf, "...");
> strbuf_addstr(&buf, branch_name);
> - options.onto_name = xstrdup(buf.buf);
> -+ free(squash_onto_name);
> -+ options.onto_name = squash_onto_name = xstrdup(buf.buf);
> ++ free(to_free);
> ++ options.onto_name = to_free = xstrdup(buf.buf);
> } else if (!options.onto_name)
> options.onto_name = options.upstream_name;
> if (strstr(options.onto_name, "...")) {
> 8: d607dbac38e ! 7: fb38dc873f9 sequencer.c: always free() the "msgbuf" in do_pick_commit()
> @@ Commit message
> we'd return before the strbuf_release(&msgbuf).
>
> Then when the "fixup" support was added in [3] this leak got worse, as
> - we added another place where we'd "return" before reaching the
> - strbuf_release().
> + in this error case we added another place where we'd "return" before
> + reaching the strbuf_release().
>
> - Let's move it to a "cleanup" label, and use an appropriate "goto". It
> - may or may not be safe to combine the existing "leave" and "cleanup"
> - labels, but this change doesn't attempt to answer that question. Let's
> - instead avoid calling update_abort_safety_file() in these cases, as we
> - didn't do so before.
> + This changes the behavior so that we'll call
> + update_abort_safety_file() in these cases where we'd previously
> + "return", but as noted in [4] "update_abort_safety_file() is a no-op
> + when rebasing and you're changing code that is only run when
> + rebasing.". Here "no-op" refers to the early return in
> + update_abort_safety_file() if git_path_seq_dir() doesn't exist.
>
> 1. 452202c74b8 (sequencer: stop releasing the strbuf in
> write_message(), 2016-10-21)
> @@ Commit message
> 2016-07-26)
> 3. 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
> 'squash' commands, 2017-01-02)
> + 4. https://lore.kernel.org/git/bcace50b-a4c3-c468-94a3-4fe0c62b3671@dunelm.org.uk/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> @@ sequencer.c: static int do_pick_commit(struct repository *r,
> - return -1;
> + opts, item->flags)) {
> + res = -1;
> -+ goto cleanup;
> ++ goto leave;
> + }
> flags |= AMEND_MSG;
> if (!final_fixup)
> @@ sequencer.c: static int do_pick_commit(struct repository *r,
> + if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
> + res = error(_("could not rename '%s' to '%s'"),
> + rebase_path_squash_msg(), dest);
> -+ goto cleanup;
> ++ goto leave;
> + }
> unlink(git_path_merge_msg(r));
> msg_file = dest;
> @@ sequencer.c: static int do_pick_commit(struct repository *r,
> /*
> * If the merge was clean or if it failed due to conflict, we write
> @@ sequencer.c: static int do_pick_commit(struct repository *r,
> - }
> -
> leave:
> -+ update_abort_safety_file();
> -+cleanup:
> free_message(commit, &msg);
> free(author);
> -- update_abort_safety_file();
> + strbuf_release(&msgbuf);
> + update_abort_safety_file();
>
> return res;
> - }
> 9: cd0489a2384 ! 8: d4b0e2a5c83 builtin/rebase.c: free() "options.strategy_opts"
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> free(options.strategy);
> + free(options.strategy_opts);
> strbuf_release(&options.git_format_patch_opt);
> - free(squash_onto_name);
> + free(to_free);
> string_list_clear(&exec, 0);
> 10: eb3678b4667 = 9: fd9c7a5547c commit.c: free() revs.commit in get_fork_point()
^ permalink raw reply
* Re: [PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak
From: Phillip Wood @ 2023-01-13 10:41 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
In-Reply-To: <patch-v2-6.9-898bb7698fc-20230112T124201Z-avarab@gmail.com>
Hi Ævar
On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> In [1] we started saving away the earlier xstrdup()'d
> "options.onto_name" assignment to free() it, but when [2] added this
> "keep_base" branch it didn't free() the already assigned value before
> re-assigning to "options.onto_name". Let's do that, and fix the memory
> leak.
As I said before I don't think this message makes any sense. It should
just say that when --keep-base is given we need to free
options.onto_name as it does not come from argv. As I also mentioned you
do not need to add "free(to_free)" as it is unused at this point.
Freeing it makes the reader wonder what was previously assigned to it.
Best Wishes
Phillip
> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/rebase.c | 3 ++-
> t/t3416-rebase-onto-threedots.sh | 1 +
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0d8c050f6b3..b4857b89f19 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1660,7 +1660,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> strbuf_addstr(&buf, options.upstream_name);
> strbuf_addstr(&buf, "...");
> strbuf_addstr(&buf, branch_name);
> - options.onto_name = xstrdup(buf.buf);
> + free(to_free);
> + options.onto_name = to_free = xstrdup(buf.buf);
> } else if (!options.onto_name)
> options.onto_name = options.upstream_name;
> if (strstr(options.onto_name, "...")) {
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ea501f2b42b..f8c4ed78c9e 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
> GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
> . "$TEST_DIRECTORY/lib-rebase.sh"
>
^ permalink raw reply
* Re: [PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free"
From: Phillip Wood @ 2023-01-13 10:37 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
In-Reply-To: <patch-v2-5.9-4fea2b77c6d-20230112T124201Z-avarab@gmail.com>
On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> The real use of the "squash_onto_name" added in [1] is to keep track
> of a value for later free()-ing, we don't subsequently use it for
> anything else.
>
> Let's rename it in preparation for re-using it for free()-ing before
> another assignment to "options.onto_name", which is an outstanding
> leak that'll be fixed in a subsequent commit.
This is good
Thanks
Phillip
> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/rebase.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5859a5387d8..0d8c050f6b3 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1036,7 +1036,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> const char *rebase_merges = NULL;
> struct string_list strategy_options = STRING_LIST_INIT_NODUP;
> struct object_id squash_onto;
> - char *squash_onto_name = NULL;
> + char *to_free = NULL;
> int reschedule_failed_exec = -1;
> int allow_preemptive_ff = 1;
> int preserve_merges_selected = 0;
> @@ -1589,7 +1589,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> &squash_onto, NULL, NULL) < 0)
> die(_("Could not create new root commit"));
> options.squash_onto = &squash_onto;
> - options.onto_name = squash_onto_name =
> + options.onto_name = to_free =
> xstrdup(oid_to_hex(&squash_onto));
> } else
> options.root_with_onto = 1;
> @@ -1835,7 +1835,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> free(options.cmd);
> free(options.strategy);
> strbuf_release(&options.git_format_patch_opt);
> - free(squash_onto_name);
> + free(to_free);
> string_list_clear(&exec, 0);
> string_list_clear(&strategy_options, 0);
> return !!ret;
^ permalink raw reply
* Re: [PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release()
From: Phillip Wood @ 2023-01-13 10:36 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
In-Reply-To: <patch-v2-4.9-e83bdfab046-20230112T124201Z-avarab@gmail.com>
Hi Ævar
On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> In [1] and [2] I added the code being moved here to cmd_revert() and
> cmd_cherry_pick(),
I'm not sure how relevant this first clause is but the change itself
looks good.
> now that we've got a "replay_opts_release()" for
> the "struct replay_opts" it should know how to free these "revs",
> rather than having these users reach into the struct to free its
> individual members.
>
> As explained in earlier change we should be using FREE_AND_NULL() in
> replay_opts_release() rather than free().
This paragraph is out of date
Best Wishes
Phillip
> 1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
> 2022-11-08)
> 2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/revert.c | 6 ------
> sequencer.c | 3 +++
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1cab16bf3ed..77d2035616e 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -248,9 +248,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
> res = run_sequencer(argc, argv, &opts);
> if (res < 0)
> die(_("revert failed"));
> - if (opts.revs)
> - release_revisions(opts.revs);
> - free(opts.revs);
> replay_opts_release(&opts);
> return res;
> }
> @@ -263,9 +260,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
> opts.action = REPLAY_PICK;
> sequencer_init_config(&opts);
> res = run_sequencer(argc, argv, &opts);
> - if (opts.revs)
> - release_revisions(opts.revs);
> - free(opts.revs);
> if (res < 0)
> die(_("cherry-pick failed"));
> replay_opts_release(&opts);
> diff --git a/sequencer.c b/sequencer.c
> index 5d8c68912a1..c729ce77260 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -362,6 +362,9 @@ void replay_opts_release(struct replay_opts *opts)
> opts->xopts_nr = 0;
> free(opts->xopts);
> strbuf_release(&opts->current_fixups);
> + if (opts->revs)
> + release_revisions(opts->revs);
> + free(opts->revs);
> }
>
> int sequencer_remove_state(struct replay_opts *opts)
^ permalink raw reply
* Re: [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase"
From: Phillip Wood @ 2023-01-13 10:34 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
In-Reply-To: <patch-v2-3.9-f06f565ceaf-20230112T124201Z-avarab@gmail.com>
Hi Ævar
On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> Make the recently added replay_opts_release() function non-static and
> use it for freeing the "struct replay_opts" constructed by the
> get_replay_opts() function in "builtin/rebase.c". See [1] for the
> initial addition of get_replay_opts().
>
> To safely call our new replay_opts_release() we'll need to change all
> the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
> loop over it and free() it (the free() in the loop doesn't need to be
> a FREE_AND_NULL()).
>
> This is because in e.g. do_interactive_rebase() we construct a "struct
> replay_opts" with "get_replay_opts()", and then call
> "complete_action()". If we get far enough in that function without
> encountering errors we'll call "pick_commits()" which (indirectly)
> calls sequencer_remove_state() at the end.
>
> But if we encounter errors anywhere along the way we'd punt out early,
> and not free() the memory we allocated. Remembering whether we
> previously called sequencer_remove_state() would be a hassle, so let's
> make it safe to re-invoke replay_opts_release() instead.
>
> I experimented with a change to be more paranoid instead, i.e. to
> exhaustively check our state via an enum. We could make sure that we:
>
> - Only allow calling "replay_opts_release()" after
> "sequencer_remove_state()", but not the other way around.
>
> - Forbid invoking either function twice in a row.
>
> But such paranoia isn't warranted here, let's instead take the easy
> way out and FREE_AND_NULL() this.
The changes below look good, but this message needs updating to reflect
the re-roll.
> @@ -359,6 +359,7 @@ static void replay_opts_release(struct replay_opts *opts)
> free(opts->strategy);
> for (size_t i = 0; i < opts->xopts_nr; i++)
> free(opts->xopts[i]);
> + opts->xopts_nr = 0;
I don't think we need this now we're only calling replay_opts_release()
once.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v6 0/2] check-attr: add support to work with tree-ish
From: Phillip Wood @ 2023-01-13 10:27 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: sunshine
In-Reply-To: <cover.1673521102.git.karthik.188@gmail.com>
Hi Karthik
On 12/01/2023 11:00, Karthik Nayak wrote:
> v1: https://lore.kernel.org/git/20221206103736.53909-1-karthik.188@gmail.com/
> v2: https://lore.kernel.org/git/CAOLa=ZSsFGBw3ta1jWN8cmUch2ca=zTEjp1xMA6Linafx9W53g@mail.gmail.com/T/#t
> v3: https://lore.kernel.org/git/20221216093552.3171319-1-karthik.188@gmail.com/
> v4: https://lore.kernel.org/git/cover.1671630304.git.karthik.188@gmail.com
> v5: https://lore.kernel.org/git/cover.1671793109.git.karthik.188@gmail.com/
>
> Given a pathname, git-check-attr(1) will list the attributes which apply to that
> pathname by reading all relevant gitattributes files. Currently there is no way
> to specify a tree-ish to read the gitattributes from.
>
> This is specifically useful in bare repositories wherein the gitattributes are
> only present in the git working tree but not available directly on the
> filesystem.
>
> This series aims to add a new flag `--source` to git-check-attr(1) which
> allows us to read gitattributes from the specified tree-ish.
>
> Changes since v5:
> - Changed the documentation and helper code to say 'tree-ish' instead of 'tree'
> - Fixed broken tests because of missing `&&`
>
> Range-diff against v5:
>
> 1: 6224754179 = 1: 6224754179 t0003: move setup for `--all` into new block
> 2: d835d989ad ! 2: 57f5957127 attr: add flag `--source` to work with tree-ish
> @@ Documentation/git-check-attr.txt: git-check-attr - Display gitattributes informa
> [verse]
> -'git check-attr' [-a | --all | <attr>...] [--] <pathname>...
> -'git check-attr' --stdin [-z] [-a | --all | <attr>...]
> -+'git check-attr' [--source <tree>] [-a | --all | <attr>...] [--] <pathname>...
> -+'git check-attr' --stdin [-z] [--source <tree>] [-a | --all | <attr>...]
> ++'git check-attr' [--source <tree-ish>] [-a | --all | <attr>...] [--] <pathname>...
> ++'git check-attr' --stdin [-z] [--source <tree-ish>] [-a | --all | <attr>...]
>
> DESCRIPTION
> -----------
> @@ Documentation/git-check-attr.txt: OPTIONS
> If `--stdin` is also given, input paths are separated
> with a NUL character instead of a linefeed character.
>
> -+--source=<tree>::
> -+ Check attributes against the specified tree-ish. Paths provided as part
> -+ of the revision will be treated as the root directory. It is common to
> ++--source=<tree-ish>::
> ++ Check attributes against the specified tree-ish. It is common to
> + specify the source tree by naming a commit, branch or tag associated
> + with it.
> +
> @@ attr.c: void git_check_attr(struct index_state *istate,
> const char *name = check->all_attrs[i].attr->name;
>
> ## attr.h ##
> +@@
> + #ifndef ATTR_H
> + #define ATTR_H
> +
> ++#include "hash.h"
> ++
> + /**
> + * gitattributes mechanism gives a uniform way to associate various attributes
> + * to set of paths.
> @@ attr.h: void attr_check_free(struct attr_check *check);
> const char *git_attr_name(const struct git_attr *);
>
> @@ builtin/check-attr.c
> static const char * const check_attr_usage[] = {
> -N_("git check-attr [-a | --all | <attr>...] [--] <pathname>..."),
> -N_("git check-attr --stdin [-z] [-a | --all | <attr>...]"),
> -+N_("git check-attr [--source <tree>] [-a | --all | <attr>...] [--] <pathname>..."),
> -+N_("git check-attr --stdin [-z] [--source <tree>] [-a | --all | <attr>...]"),
> ++N_("git check-attr [--source <tree-ish>] [-a | --all | <attr>...] [--] <pathname>..."),
> ++N_("git check-attr --stdin [-z] [--source <tree-ish>] [-a | --all | <attr>...]"),
> NULL
> };
>
> @@ t/t0003-attributes.sh: attr_check_quote () {
>
> + git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
> + echo "$path: test: $expect" >expect &&
> -+ test_cmp expect actual
> ++ test_cmp expect actual &&
> + test_must_be_empty err
> }
>
> @@ t/t0003-attributes.sh: test_expect_success 'setup' '
>
> +test_expect_success 'setup branches' '
> + mkdir -p foo/bar &&
> -+ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
> ++ test_commit --printf "add .gitattributes" foo/bar/.gitattributes \
> + "f test=f\na/i test=n\n" tag-1 &&
> -+
> -+ mkdir -p foo/bar &&
> -+ test_commit --printf "add .gitattributes" foo/bar/.gitattribute \
> -+ "g test=g\na/i test=m\n" tag-2
> ++ test_commit --printf "add .gitattributes" foo/bar/.gitattributes \
> ++ "g test=g\na/i test=m\n" tag-2 &&
> ++ rm foo/bar/.gitattributes
> +'
> +
> test_expect_success 'command line checks' '
The changes in the range-diff look good - this version addresses all of
my previous comments - thanks.
Best Wishes
Phillip
> Karthik Nayak (2):
> t0003: move setup for `--all` into new block
> attr: add flag `--source` to work with tree-ish
>
> Documentation/git-check-attr.txt | 9 ++-
> archive.c | 2 +-
> attr.c | 97 +++++++++++++++++++++++---------
> attr.h | 7 ++-
> builtin/check-attr.c | 35 +++++++-----
> builtin/pack-objects.c | 2 +-
> convert.c | 2 +-
> ll-merge.c | 4 +-
> pathspec.c | 2 +-
> t/t0003-attributes.sh | 48 +++++++++++++++-
> userdiff.c | 2 +-
> ws.c | 2 +-
> 12 files changed, 157 insertions(+), 55 deletions(-)
>
^ 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