* [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
From: Patrick Steinhardt @ 2023-10-13 12:37 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Junio C Hamano, Taylor Blau
In-Reply-To: <ZSkCGS3JPEQ71dOF@tanuki>
[-- Attachment #1: Type: text/plain, Size: 4246 bytes --]
Commit graphs can become stale and contain references to commits that do
not exist in the object database anymore. Theoretically, this can lead
to a scenario where we are able to successfully look up any such commit
via the commit graph even though such a lookup would fail if done via
the object database directly.
As the commit graph is mostly intended as a sort of cache to speed up
parsing of commits we do not want to have diverging behaviour in a
repository with and a repository without commit graphs, no matter
whether they are stale or not. As commits are otherwise immutable, the
only thing that we really need to care about is thus the presence or
absence of a commit.
To address potentially stale commit data that may exist in the graph,
our `lookup_commit_in_graph()` function will check for the commit's
existence in both the commit graph, but also in the object database. So
even if we were able to look up the commit's data in the graph, we would
still pretend as if the commit didn't exist if it is missing in the
object database.
We don't have the same safety net in `parse_commit_in_graph_one()`
though. This function is mostly used internally in "commit-graph.c"
itself to validate the commit graph, and this usage is fine. We do
expose its functionality via `parse_commit_in_graph()` though, which
gets called by `repo_parse_commit_internal()`, and that function is in
turn used in many places in our codebase.
For all I can see this function is never used to directly turn an object
ID into a commit object without additional safety checks before or after
this lookup. What it is being used for though is to walk history via the
parent chain of commits. So when commits in the parent chain of a graph
walk are missing it is possible that we wouldn't notice if that missing
commit was part of the commit graph. Thus, a query like `git rev-parse
HEAD~2` can succeed even if the intermittent commit is missing.
It's unclear whether there are additional ways in which such stale
commit graphs can lead to problems. In any case, it feels like this is a
bigger bug waiting to happen when we gain additional direct or indirect
callers of `repo_parse_commit_internal()`. So let's fix the inconsistent
behaviour by checking for object existence via the object database, as
well.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
commit.c | 7 ++++++-
t/t5318-commit-graph.sh | 23 +++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index b3223478bc..109e9217e3 100644
--- a/commit.c
+++ b/commit.c
@@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,
return -1;
if (item->object.parsed)
return 0;
- if (use_commit_graph && parse_commit_in_graph(r, item))
+ if (use_commit_graph && parse_commit_in_graph(r, item)) {
+ if (!has_object(r, &item->object.oid, 0))
+ return quiet_on_missing ? -1 :
+ error(_("commit %s exists in commit-graph but not in the object database"),
+ oid_to_hex(&item->object.oid));
return 0;
+ }
if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
return quiet_on_missing ? -1 :
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ba65f17dd9..25f8e9e2d3 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -821,4 +821,27 @@ test_expect_success 'overflow during generation version upgrade' '
)
'
+test_expect_success 'commit exists in commit-graph but not in object database' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+ git commit-graph write --reachable &&
+
+ # Corrupt the repository by deleting the intermittent commit
+ # object. Commands should notice that this object is absent and
+ # thus that the repository is corrupt even if the commit graph
+ # exists.
+ oid=$(git rev-parse B) &&
+ rm .git/objects/"$(test_oid_to_path "$oid")" &&
+
+ test_must_fail git rev-parse HEAD~2 2>error &&
+ grep "error: commit $oid exists in commit-graph but not in the object database" error
+ )
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: why does git set X in LESS env var?
From: Christoph Anton Mitterer @ 2023-10-13 13:45 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <6d673c1bdae41236e95e3a9fca853731@manjaro.org>
On Fri, 2023-10-13 at 06:43 +0200, Dragan Simic wrote:
> > *If* some changes were made to how git handles this, it might
> > perhaps
> > be worth to consider not to touch LESS at all, but only add the
> > required settings via command line arguments (i.e. -F -R ...).
>
> Actually, that would be wrong. If someone sets $LESS or $PAGER (or
> $GIT_PAGER, more specifically), it's up to the utility that invokes
> the
> pager internally not to override the user preferences configured
> through
> these environment variables. That's how everyone can customize the
> pager behavior.
Well, but if its clear that the output would otherwise be garbage (e.g.
because -R is missing).
In any case right now we have the situation that a user cannot just
easily set LESS in his environment, with a minimum set of options, and
git's use of less will continue flawlessly out of the box, as the -R
would be missing.
> > Or perhaps only remove options from it, if they're known to break
> > the
> > behaviour with git (like -+R might).
>
> Again, not the way the whole thing with pagination works. If someone
> sets their environment variables wrong, it's simply the way they want
> it, and it isn't anyone else's business to attempt fixing it
> automatically.
Well, I wouldn't agree with that.
LESS foremost a env var to configure less (surprise ^^).
If git (or anyone else) uses less internally, e.g. because they don't
want to implement their own pager, fine... but then they cannot just
blindly assume that LESS is set only for git's (or any other tool's
needs).
So I'd say the proper way is rather that any such tool makes sure, that
any options strictly required as set no matter what. Just as e.g. delta
does.
> Well, fragile or not, that's the way it works. It has its downsides
> for
> sure, but it's all about having each utility handle the environment
> carefully and document it in its man page(s), so the users can also
> carefully craft the values of their customized environment variables.
Sure, but from a user's view, the use of less (or anything else) within
git is conceptually completely opaque.
In less' manpage LESS isn't documented as "oh and you must make sure -R
is included or otherwise git will break"...
> $LESS can be seen as a global set of the common options for less(1),
o.O ... but, as I've described, one cannot really use it as that:
If I globally set e.g. LESS="F" because my desire is to make less
always exit as soon as the file fits on a screen, which I think is a
reasonable thing to do, git would no longer add "R" and output would
break.
> You don't have to define an alias, there's $GIT_PAGER for that
> purpose,
> as I already explained above.
Well, yes... and as I've said before, one could also solve it via
git_config... but the problem stays the same... as soon as someone
wants to use LESS as global less options just as you described it
yourself, git will no longer worker properly because of the missing -R.
And actually if one would use GIT_PAGER one would again defeat the
purpose of a allegedly global options LESS, because unless one does
something like GIT_PAGER="${LESS}R" it wouldn't see any changes made to
LESS.
> Moreover, the whole idea of the various utilities touching the $LESS
> variable internally is to provide sane defaults to the users that
> don't
> configure $LESS, $PAGER, etc. on their own.
Then I don't see what the big problem would be to just do it via a
command argument - if someone really has ever some reasons to remove --
RAW‐CONTROL‐CHARS from the command options when less is invoked via git
... then he could still go into git_config and set that manually.
But it would seem to me that the overall handling would be much more
what one expects, than when doing the same via LESS.
> I don't know what delta is and how it actually paginates its outputs,
> but it should follow the rules of the environment-based pager
> configuration that I described in detail above.
Well, AFAIU, it doesn't and for good reasons :-)
Anyway... I think all necessary things have been said and this thread
has grown far to large with only semi-related stuff... so thanks for
all the replies why git uses "-X".
Cheers,
Chris.
^ permalink raw reply
* Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Uwe Kleine-König @ 2023-10-13 14:14 UTC (permalink / raw)
To: git; +Cc: entwicklung, Michael Strawbridge, Luben Tuikov
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
Hello,
$ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
error: unable to extract a valid address from:
This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").
Bisection points at
a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
I didn't try to understand that change and fix the problem.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-13 15:00 UTC (permalink / raw)
To: Christoph Anton Mitterer; +Cc: git
In-Reply-To: <48ff9c2ac262cec32ab4681e8417413488278294.camel@scientia.org>
On 2023-10-13 15:45, Christoph Anton Mitterer wrote:
> On Fri, 2023-10-13 at 06:43 +0200, Dragan Simic wrote:
>> Actually, that would be wrong. If someone sets $LESS or $PAGER (or
>> $GIT_PAGER, more specifically), it's up to the utility that invokes
>> the pager internally not to override the user preferences configured
>> through these environment variables. That's how everyone can
>> customize
>> the pager behavior.
>
> Well, but if its clear that the output would otherwise be garbage (e.g.
> because -R is missing).
Well, it's the basic principle of "garbage in, garbage out". If there's
something wrong with the contents of the environment variables, that's
simply the way the user configured it, and it's only their job to get it
right.
> In any case right now we have the situation that a user cannot just
> easily set LESS in his environment, with a minimum set of options, and
> git's use of less will continue flawlessly out of the box, as the -R
> would be missing.
Let me repeat that it isn't the job of git or any other pager-enabled
utility to fix the user-defined environment. Otherwise, the user
actually wouldn't be able to make their choice freely.
>> Again, not the way the whole thing with pagination works. If someone
>> sets their environment variables wrong, it's simply the way they want
>> it, and it isn't anyone else's business to attempt fixing it
>> automatically.
>
> Well, I wouldn't agree with that.
> LESS foremost a env var to configure less (surprise ^^).
>
> If git (or anyone else) uses less internally, e.g. because they don't
> want to implement their own pager, fine... but then they cannot just
> blindly assume that LESS is set only for git's (or any other tool's
> needs).
You seem to be missing the presence of other enviroment variables,
namely $GIT_PAGER, which I already described in detail in my previous
reply. I'd appreciate if you'd read that description in detail, and
possibly test it a bit.
> So I'd say the proper way is rather that any such tool makes sure, that
> any options strictly required as set no matter what. Just as e.g. delta
> does.
Again, that's simply wrong and defeats the user's freedom of choice.
>> Well, fragile or not, that's the way it works. It has its downsides
>> for
>> sure, but it's all about having each utility handle the environment
>> carefully and document it in its man page(s), so the users can also
>> carefully craft the values of their customized environment variables.
>
> Sure, but from a user's view, the use of less (or anything else) within
> git is conceptually completely opaque.
Actually, it isn't, because there are $LESS, $PAGER and $GIT_PAGER
environment variables to customize the behavior.
> In less' manpage LESS isn't documented as "oh and you must make sure -R
> is included or otherwise git will break"...
Quite frankly, it would be silly to expect the less(1) man page to
mention something about git(1).
>> $LESS can be seen as a global set of the common options for less(1),
>
> o.O ... but, as I've described, one cannot really use it as that:
>
> If I globally set e.g. LESS="F" because my desire is to make less
> always exit as soon as the file fits on a screen, which I think is a
> reasonable thing to do, git would no longer add "R" and output would
> break.
Again, you seem not to understand well the distinction between the
global settings (i.e. $LESS and $PAGER) and the utility-specific
settings (e.g. $GIT_PAGER). Or you maybe simply refuse to understand
it, I don't know.
>> You don't have to define an alias, there's $GIT_PAGER for that
>> purpose, as I already explained above.
>
> Well, yes... and as I've said before, one could also solve it via
> git_config... but the problem stays the same... as soon as someone
> wants to use LESS as global less options just as you described it
> yourself, git will no longer worker properly because of the missing -R.
>
> And actually if one would use GIT_PAGER one would again defeat the
> purpose of a allegedly global options LESS, because unless one does
> something like GIT_PAGER="${LESS}R" it wouldn't see any changes made to
> LESS.
Let me clarify that the contents of $LESS is applied by less(1)
internally, so the final runtime configuration for less(1) is a sum of
the configurations made available through the $LESS and $PAGER (or
$GIT_PAGER) environment variables. It's a rather powerful approach, if
used properly.
>> Moreover, the whole idea of the various utilities touching the $LESS
>> variable internally is to provide sane defaults to the users that
>> don't
>> configure $LESS, $PAGER, etc. on their own.
>
> Then I don't see what the big problem would be to just do it via a
> command argument - if someone really has ever some reasons to remove --
> RAW‐CONTROL‐CHARS from the command options when less is invoked via git
> ... then he could still go into git_config and set that manually.
>
> But it would seem to me that the overall handling would be much more
> what one expects, than when doing the same via LESS.
Again, adding or modifying any command-line arguments by git itself
would defeat the purpose of the environment variables and prevent the
users from making their choice of the pagination configuration freely.
>> I don't know what delta is and how it actually paginates its outputs,
>> but it should follow the rules of the environment-based pager
>> configuration that I described in detail above.
>
> Well, AFAIU, it doesn't and for good reasons :-)
In that case, delta does it wrong, and I hope you understand why.
> Anyway... I think all necessary things have been said and this thread
> has grown far to large with only semi-related stuff... so thanks for
> all the replies why git uses "-X".
I hope all this was useful to you. It was useful to me. :)
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Kristoffer Haugsbakk @ 2023-10-13 15:07 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: entwicklung, Michael Strawbridge, Luben Tuikov, git
In-Reply-To: <20231013141437.ywrhw65xdapmev7d@pengutronix.de>
On Fri, Oct 13, 2023, at 16:14, Uwe Kleine-König wrote:
> $ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
> Use of uninitialized value $address in sprintf at
This looks the same problem as https://lore.kernel.org/git/ZQ1eGzqfyoeeTBUq@debian.me/
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v4 0/2] attr: add attr.tree config
From: John Cai @ 2023-10-13 15:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: John Cai via GitGitGadget, git, Jeff King, Jonathan Tan,
Eric Sunshine
In-Reply-To: <xmqqjzrskdzq.fsf@gitster.g>
Hi Junio,
On 11 Oct 2023, at 18:09, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06)
>> provided the ability to pass in a treeish as the attr source. When a
>> revision does not resolve to a valid tree is passed, Git will die. At
>> GitLab, we server repositories as bare repos and would like to always read
>> attributes from the default branch, so we'd like to pass in HEAD as the
>> treeish to read gitattributes from on every command. In this context we
>> would not want Git to die if HEAD is unborn, like in the case of empty
>> repositories.
>>
>> Instead of modifying the default behavior of --attr-source, create a new
>> config attr.tree with which an admin can configure a ref for all commands to
>> read gitattributes from. Also make the default tree to read from HEAD on
>> bare repositories.
>>
>> Changes since v2:
>>
>> * relax the restrictions around attr.tree so that if it does not resolve to
>> a valid treeish, ignore it.
>> * add a commit to default to HEAD in bare repositories
>>
>> Changes since v1:
>>
>> * Added a commit to add attr.tree config
>
> THis is v4 so there must be some changes since v3 that we are missing?
Oops I messed up the ordering of changes here. I'll fix in the (hopefully) final
re-roll
>
>> Range-diff vs v3:
>>
>> 1: cef206d47c7 ! 1: eaa27c47810 attr: read attributes from HEAD when bare repo
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>> +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
>> + test_when_finished rm -rf test bare_with_gitattribute &&
>> + git init test &&
>> -+ (
>> -+ cd test &&
>> -+ test_commit gitattributes .gitattributes "f/path test=val"
>> -+ ) &&
>> ++ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
>
> OK.
>
>> 2: dadb822da99 ! 2: 749d8a8082e attr: add attr.tree for setting the treeish to read attributes from
>> @@ Documentation/config.txt: other popular tools, and describe them in your documen
>>
>> ## Documentation/config/attr.txt (new) ##
>> @@
>> -+attr.tree:
>> -+ A <tree-ish> to read gitattributes from instead of the worktree. See
>> -+ linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,
>> -+ treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take
>> -+ precedence over attr.tree.
>> ++attr.tree::
>> ++ A reference to a tree in the repository from which to read attributes,
>> ++ instead of the `.gitattributes` file in the working tree. In a bare
>> ++ repository, this defaults to `HEAD:.gitattributes`. If the value does
>> ++ not resolve to a valid tree object, an empty tree is used instead.
>> ++ When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
>> ++ command line option are used, this configuration variable has no effect.
>
> OK.
>
>> -+ if (!default_attr_source_tree_object_name) {
>> ++ if (!default_attr_source_tree_object_name && git_attr_tree) {
>> + default_attr_source_tree_object_name = git_attr_tree;
>> + ignore_bad_attr_tree = 1;
>> + }
>
> Makes sense.
>
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>>
>> +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
>> +
>> ++test_expect_success '--attr-source is bad' '
>> ++ test_when_finished rm -rf empty &&
>> ++ git init empty &&
>> ++ (
>> ++ cd empty &&
>> ++ echo "$bad_attr_source_err" >expect_err &&
>> ++ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
>> ++ test_cmp expect_err err
>> ++ )
>> ++'
>
> OK. We fail when explicitly given a bad attr-source.
>
>> +test_expect_success 'attr.tree when HEAD is unborn' '
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> + cd empty &&
>> -+ echo $bad_attr_source_err >expect_err &&
>> + echo "f/path: test: unspecified" >expect &&
>> + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
>> + test_must_be_empty err &&
>
> But we silently ignore when given via a configuration variable.
>
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>> + git init empty &&
>> + (
>> + cd empty &&
>> -+ echo $bad_attr_source_err >expect_err &&
>> + echo "f/path: test: unspecified" >expect &&
>> + git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
>> + test_must_be_empty err &&
>
> Ditto. Is this any different from the above? Both points at an
> object that does not exist. If one were pointing at an object that
> does not exist (e.g., HEAD before the initial commit) and the other
> were pointing at an object that is not a tree-ish (e.g., a blob),
> then having two separate tests may make sense, but otherwise, I am
> not sure about the value proposition of the second test.
Yeah looking at it now, this test seems like it doesn't add much.
>
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
>> test_cmp expect actual
>> '
>>
>> -+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
>> ++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
>> + test_when_finished rm -rf empty &&
>> + git init empty &&
>> + (
>> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
>> + test_commit "val2" .gitattributes "f/path test=val2" &&
>> + git checkout attr-source &&
>> + echo "f/path: test: val1" >expect &&
>> -+ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
>> ++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
>> ++ check-attr test -- f/path >actual &&
>> + test_cmp expect actual &&
>> -+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
>> ++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
>> ++ check-attr test -- f/path >actual &&
>> + test_cmp expect actual
>> + )
>> +'
>
> Looking good.
>
> Thanks. Queued.
thanks!
John
^ permalink raw reply
* [PATCH 0/2] [Outreachy][PATCH v2] branch.c: adjust error messages to coding guidelines
From: Isoken June Ibizugbe via GitGitGadget @ 2023-10-13 15:33 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe
As per the CodingGuidelines document, it is recommended that a single-line
message provided to error messages such as die(), error() and warning(),
should start with a lowercase letter and should not end with a period.
Signed-off-by: Isoken June Ibizugbe isokenjune@gmail.com
Isoken June Ibizugbe (2):
branch.c: ammend error messages for die()
branch.c: adjust error messages to coding guidelines
builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 33 deletions(-)
base-commit: aab89be2eb6ca51eefeb8c8066f673f447058856
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1590%2FJunie06%2Famend-error-mesg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1590/Junie06/amend-error-mesg-v1
Pull-Request: https://github.com/git/git/pull/1590
--
gitgitgadget
^ permalink raw reply
* [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Isoken June Ibizugbe via GitGitGadget @ 2023-10-13 15:33 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe, Isoken June Ibizugbe
In-Reply-To: <pull.1590.git.git.1697211227.gitgitgadget@gmail.com>
From: Isoken June Ibizugbe <isokenjune@gmail.com>
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index a756543d644..e7ee9bd0f15 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
(head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
if (merged)
warning(_("deleting branch '%s' that has been merged to\n"
- " '%s', but not yet merged to HEAD."),
+ " '%s', but not yet merged to HEAD"),
name, reference_name);
else
warning(_("not deleting branch '%s' that is not yet merged to\n"
- " '%s', even though it is merged to HEAD."),
+ " '%s', even though it is merged to HEAD"),
name, reference_name);
}
free(reference_name_to_free);
@@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
{
struct commit *rev = lookup_commit_reference(the_repository, oid);
if (!force && !rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
+ error(_("couldn't look up commit object for '%s'"), refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
+ error(_("the branch '%s' is not fully merged.\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'"), branchname, branchname);
return -1;
}
return 0;
@@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "branch.%s", branchname);
if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
+ warning(_("update of config-file failed"));
strbuf_release(&buf);
}
@@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("Cannot delete branch '%s' "
+ error(_("cannot delete branch '%s' "
"used by worktree at '%s'"),
bname.buf, path);
ret = 1;
@@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
&oid, &flags);
if (!target) {
if (remote_branch) {
- error(_("remote-tracking branch '%s' not found."), bname.buf);
+ error(_("remote-tracking branch '%s' not found"), bname.buf);
} else {
char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
char *virtual_target = resolve_refdup(virtual_name,
@@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
"Did you forget --remote?"),
bname.buf);
else
- error(_("branch '%s' not found."), bname.buf);
+ error(_("branch '%s' not found"), bname.buf);
FREE_AND_NULL(virtual_target);
}
ret = 1;
@@ -630,17 +630,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (recovery) {
if (copy)
- warning(_("Created a copy of a misnamed branch '%s'"),
+ warning(_("created a copy of a misnamed branch '%s'"),
interpreted_oldname);
else
- warning(_("Renamed a misnamed branch '%s' away"),
+ warning(_("renamed a misnamed branch '%s' away"),
interpreted_oldname);
}
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated"), newname);
strbuf_release(&logmsg);
@@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
error((!argc || branch_checked_out(branch_ref.buf))
- ? _("No commit on branch '%s' yet.")
- : _("No branch named '%s'."),
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"),
branch_name);
else if (!edit_branch_description(branch_name))
ret = 0; /* happy */
@@ -893,7 +893,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
die(copy? _("cannot copy the current branch while not on any")
- : _("cannot rename the current branch while not on any."));
+ : _("cannot rename the current branch while not on any"));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
else if (argc == 2)
@@ -965,7 +965,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] branch.c: ammend error messages for die()
From: Isoken June Ibizugbe via GitGitGadget @ 2023-10-13 15:33 UTC (permalink / raw)
To: git; +Cc: Isoken June Ibizugbe, Isoken June Ibizugbe
In-Reply-To: <pull.1590.git.git.1697211227.gitgitgadget@gmail.com>
From: Isoken June Ibizugbe <isokenjune@gmail.com>
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
builtin/branch.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a7..a756543d644 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch %s is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch %s is being bisected at %s"),
target, wt->path);
}
}
@@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (ref_exists(oldref.buf))
recovery = 1;
else
- die(_("Invalid branch name: '%s'"), oldname);
+ die(_("invalid branch name: '%s'"), oldname);
}
for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
if (oldref_usage & IS_HEAD)
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -624,9 +624,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && !(oldref_usage & IS_ORPHAN) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
@@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy && (oldref_usage & IS_HEAD) &&
replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to %s, but HEAD is not updated!"), newname);
strbuf_release(&logmsg);
strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldref);
strbuf_release(&newref);
strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
@@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -892,7 +892,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
+ die(copy? _("cannot copy the current branch while not on any")
: _("cannot rename the current branch while not on any."));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
@@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || branch_checked_out(branch->refname))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Isoken Ibizugbe @ 2023-10-13 15:40 UTC (permalink / raw)
Cc: git
In-Reply-To: <91e4ad3984147fcc277254a3f6836bf79f5c9550.1697211227.git.gitgitgadget@gmail.com>
On Fri, Oct 13, 2023 at 4:33 PM Isoken June Ibizugbe via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Isoken June Ibizugbe <isokenjune@gmail.com>
>
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
> builtin/branch.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index a756543d644..e7ee9bd0f15 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
> (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
> if (merged)
> warning(_("deleting branch '%s' that has been merged to\n"
> - " '%s', but not yet merged to HEAD."),
> + " '%s', but not yet merged to HEAD"),
> name, reference_name);
> else
> warning(_("not deleting branch '%s' that is not yet merged to\n"
> - " '%s', even though it is merged to HEAD."),
> + " '%s', even though it is merged to HEAD"),
> name, reference_name);
> }
> free(reference_name_to_free);
> @@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
> {
> struct commit *rev = lookup_commit_reference(the_repository, oid);
> if (!force && !rev) {
> - error(_("Couldn't look up commit object for '%s'"), refname);
> + error(_("couldn't look up commit object for '%s'"), refname);
> return -1;
> }
> if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
> - error(_("The branch '%s' is not fully merged.\n"
> + error(_("the branch '%s' is not fully merged.\n"
> "If you are sure you want to delete it, "
> - "run 'git branch -D %s'."), branchname, branchname);
> + "run 'git branch -D %s'"), branchname, branchname);
> return -1;
> }
> return 0;
> @@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
> struct strbuf buf = STRBUF_INIT;
> strbuf_addf(&buf, "branch.%s", branchname);
> if (git_config_rename_section(buf.buf, NULL) < 0)
> - warning(_("Update of config-file failed"));
> + warning(_("update of config-file failed"));
> strbuf_release(&buf);
> }
>
> @@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> if (kinds == FILTER_REFS_BRANCHES) {
> const char *path;
> if ((path = branch_checked_out(name))) {
> - error(_("Cannot delete branch '%s' "
> + error(_("cannot delete branch '%s' "
> "used by worktree at '%s'"),
> bname.buf, path);
> ret = 1;
> @@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> &oid, &flags);
> if (!target) {
> if (remote_branch) {
> - error(_("remote-tracking branch '%s' not found."), bname.buf);
> + error(_("remote-tracking branch '%s' not found"), bname.buf);
> } else {
> char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
> char *virtual_target = resolve_refdup(virtual_name,
> @@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> "Did you forget --remote?"),
> bname.buf);
> else
> - error(_("branch '%s' not found."), bname.buf);
> + error(_("branch '%s' not found"), bname.buf);
> FREE_AND_NULL(virtual_target);
> }
> ret = 1;
> @@ -630,17 +630,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
> if (recovery) {
> if (copy)
> - warning(_("Created a copy of a misnamed branch '%s'"),
> + warning(_("created a copy of a misnamed branch '%s'"),
> interpreted_oldname);
> else
> - warning(_("Renamed a misnamed branch '%s' away"),
> + warning(_("renamed a misnamed branch '%s' away"),
> interpreted_oldname);
> }
>
> if (!copy && (oldref_usage & IS_HEAD) &&
> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> logmsg.buf))
> - die(_("branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated"), newname);
>
> strbuf_release(&logmsg);
>
> @@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
> if (!ref_exists(branch_ref.buf))
> error((!argc || branch_checked_out(branch_ref.buf))
> - ? _("No commit on branch '%s' yet.")
> - : _("No branch named '%s'."),
> + ? _("no commit on branch '%s' yet")
> + : _("no branch named '%s'"),
> branch_name);
> else if (!edit_branch_description(branch_name))
> ret = 0; /* happy */
> @@ -893,7 +893,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> die(_("branch name required"));
> else if ((argc == 1) && filter.detached)
> die(copy? _("cannot copy the current branch while not on any")
> - : _("cannot rename the current branch while not on any."));
> + : _("cannot rename the current branch while not on any"));
> else if (argc == 1)
> copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> else if (argc == 2)
> @@ -965,7 +965,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
>
> if (track == BRANCH_TRACK_OVERRIDE)
> --
> gitgitgadget
I am sorry if I made any mistakes, It's my first time using
gitgitgadget. This commit was already sent before through git-send
email. The patch was intended to submit the revisions.
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Junio C Hamano @ 2023-10-13 16:39 UTC (permalink / raw)
To: Sebastian Thiel; +Cc: git, Josh Triplett, Kristoffer Haugsbakk
In-Reply-To: <9C4A2AFD-AAA2-4ABA-8A8B-2133FD870366@icloud.com>
Sebastian Thiel <sebastian.thiel@icloud.com> writes:
> But if there is now a prefix, I feel that it might as well be chosen so that it
> is easier to remember and/or less likely to cause conflicts.
Another criteria is that it is not very often used in real
pathnames, of course, and '!' and '$' are good ones.
Come to think of it, we might be able to retrofit '!' without too
much damage. Something like "!unignored" is now a deprecated but
still supported way to say "!!unignored", "!*precious" is new, and
"\!anything" is a pathname that begins with '!'.
> Yes, I think my paragraph above is exactly that but with examples to practice
> the new syntax-proposal.
OK.
^ permalink raw reply
* Re: [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Junio C Hamano @ 2023-10-13 17:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
In-Reply-To: <ZSkCGS3JPEQ71dOF@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> actually exists in the object database. It's the only callsite of that
> function outside of "commit-graph.c", as all other external callers
> would call `lookup_commit_in_graph()` which _does_ perform the object
> existence check.
>
> So I think that the proper way to address the regression would be a
> patch similar to the following:
>
> diff --git a/commit.c b/commit.c
> index b3223478bc..109e9217e3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,
> return -1;
> if (item->object.parsed)
> return 0;
> - if (use_commit_graph && parse_commit_in_graph(r, item))
> + if (use_commit_graph && parse_commit_in_graph(r, item)) {
> + if (!has_object(r, &item->object.oid, 0))
> + return quiet_on_missing ? -1 :
> + error(_("commit %s exists in commit-graph but not in the object database"),
> + oid_to_hex(&item->object.oid));
> return 0;
> + }
It is (overly) conservative, which I generally should find pleasing,
but as I said, for secondary information like commit-graph that is
derived from the primary source only to precompute for optimization,
our general attitude should be to trust it and let the optimization
kick in, unless the operation being performed primarily cares about
the case where the result from using and not using the secondary
source differs. An obvious example of such an operation is "fsck",
where we do care and want to notice when the underlying object graph
and what commit-graph records contradict with each other. And my
suggestion to disable commit-graph while running the "rev-list"
command with the "--missing" option is because that usage would fall
into the same category (please correct me if that is not the case) [*].
So for the purpose of "rev-list --missing", the above change does
take us closer to the answer we would get from the primary source,
but isn't it pessimising other users unnecessarily? Do we have a
rough idea (if we have a benchmark that would be even better) how
much this lack of has_object() check is contributing to the
performance benefit of using commit-graph? Majority of callers of
this code should not have to pay the additional overhead here, so
unless it is small enough, this would be pessimising the generic
code for everybody to please "rev-list --missing", which is why I am
worried about going in this direction.
^ permalink raw reply
* [PATCH v5 1/2] attr: read attributes from HEAD when bare repo
From: John Cai via GitGitGadget @ 2023-10-13 17:39 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai, John Cai
In-Reply-To: <pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
The motivation for 44451a2e5e (attr: teach "--attr-source=<tree>" global
option to "git" , 2023-05-06), was to make it possible to use
gitattributes with bare repositories.
To make it easier to read gitattributes in bare repositories however,
let's just make HEAD:.gitattributes the default. This is in line with
how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare
repositories, 2012-12-13).
Signed-off-by: John Cai <johncai86@gmail.com>
---
attr.c | 12 +++++++++++-
t/t0003-attributes.sh | 11 +++++++++++
t/t5001-archive-attr.sh | 2 +-
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 71c84fbcf86..bf2ea1626a6 100644
--- a/attr.c
+++ b/attr.c
@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate,
}
static const char *default_attr_source_tree_object_name;
+static int ignore_bad_attr_tree;
void set_git_attr_source(const char *tree_object_name)
{
@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name &&
+ startup_info->have_repository &&
+ is_bare_repository()) {
+ default_attr_source_tree_object_name = "HEAD";
+ ignore_bad_attr_tree = 1;
+ }
+
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
- if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))
+ if (repo_get_oid_treeish(the_repository,
+ default_attr_source_tree_object_name,
+ attr_source) && !ignore_bad_attr_tree)
die(_("bad --attr-source or GIT_ATTR_SOURCE"));
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..5665cdc079f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -342,6 +342,17 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+
+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
+ test_when_finished rm -rf test bare_with_gitattribute &&
+ git init test &&
+ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
+ git clone --bare test bare_with_gitattribute &&
+ echo "f/path: test: val" >expect &&
+ git -C bare_with_gitattribute check-attr test -- f/path >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 0ff47a239db..eaf959d8f63 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' '
'
test_expect_missing bare-worktree/ignored
-test_expect_exists bare-worktree/ignored-by-tree
+test_expect_missing bare-worktree/ignored-by-tree
test_expect_exists bare-worktree/ignored-by-worktree
test_expect_success 'export-subst' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 0/2] attr: add attr.tree config
From: John Cai via GitGitGadget @ 2023-10-13 17:39 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai
In-Reply-To: <pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com>
44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06)
provided the ability to pass in a treeish as the attr source. When a
revision does not resolve to a valid tree is passed, Git will die. At
GitLab, we server repositories as bare repos and would like to always read
attributes from the default branch, so we'd like to pass in HEAD as the
treeish to read gitattributes from on every command. In this context we
would not want Git to die if HEAD is unborn, like in the case of empty
repositories.
Instead of modifying the default behavior of --attr-source, create a new
config attr.tree with which an admin can configure a ref for all commands to
read gitattributes from. Also make the default tree to read from HEAD on
bare repositories.
Changes since v4:
* removed superfluous test
Changes since v3:
* clarified attr logic around ignoring errors if source set by attr.tree is
invalid
* refactored tests by using helpers
* modified test to check for precedence between --attr-source, attr.tree,
GIT_ATTR_SOURCE
Changes since v2:
* relax the restrictions around attr.tree so that if it does not resolve to
a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositories
* remove commit that adds attr.allowInvalidSource
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: read attributes from HEAD when bare repo
attr: add attr.tree for setting the treeish to read attributes from
Documentation/config.txt | 2 +
Documentation/config/attr.txt | 7 ++++
attr.c | 19 ++++++++-
attr.h | 2 +
config.c | 16 ++++++++
t/t0003-attributes.sh | 72 +++++++++++++++++++++++++++++++++++
t/t5001-archive-attr.sh | 2 +-
7 files changed, 118 insertions(+), 2 deletions(-)
create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v5
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v4:
1: eaa27c47810 = 1: eaa27c47810 attr: read attributes from HEAD when bare repo
2: 749d8a8082e ! 2: df4b3f53309 attr: add attr.tree for setting the treeish to read attributes from
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ )
+'
+
-+test_expect_success 'attr.tree points to non-existing ref' '
-+ test_when_finished rm -rf empty &&
-+ git init empty &&
-+ (
-+ cd empty &&
-+ echo "f/path: test: unspecified" >expect &&
-+ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
-+ test_must_be_empty err &&
-+ test_cmp expect actual
-+ )
-+'
-+
+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
--
gitgitgadget
^ permalink raw reply
* [PATCH v5 2/2] attr: add attr.tree for setting the treeish to read attributes from
From: John Cai via GitGitGadget @ 2023-10-13 17:39 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Eric Sunshine, John Cai, John Cai
In-Reply-To: <pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git",
2023-05-06) provided the ability to pass in a treeish as the attr
source. In the context of serving Git repositories as bare repos like we
do at GitLab however, it would be easier to point --attr-source to HEAD
for all commands by setting it once.
Add a new config attr.tree that allows this.
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/config.txt | 2 ++
Documentation/config/attr.txt | 7 ++++
attr.c | 7 ++++
attr.h | 2 ++
config.c | 16 +++++++++
t/t0003-attributes.sh | 61 +++++++++++++++++++++++++++++++++++
6 files changed, 95 insertions(+)
create mode 100644 Documentation/config/attr.txt
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.
include::config/advice.txt[]
+include::config/attr.txt[]
+
include::config/core.txt[]
include::config/add.txt[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..1a482d6af2b
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -0,0 +1,7 @@
+attr.tree::
+ A reference to a tree in the repository from which to read attributes,
+ instead of the `.gitattributes` file in the working tree. In a bare
+ repository, this defaults to `HEAD:.gitattributes`. If the value does
+ not resolve to a valid tree object, an empty tree is used instead.
+ When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
+ command line option are used, this configuration variable has no effect.
diff --git a/attr.c b/attr.c
index bf2ea1626a6..e62876dfd3e 100644
--- a/attr.c
+++ b/attr.c
@@ -24,6 +24,8 @@
#include "tree-walk.h"
#include "object-name.h"
+const char *git_attr_tree;
+
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
static const char git_attr__unknown[] = "(builtin)unknown";
@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name && git_attr_tree) {
+ default_attr_source_tree_object_name = git_attr_tree;
+ ignore_bad_attr_tree = 1;
+ }
+
if (!default_attr_source_tree_object_name &&
startup_info->have_repository &&
is_bare_repository()) {
diff --git a/attr.h b/attr.h
index 2b745df4054..127998ae013 100644
--- a/attr.h
+++ b/attr.h
@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);
/* Return whether the system gitattributes file is enabled and should be used. */
int git_attr_system_is_enabled(void);
+extern const char *git_attr_tree;
+
#endif /* ATTR_H */
diff --git a/config.c b/config.c
index 3846a37be97..fb6a2db1d9b 100644
--- a/config.c
+++ b/config.c
@@ -18,6 +18,7 @@
#include "repository.h"
#include "lockfile.h"
#include "mailmap.h"
+#include "attr.h"
#include "exec-cmd.h"
#include "strbuf.h"
#include "quote.h"
@@ -1904,6 +1905,18 @@ static int git_default_mailmap_config(const char *var, const char *value)
return 0;
}
+static int git_default_attr_config(const char *var, const char *value)
+{
+ if (!strcmp(var, "attr.tree"))
+ return git_config_string(&git_attr_tree, var, value);
+
+ /*
+ * Add other attribute related config variables here and to
+ * Documentation/config/attr.txt.
+ */
+ return 0;
+}
+
int git_default_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
@@ -1927,6 +1940,9 @@ int git_default_config(const char *var, const char *value,
if (starts_with(var, "mailmap."))
return git_default_mailmap_config(var, value);
+ if (starts_with(var, "attr."))
+ return git_default_attr_config(var, value);
+
if (starts_with(var, "advice.") || starts_with(var, "color.advice"))
return git_default_advice_config(var, value);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 5665cdc079f..ecf43ab5454 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -40,6 +40,10 @@ attr_check_source () {
test_cmp expect actual &&
test_must_be_empty err
+ git $git_opts -c "attr.tree=$source" check-attr test -- "$path" >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+
GIT_ATTR_SOURCE="$source" git $git_opts check-attr test -- "$path" >actual 2>err &&
test_cmp expect actual &&
test_must_be_empty err
@@ -342,6 +346,43 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
+test_expect_success '--attr-source is bad' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "$bad_attr_source_err" >expect_err &&
+ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
+ test_cmp expect_err err
+ )
+'
+
+test_expect_success 'attr.tree when HEAD is unborn' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "f/path test=val" >.gitattributes &&
+ echo "f/path: test: val" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+ )
+'
test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
test_when_finished rm -rf test bare_with_gitattribute &&
@@ -353,6 +394,26 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
test_cmp expect actual
'
+test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ git checkout -b attr-source &&
+ test_commit "val1" .gitattributes "f/path test=val1" &&
+ git checkout -b attr-tree &&
+ test_commit "val2" .gitattributes "f/path test=val2" &&
+ git checkout attr-source &&
+ echo "f/path: test: val1" >expect &&
+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
+ check-attr test -- f/path >actual &&
+ test_cmp expect actual &&
+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
+ check-attr test -- f/path >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
From: Junio C Hamano @ 2023-10-13 18:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Taylor Blau
In-Reply-To: <b0bf576c51a706367a758b8e30eca37edb9c2734.1697200576.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> @@ -572,8 +572,13 @@ int repo_parse_commit_internal(struct repository *r,
> return -1;
> if (item->object.parsed)
> return 0;
> - if (use_commit_graph && parse_commit_in_graph(r, item))
> + if (use_commit_graph && parse_commit_in_graph(r, item)) {
> + if (!has_object(r, &item->object.oid, 0))
> + return quiet_on_missing ? -1 :
> + error(_("commit %s exists in commit-graph but not in the object database"),
> + oid_to_hex(&item->object.oid));
> return 0;
> + }
Ever since this codepath was introduced by 177722b3 (commit:
integrate commit graph with commit parsing, 2018-04-10), we blindly
trusted what commit-graph file says. This change is a strict
improvement in the correctness department, but there are two things
that are a bit worrying.
One. The additional check should certainly be cheaper than a full
reading and parsing of an object, either from a loose object file or
from a pack entry. It may not hurt performance too much, but it
still would give us more confidence if we know by how much we are
pessimising good cases where the commit-graph does match reality.
Our stance on these secondary files that store precomputed values
for optimization purposes is in general to use them blindly unless
in exceptional cases where the operation values the correctness even
when the validity of these secondary files is dubious (e.g., "fsck"),
and doing this extra check regardless of the caller at this low level
of the callchain is a bit worrying.
Another is that by the time parse_commit_in_graph() returns true and
we realize that the answer we got is bogus by asking has_object(),
item->object.parsed has already been toggled on, so the caller now
has a commit object that claimed it was already parsed and does not
match reality. Hopefully the caller takes an early exit upon seeing
a failure from parse_commit_gently() and the .parsed bit does not
matter, but maybe I am missing a case where it does. I dunno.
Other than that, sounds very sensible and the code change is clean.
Will queue. Thanks.
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index ba65f17dd9..25f8e9e2d3 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -821,4 +821,27 @@ test_expect_success 'overflow during generation version upgrade' '
> )
> '
>
> +test_expect_success 'commit exists in commit-graph but not in object database' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + test_commit A &&
> + test_commit B &&
> + test_commit C &&
> + git commit-graph write --reachable &&
> +
> + # Corrupt the repository by deleting the intermittent commit
> + # object. Commands should notice that this object is absent and
> + # thus that the repository is corrupt even if the commit graph
> + # exists.
> + oid=$(git rev-parse B) &&
> + rm .git/objects/"$(test_oid_to_path "$oid")" &&
> +
> + test_must_fail git rev-parse HEAD~2 2>error &&
> + grep "error: commit $oid exists in commit-graph but not in the object database" error
> + )
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-13 18:25 UTC (permalink / raw)
To: Isoken June Ibizugbe via GitGitGadget, git; +Cc: Isoken June Ibizugbe
In-Reply-To: <91e4ad3984147fcc277254a3f6836bf79f5c9550.1697211227.git.gitgitgadget@gmail.com>
On 13-oct-2023 15:33:47, Isoken June Ibizugbe via GitGitGadget wrote:
> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> logmsg.buf))
> - die(_("branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated"), newname);
Thanks. This change is not explicitly suggested in the guidelines, but I think
it fits well in the spirit of this series.
> @@ -965,7 +965,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
You have mistakenly deleted this full stop in the previous [1/2] patch.
Therefore, if you stop doing so, you do not need to add it here.
^ permalink raw reply
* Re: [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Junio C Hamano @ 2023-10-13 18:29 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git
In-Reply-To: <CAJHH8bE15VotFy7QQ-Npmfk9ePvf=-h5SP+Q8phrDBRj8Ti=hQ@mail.gmail.com>
Isoken Ibizugbe <isokenjune@gmail.com> writes:
> I am sorry if I made any mistakes, It's my first time using
> gitgitgadget. This commit was already sent before through git-send
> email. The patch was intended to submit the revisions.
I cannot help with GGG, but it looked strange for this change to be
in two patches, especially since what the rule of deciding which
message is to be touched in which one of the two patches were not
explained anywhere in the series.
I suspect what you want to do is to squash these two commits down
into a single commit, review the resulting code change and also the
commit log message to see if the latter still matches what the
combined patch does, update them as needed, run the test suite
again.
And then format-patch without the cover letter to send the resulting
patch out. Or force push to tell GGG to resend a new iteration
without any cover letter.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/2] [Outreachy][PATCH v2] branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-13 18:30 UTC (permalink / raw)
To: Isoken June Ibizugbe via GitGitGadget, git; +Cc: Isoken June Ibizugbe
In-Reply-To: <pull.1590.git.git.1697211227.gitgitgadget@gmail.com>
On 13-oct-2023 15:33:45, Isoken June Ibizugbe via GitGitGadget wrote:
> As per the CodingGuidelines document, it is recommended that a single-line
> message provided to error messages such as die(), error() and warning(),
> should start with a lowercase letter and should not end with a period.
>
> Signed-off-by: Isoken June Ibizugbe isokenjune@gmail.com
>
> Isoken June Ibizugbe (2):
> branch.c: ammend error messages for die()
> branch.c: adjust error messages to coding guidelines
I don't understand why two commits. Maybe it was unintentional and you
want to merge all changes into one commit, using git rebase.
>
> builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
> 1 file changed, 33 insertions(+), 33 deletions(-)
You still need to adjust the tests. Take a look at:
https://github.com/git/git/actions/runs/6509642887/job/17681596358
^ permalink raw reply
* [Outreachy Applicant]
From: estherugbiedah @ 2023-10-13 18:44 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: christian.couder@gmail.com
In-Reply-To: <355182457.15373219.1697222676128.ref@mail.yahoo.com>
Hello Git Community,
I am an Outreachy applicant for the 2023 cohort and I am thrilled to contribute to the Git project.
Having reviewed the available projects, I am particularly interested in this project. I believe that contributing to this project will not only enhance my skills but also allow me to make a meaningful impact.
I am ready and excited to immerse myself in the project and make valuable contributions.
Thank you for your guidance, and I look forward to working together on this exciting journey.
^ permalink raw reply
* Re: [PATCH v5 0/2] attr: add attr.tree config
From: Junio C Hamano @ 2023-10-13 18:52 UTC (permalink / raw)
To: John Cai via GitGitGadget
Cc: git, Jeff King, Jonathan Tan, Eric Sunshine, John Cai
In-Reply-To: <pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v4:
>
> * removed superfluous test
An alternative would have been to point with the ref some non-tree
object like a blob, but as the outcome should be the same as missing
case (from the code --- which is not exactly kosher), it should be
OK.
if (repo_get_oid_treeish(the_repository,
default_attr_source_tree_object_name,
attr_source) && !ignore_bad_attr_tree)
die(_("bad --attr-source or GIT_ATTR_SOURCE"));
OOPS! Sorry for not noticing earlier, but repo_get_oid_treeish()
does *NOT* error out when the discovered object is not a treeish, as
the suggested object type is merely supplied for disambiguation
purposes (e.g., with objects 012345 that is a tree and 012346 that
is a blob, you can still ask for treeish "01234" but if you ask for
an object "01234" it will fail).
So, the alternative test would have caught this bug, no? Instead of
silently treating the non-treeish as an empty tree, we would have
died much later when the object supposedly a tree-ish turns out to
be a blob, or something?
^ permalink raw reply
* [PATCH 0/8] chunk-format: introduce `pair_chunk_expect()` convenience API
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20231009205544.GA3281950@coredump.intra.peff.net>
While reviewing this series, I noticed a couple of spots that would be
helped by having a convenience function that stores the start of a
chunk in a designated location *after* checking that the chunk has the
expected size.
I called this function `pair_chunk_expect()` and touched up seven spots
that I think could benefit from having a convenience function like this.
This applies directly on top of 'jk/chunk-bounds'. Thanks in advance for
your review!
Taylor Blau (8):
chunk-format: introduce `pair_chunk_expect()` helper
commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
midx: read `OIDF` chunk with `pair_chunk_expect()`
midx: read `OIDL` chunk with `pair_chunk_expect()`
midx: read `OOFF` chunk with `pair_chunk_expect()`
chunk-format.c | 22 +++++++++++++++++
chunk-format.h | 12 +++++++++-
commit-graph.c | 65 +++++++++++++-------------------------------------
midx.c | 58 ++++++++++++--------------------------------
4 files changed, 65 insertions(+), 92 deletions(-)
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply
* [PATCH 1/8] chunk-format: introduce `pair_chunk_expect()` helper
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
In previous commits, the pair_chunk() interface grew a required "size"
pointer, so that the caller is forced to at least have a handle on the
actual size of the given chunk.
Many callers were converted to the new interface. A handful were instead
converted from the unsafe version of pair_chunk() to read_chunk() so
that they could check their expected size.
This led to a lot of code like:
static int graph_read_oid_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
if (chunk_size != 256 * sizeof(uint32_t))
return error("commit-graph oid fanout chunk is wrong size");
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
return 0;
}
, leaving the caller to use read_chunk(), like so:
read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
The callback to read_chunk() (in the above, `graph_read_oid_fanout()`)
does nothing more than (a) assign a pointer to the location of the start
of the chunk in the mmap'd file, and (b) assert that it has the correct
size.
For callers that know the expected size of their chunk(s) up-front, we
can simplify this by teaching the chunk-format API itself to validate
the expected size for us.
This is wrapped in a new function, called `pair_chunk_expect()` which
takes a "size_t" instead of a "size_t *", and validates that the given
chunk matches the expected size as given.
This will allow us to reduce the number of lines of code it takes to
perform these basic read_chunk() operations, by taking the above and
replacing it with something like:
if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT,
(const unsigned char **)&graph->chunk_oid_fanout,
256 * sizeof(uint32_t)))
error(_("commit-graph oid fanout chunk is wrong size"));
We will perform those transformations in the following commits.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
chunk-format.c | 22 ++++++++++++++++++++++
chunk-format.h | 12 +++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/chunk-format.c b/chunk-format.c
index cdc7f39b70..9550f15699 100644
--- a/chunk-format.c
+++ b/chunk-format.c
@@ -163,6 +163,8 @@ int read_table_of_contents(struct chunkfile *cf,
struct pair_chunk_data {
const unsigned char **p;
size_t *size;
+
+ size_t expected_size;
};
static int pair_chunk_fn(const unsigned char *chunk_start,
@@ -175,6 +177,17 @@ static int pair_chunk_fn(const unsigned char *chunk_start,
return 0;
}
+static int pair_chunk_expect_fn(const unsigned char *chunk_start,
+ size_t chunk_size,
+ void *data)
+{
+ struct pair_chunk_data *pcd = data;
+ if (pcd->expected_size != chunk_size)
+ return -1;
+ *pcd->p = chunk_start;
+ return 0;
+}
+
int pair_chunk(struct chunkfile *cf,
uint32_t chunk_id,
const unsigned char **p,
@@ -184,6 +197,15 @@ int pair_chunk(struct chunkfile *cf,
return read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);
}
+int pair_chunk_expect(struct chunkfile *cf,
+ uint32_t chunk_id,
+ const unsigned char **p,
+ size_t expected_size)
+{
+ struct pair_chunk_data pcd = { .p = p, .expected_size = expected_size };
+ return read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);
+}
+
int read_chunk(struct chunkfile *cf,
uint32_t chunk_id,
chunk_read_fn fn,
diff --git a/chunk-format.h b/chunk-format.h
index 14b76180ef..92c529d7ab 100644
--- a/chunk-format.h
+++ b/chunk-format.h
@@ -17,7 +17,8 @@ struct chunkfile;
*
* If reading a file, use a NULL 'struct hashfile *' and then call
* read_table_of_contents(). Supply the memory-mapped data to the
- * pair_chunk() or read_chunk() methods, as appropriate.
+ * pair_chunk(), pair_chunk_expect(), or read_chunk() methods, as
+ * appropriate.
*
* DO NOT MIX THESE MODES. Use different 'struct chunkfile' instances
* for reading and writing.
@@ -54,6 +55,15 @@ int pair_chunk(struct chunkfile *cf,
const unsigned char **p,
size_t *size);
+/*
+ * Similar to 'pair_chunk', but used for callers who have an expected
+ * size for the given 'chunk_id' in advance.
+ */
+int pair_chunk_expect(struct chunkfile *cf,
+ uint32_t chunk_id,
+ const unsigned char **p,
+ size_t expected_size);
+
typedef int (*chunk_read_fn)(const unsigned char *chunk_start,
size_t chunk_size, void *data);
/*
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 2/8] commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
The `OIDF` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
While we're at it, let's mark the error() string for translation.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 1f334987b5..cdefd7f926 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -321,16 +321,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)
return 0;
}
-static int graph_read_oid_fanout(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size != 256 * sizeof(uint32_t))
- return error("commit-graph oid fanout chunk is wrong size");
- g->chunk_oid_fanout = (const uint32_t *)chunk_start;
- return 0;
-}
-
static int graph_read_oid_lookup(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -462,7 +452,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
GRAPH_HEADER_SIZE, graph->num_chunks, 1))
goto free_and_return;
- read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT,
+ (const unsigned char **)&graph->chunk_oid_fanout,
+ 256 * sizeof(uint32_t)))
+ error(_("commit-graph oid fanout chunk is wrong size"));
read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 3/8] commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
Perform a similar conversion as in the previous commit read the CDAT
bits.
While we're here, mark the error() string for translation, and guard
against overflow when computing the expected size by wrapping it in an
st_mult() call.
Note that the pre-image of this patch was already sufficiently guarded
against overflow, since GRAPH_DATA_WIDTH is defined as
(the_hash_algo->rawsz + 16), so the expression in the parenthesis would
get performed as a size_t, and then g->num_commits would be promoted to
the width of size_t for the purposes of evaluating this expression.
But let's make it explicitly clear that this computation is safe by
wrapping it in an st_mult() call.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index cdefd7f926..97d4824673 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -330,16 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_commit_data(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)
- return error("commit-graph commit data chunk is wrong size");
- g->chunk_commit_data = chunk_start;
- return 0;
-}
-
static int graph_read_generation_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -457,7 +447,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
256 * sizeof(uint32_t)))
error(_("commit-graph oid fanout chunk is wrong size"));
read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
- read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_DATA,
+ &graph->chunk_commit_data,
+ st_mult(graph->num_commits, GRAPH_DATA_WIDTH)))
+ error(_("commit-graph commit data chunk is wrong size"));
pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,
&graph->chunk_extra_edges_size);
pair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox