* RE: libification: how to avoid symbol collisions?
From: rsbecker @ 2024-02-09 13:31 UTC (permalink / raw)
To: 'Kyle Lippincott', git
In-Reply-To: <CAO_smVji5aGjx1V-EGbumRRpOuGY0SkXZUn9g4LxKmMO3aw=Sg@mail.gmail.com>
On Thursday, February 8, 2024 9:30 PM, Kyle Lippincott wrote:
>While thinking about libification, I was wondering what we can/need to do about
>symbols (specifically functions, since our libraries will likely have few to no extern
>variables) that need to escape their translation unit (.c file) but that we don't want
>to risk colliding with symbols from our "host" project.
>
>For any header that we're offering up as an API boundary we can have prefixed
>names, but there are symbols from git-compat-util.h with simple and likely
>common names like `die`, `usage`, error`, etc. I'm far from an expert on linkers, but
>I'm under the impression that even though we'll only be #including git-compat-
>util.h in our own .c files (so the compiler for the host project wouldn't see them),
>the produced static library will still be "providing" these symbols unless we mark
>them as `static` (and if we mark them as `static`, they can't be used outside of their
>translation unit). This means that if the host project has a version of `die` (or links
>against yet another library that does), we run into what C++ calls a One Definition
>Rule (ODR)
>violation: we have two providers of the symbol `die` with different
>implementations, and the behavior is undefined (no error needs to be generated as
>far as I know).
>
>With dynamic libraries I believe that we have more control over what gets exposed,
>but I don't know of functionality for this when linking statically. I'm assuming there
>is no such functionality, as projects like openssl (ty Randall for mentioning this)
>appear to have a convention of prefixing the symbols they put in their "public" API
>(i.e. in non-internal header files) with things like OSSL_, and of prefixing the symbols
>they put in their "private" APIs that can't be marked as `static` with `ossl_`. I'd love
>to be wrong about this. :)
>
>If I'm right that this is an issue, does this imply that we'd need to rename every non-
>static function in the git codebase that's part of a library to have a `git_` prefix, even
>if it won't be used outside of the git project's own .c files? Is there a solution that
>doesn't involve making it so that we have to type `git_` a billion times a day that's
>also maintainable? We could try to guess at how likely a name collision would be
>and only do this for ones where it's obviously going to collide, but if we get it wrong,
>I'm concerned that we'll run into subtle ODR violations that *at best* erode
>confidence in our library, and can actually cause outages, data corruption, and
>security/privacy issues.
I think we only need to do this for functions that are in the libification code-base for non-static symbols (and any data elements that may end up in a DLL some day). The bulk of the non-libified code base would only need to adapt to new symbol names if those symbols are specifically moved. die(), error(), are probably going to be impacted, but they can be aliased with #defines internally to git to git_die() or git_error(), for example.
--Randall
^ permalink raw reply
* [PATCH] doc: git.txt-Fix inconsistency param description
From: 秃头灯笼鱼 via GitGitGadget @ 2024-02-09 11:51 UTC (permalink / raw)
To: git
Cc: 秃头灯笼鱼,
秃头灯笼鱼
From: =?UTF-8?q?=E7=A7=83=E5=A4=B4=E7=81=AF=E7=AC=BC=E9=B1=BC?=
<ttdlyu@163.com>
Signed-off-by: 秃头灯笼鱼 <ttdlyu@163.com>
---
Fix the inconsistency in the description of the namespace option's
parameter
Since the parameter of the --namespace option can contain path
separators \ and can be correctly parsed, I believe that
--namespace=<name> in the SYNOPSIS section should be changed to
--namespace=<path> to match the description in the OPTIONS section.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1669%2Fttdly%2Fdoc%2Fgit.txt-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1669/ttdly/doc/git.txt-v1
Pull-Request: https://github.com/git/git/pull/1669
Documentation/git.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 0d25224c969..eee277495bd 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
[--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
[-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
- [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+ [--git-dir=<path>] [--work-tree=<path>] [--namespace=<path>]
[--config-env=<name>=<envvar>] <command> [<args>]
DESCRIPTION
base-commit: 5216f8f5c4089ec29ce49afa147434c23e0f0163
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] prune: mark rebase autostash and orig-head as reachable
From: Phillip Wood @ 2024-02-09 11:08 UTC (permalink / raw)
To: Eric Sunshine, Phillip Wood via GitGitGadget
Cc: git, Orgad Shaneh, Phillip Wood
In-Reply-To: <CAPig+cRASFkTD6=YumZJKv7XJjr1asxKB-mAyBFox8tuwmNnFw@mail.gmail.com>
Hi Eric
On 08/02/2024 17:25, Eric Sunshine wrote:
> On Thu, Feb 8, 2024 at 12:00 PM Phillip Wood via GitGitGadget
>> +static int add_one_file(const char *path, struct rev_info *revs)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + if (!read_oneliner(&buf, path, READ_ONELINER_SKIP_IF_EMPTY)) {
>> + strbuf_release(&buf);
>> + return 0;
>> + }
>> + strbuf_trim(&buf);
>> + if (!get_oid_hex(buf.buf, &oid)) {
>> + object = parse_object_or_die(&oid, buf.buf);
>> + add_pending_object(revs, object, "");
>> + }
>> + return 0;
>> +}
>
> Is this leaking the strbuf? Should the function instead end with:
>
> strbuf_release(&buf);
> return 0;
Yes, well spotted
> Also, what is the significance of the return value of this function?
> All code paths seem to be returning 0 unconditionally, and the caller
> ignores the return value.
Good point, I think in an earlier draft it returned an error at one
point, I'll change the return types.
>> +/* Mark objects recored in rebase state files as reachable. */
>
> s/recored/recorded/
Sharp eyes as ever, thanks for looking at this I'll re-roll
Phillip
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Kristoffer Haugsbakk @ 2024-02-09 11:02 UTC (permalink / raw)
To: Marcus Tillmanns; +Cc: git@vger.kernel.org, Phillip Wood
In-Reply-To: <26317088-7020-43EF-8B60-41D719A6D145@qt.io>
On Fri, Feb 9, 2024, at 09:46, Marcus Tillmanns wrote:
> Everything you said is true. But it misses the point.
>
> If I try to commit for the first time, don’t want to set the global
> config, look at man git-commit and search for “author", it says
> “—author” to specify the author.
That reminds me of a mistake I once did. I was setting things up on a
different machine and I set `author.*` in my config. Then I got very
confused when `git commit` complained. I totally failed to see that I
need to set `user.*`, not `author.*`.
Ideally I think I should have gotten this error:
```
Committer identity unknown
The `author` config is set but not `committer`.
Did you mean to set `user` (author and committer)?
```
> Since “comitter” is such a hidden feature that even long time users of
> git don’t necessarily know about it, when I then specify “—author” and
> get the “same” error message again, I have no clue what’s going on,
> since I just specified my user name and email, and still I’m told it
> cannot be determined.
This is a very good point. The _committer_ is pretty well-hidden in
normal workflows. And probably irrelevant.
So when a user gets this error:
```
Committer identity unknown
*** Please tell me who you are.
```
And have never heard of “committer” before… what is she to think? I
think it’s very natural to conclude that “author” and “committer” are
the same thing. So she thinks:
“ Okay, so the program is complaining about the author not being
set. (It calls it “committer” here for some reason.) But I have set
the author…
Maybe this is another case of: it all makes perfect sense if you already
know all the concepts.
>> Your report would have been more clear if you included the error:
>
> Had I had any idea that the report was different between with / without
> —author I probably would have added it, or found out what the issue was.
You don’t know what you don’t know. That’s why it’s best to include all
context.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 0/2] minute "git bisect" doc updates
From: Christian Couder @ 2024-02-09 9:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20240207214436.538586-1-gitster@pobox.com>
On Wed, Feb 7, 2024 at 11:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "git bisect" documentation was a bit sketchy on alternative keywords
> "new" and "old", that are used to signal if a command is from the
> part of the history that is newer or older than "a significant
> event" the bisection is trying to find. Here are two small patches
> that improves the documentation.
>
> I am trying to flush my "stalled topics" queue. Here is a small and
> (hopefully) easy-to-finish one.
>
> The original discussion was from early December 2023 and can be
> found at
>
> https://lore.kernel.org/git/CAC4O8c9ieZC4SBJf54ZuTfAvnkhGuDaibBQ-m9Zw_n5VhUFPag@mail.gmail.com/
>
> Junio C Hamano (2):
> bisect: document "terms" subcommand more fully
> bisect: document command line arguments for "bisect start"
Both patches look good to me. You can add my "Reviewed-by:".
Thanks!
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Marcus Tillmanns @ 2024-02-09 8:46 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git@vger.kernel.org, Phillip Wood
In-Reply-To: <3c3db003-1506-47c4-a010-a8b783dff959@app.fastmail.com>
Everything you said is true. But it misses the point.
If I try to commit for the first time, don’t want to set the global config, look at man git-commit and search for “author", it says “—author” to specify the author.
Since “comitter” is such a hidden feature that even long time users of git don’t necessarily know about it, when I then specify “—author” and get the “same” error message again, I have no clue what’s going on, since I just specified my user name and email, and still I’m told it cannot be determined.
> On 9. Feb 2024, at 09:37, Kristoffer Haugsbakk <code@khaugsbakk.name> wrote:
>
> On Fri, Feb 9, 2024, at 08:43, Marcus Tillmanns wrote:
>> Uff, that’s a mean trap. Especially since there is no “—committer”
>> option as far as I can see.
>
> Your report would have been more clear if you included the error:
Had I had any idea that the report was different between with / without —author I probably would have added it, or found out what the issue was.
Cheers,
Marcus
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Kristoffer Haugsbakk @ 2024-02-09 8:37 UTC (permalink / raw)
To: Marcus Tillmanns; +Cc: git@vger.kernel.org, Phillip Wood
In-Reply-To: <60512662-9BE1-4DF7-A4E0-FD2E852E8E76@qt.io>
On Fri, Feb 9, 2024, at 08:43, Marcus Tillmanns wrote:
> Uff, that’s a mean trap. Especially since there is no “—committer”
> option as far as I can see.
Your report would have been more clear if you included the error:
```
$ git commit -m "Test" --author "My Name <my@email.com>"
Committer identity unknown
*** Please tell me who you are.
Run
git config --global user.email "you@example.com"
git config --global user.name "Your Name"
to set your account's default identity.
Omit --global to set the identity only in this repository.
fatal: unable to auto-detect email address (got '<machine>')
```
I guess there is no `--committer` since there are use-cases for
attributing the commit to someone else (like you just received a
patch or diff from someone) but there isn’t really a common use-case for
attributing the committer to someone else—the point of the “committer”
is to identify who did the commit itself.
And the error tells you about something that you would want to do in any
case. Because passing in the committer with
```
GIT_COMMITTER_NAME
GIT_COMMITTER_EMAIL
```
every time is a lot of work.
I guess the most common workflow is to configure your user globally. But
it’s often the case that you might have a work identity (like the
email). What I do is to try to remember to configure it locally in those
repositories as well. But in the most recent case I forgot about it and
had to rewrite the commits. :) So an alternative is to not set any
global user. And then you will get this complaint every time you start
work in a new repository:
```
Committer identity unknown
*** Please tell me who you are.
```
And then you can copy–paste one of your identities into the local
config. And be certain that you won’t make any commits with the wrong
identity.
That’s more work but also safer.
So I guess that “Please tell me who you are” might be a part of some
peoples’ usual repository setup workflow.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] Add ideas for GSoC 2024
From: Christian Couder @ 2024-02-09 8:36 UTC (permalink / raw)
To: Kaartic Sivaraam
Cc: Patrick Steinhardt, git, Taylor Blau, Junio C Hamano,
Victoria Dye, Karthik Nayak
In-Reply-To: <F2684AB9-C4F3-43C3-91F2-A6D7D71F4927@gmail.com>
Hi Kaartic,
On Thu, Feb 8, 2024 at 3:02 PM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> On 6 February 2024 1:43:02 pm IST, Christian Couder <christian.couder@gmail.com> wrote:
> >On Tue, Feb 6, 2024 at 6:51 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> >> I don't quite mind either way. I think overall we have enough tests that
> >> can be converted even if both projects got picked up separately. And the
> >> reftable unit tests are a bit more involved than the other tests given
> >> that their coding style doesn't fit at all into the Git project. So it's
> >> not like they can just be copied over, they definitely need some special
> >> care.
> >>
> >> Also, the technical complexity of the "reftable" backend is rather high,
> >> which is another hurdle to take.
> >>
> >> Which overall makes me lean more towards keeping this as a separate
> >> project now that I think about it.
>
> Makes sense. I suppose we need to capture the distinction more clearly in the ideas page.
>
> I've tweaked the doc for the same. Do check it out and feel free to suggest any corrections.
>
> Ideas page: https://git.github.io/SoC-2024-Ideas/
Thanks! It looks good to me too.
> >Ok, for me. If we have a contributor working on each of these 2
> >projects, we just need to be clear that the contributors should not
> >work together on the 2 projects as I think the GSoC forbids that.
>
> Indeed. We must make sure to communicate this to selected contributors if we end up choosing two of them for the unit test migration projects.
>
> On a related note, I think I could help as a co-mentor the non-reftable unit tests migration project if we don't find any other willing volunteer. :-)
>
> I'm hoping I should be of some help on guiding the contributor as a co-mentor. Feel free to let me correct me if I might potentially lack required knowledge.
Thanks a lot for volunteering to co-mentor with me! I think you don't
need any special knowledge and you will be very helpful as usual.
> >> > That said, how helpful would it be to link the following doc in the unit
> >> > testing related ideas?
> >> >
> >> > https://github.com/git/git/blob/master/Documentation/technical/unit-tests.txt
> >>
> >> Makes sense to me.
> >
> >To me too.
> >
> >> > Would it worth linking the reftable technical doc for the above ideas?
> >> >
> >> > https://git-scm.com/docs/reftable
> >> >
> >> > I could see it goes into a lot of detail. I'm just wondering if link to it
> >> > would help someone who's looking to learn about reftable.
> >>
> >> Definitely doesn't hurt.
> >
> >I agree.
>
> Thanks for the feedback. Included both of these links in relevant ideas too. Feel free to cross-check them!
Great, thanks!
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Konstantin Khomoutov @ 2024-02-09 8:21 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <60512662-9BE1-4DF7-A4E0-FD2E852E8E76@qt.io>
On Fri, Feb 09, 2024 at 07:43:01AM +0000, Marcus Tillmanns wrote:
> Uff, that’s a mean trap. Especially since there is no “--committer” option
> as far as I can see. Also the difference between the two error messages is
> just one (the first) word.
>
> Maybe that could be made more obvious, especially if the user specifies
> “--author” already.
There's a way to specify the author and e-mail on the command-line during a
single invocation:
git -c user.name=marcus -c user.email=user@example.com commit ...
This "-c" command-line option is explained in the "root" Git manual page
(run `git help git` for instance).
The reasoning behind having different author and committer is that Git has
been created to handle the development of Linux where most of the code flowed
(and still does, I presume) in in the form of patches sent to a set of mailing
lists, so the person creating a Git commit out of such a patch in most cases
not the person who has authored the change introduced by the patch (the
author).
Also note that you do not have to set the name and e-mail globally: Git has
three levels of configuration, one of which is "local" meaning it's stored in
the repository's metadata. So you can basically run
git config --local --add user.name marcus
to have this setting apply only to the repository which you "are in"
currently. This value will override any "global" (per-user) settings and any
"system" settings.
^ permalink raw reply
* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Patrick Steinhardt @ 2024-02-09 8:17 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <3f14077f-c70c-5eef-5b25-984fdf7b3b68@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]
On Fri, Feb 09, 2024 at 09:15:15AM +0100, Johannes Schindelin wrote:
> Hi Patrick,
>
> On Tue, 6 Feb 2024, Patrick Steinhardt wrote:
>
> > On Mon, Feb 05, 2024 at 02:35:53PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > [snip]
> > > diff --git a/rerere.c b/rerere.c
> > > index ca7e77ba68c..13c94ded037 100644
> > > --- a/rerere.c
> > > +++ b/rerere.c
> > > @@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,
> > > mmfile[i].ptr = repo_read_object_file(the_repository,
> > > &ce->oid, &type,
> > > &size);
> > > + if (!mmfile[i].ptr)
> > > + die(_("unable to read %s"),
> > > + oid_to_hex(&ce->oid));
> > > mmfile[i].size = size;
> > > }
> > > }
> >
> > A few lines below this we check whether `mmfile[i].ptr` is `NULL` and
> > replace it with the empty string if so. So this patch here is basically
> > a change in behaviour where we now die instead of falling back to the
> > empty value.
> >
> > I'm not familiar enough with the code to say whether the old behaviour
> > is intended or not -- it certainly feels somewhat weird to me. But it
> > did leave me wondering and could maybe use some explanation.
>
> Hmm. That's a good point. The `mmfile[i].ptr == NULL` situation is indeed
> handled specifically.
>
> However, after reading the code I come to the conclusion that the `i`
> refers to the stage of an index entry, i.e. that loop
> (https://github.com/git/git/blob/v2.43.0/rerere.c#L981-L983) handles the
> case where conflicts are due to deletions (where one side of the merge
> deleted the file) or double-adds (where both sides of the merge added the
> file, with different contents).
>
> Therefore I would suggest that ignoring missing blobs (as is the pre-patch
> behavior) would mishandle the available data and paper over a corruption
> of the database (the blob is reachable via the Git index, but is missing).
Yeah, that explanation sounds reasonable to me, thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Johannes Schindelin @ 2024-02-09 8:15 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <ZcHW_bc6N5umk2G4@tanuki>
Hi Patrick,
On Tue, 6 Feb 2024, Patrick Steinhardt wrote:
> On Mon, Feb 05, 2024 at 02:35:53PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [snip]
> > diff --git a/rerere.c b/rerere.c
> > index ca7e77ba68c..13c94ded037 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,
> > mmfile[i].ptr = repo_read_object_file(the_repository,
> > &ce->oid, &type,
> > &size);
> > + if (!mmfile[i].ptr)
> > + die(_("unable to read %s"),
> > + oid_to_hex(&ce->oid));
> > mmfile[i].size = size;
> > }
> > }
>
> A few lines below this we check whether `mmfile[i].ptr` is `NULL` and
> replace it with the empty string if so. So this patch here is basically
> a change in behaviour where we now die instead of falling back to the
> empty value.
>
> I'm not familiar enough with the code to say whether the old behaviour
> is intended or not -- it certainly feels somewhat weird to me. But it
> did leave me wondering and could maybe use some explanation.
Hmm. That's a good point. The `mmfile[i].ptr == NULL` situation is indeed
handled specifically.
However, after reading the code I come to the conclusion that the `i`
refers to the stage of an index entry, i.e. that loop
(https://github.com/git/git/blob/v2.43.0/rerere.c#L981-L983) handles the
case where conflicts are due to deletions (where one side of the merge
deleted the file) or double-adds (where both sides of the merge added the
file, with different contents).
Therefore I would suggest that ignoring missing blobs (as is the pre-patch
behavior) would mishandle the available data and paper over a corruption
of the database (the blob is reachable via the Git index, but is missing).
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Patrick Steinhardt @ 2024-02-09 8:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Phillip Wood, phillip.wood, git
In-Reply-To: <xmqq34u2onaj.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]
On Thu, Feb 08, 2024 at 09:53:24AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > As I said -- this is a bug, and I agree that it shouldn't happen. But
> > bugs happen, and especially with the new reftable format I expect them
> > to happen. What I look for in this context is to create the tools to fix
> > problems like this, but `--include-root-refs` doesn't. A flag that
> > unconditionally returns all refs, regardless of whether they have a bad
> > name or not, does address the issue. Think of it of more of a debugging
> > tool.
>
> OK, "--include-all-refs" would be fine. And without bugs there
> should not be a difference.
>
> Where does "all refs in this worktree" you mentioned fit in this
> picture? Should a bogus "ref/foo/bar" be considered to be worktree
> specific, or is it an incorrect attempt to create a ref that is
> specific to 'foo' worktree that is not the current one and be
> filtered out?
Good question indeed. The reason I said "all refs in this worktree" is
mostly that I don't think we should also be returning refs from other
worktrees. I consider their refdbs to be separate refdbs, even though it
is possible to access them via the special "worktrees/$wt/refs" syntax.
So I would say that such an option should return all refs of the current
worktree's refdb, plus the common worktree refs.
Another important question here is how the "files" backend should behave
with such a flag. Should it try to read all loose files as refs and
return those which just happen to look like one? That feels really wrong
to me, as we would now have to special case those namespaces where we
know that it's not a proper ref, like "worktrees/", "rebase-apply/" and
others.
The alternative would be to make it behave like `--include-root-refs`,
where we do a best effort and live with the fact that the "files"
backend cannot enumerate all refs it has created. This would mean that
behaviour between the "files" and "reftable" backend is diverging though
because the latter can trivially figure out all refs it contains. The
question is whether we are fine with that or not.
Depending on the answer, I think we can go one of two ways:
- Accept the diverging behaviour and add `--include-all-refs`. The
"files" backend does a best effort and only includes root refs, the
"reftable" backend lists all refs.
- Double down on the fact that refs must either be pseudo refs or
start with "refs/" and treat any ref that doesn't fit this bill as
corrupted. The consequence here would be that we instead go with
`--include-root-refs` that can be implemented the same for both
backends. In addition, we add checks to git-fsck(1) to surface and
flag refs with bogus names for the "reftable" backend.
While I seem to have convinced you that `--include-all-refs` might not
be a bad idea after all, you have convinced me by now that the second
option would be preferable. I'd be okay with either of these options as
both of them address the issue at hand.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Johannes Schindelin @ 2024-02-09 8:06 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <CAO_smVhrMn=-uF1B6+RA8A+VLCEN=o57zbQPtr8hpxRKY=qJRQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]
Hi Kyle,
On Mon, 5 Feb 2024, Kyle Lippincott wrote:
> On Mon, Feb 5, 2024 at 6:36 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > index e65cae0bcf7..caf20fd5bdd 100644
> > --- a/builtin/notes.c
> > +++ b/builtin/notes.c
> > @@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> > struct strbuf buf = STRBUF_INIT;
> > char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);
> >
> > - if (prev_buf && size)
> > + if (!prev_buf)
> > + die(_("unable to read %s"), oid_to_hex(note));
>
> This changes the behavior of this function. Previously, it would not
> add prev_buf output, but still succeed. This now dies.
It does change behavior. The previous behavior looked up the note OID,
then tried to read it, and if it was missing just pretended that there had
not been a note.
I'm not quite sure whether we should keep that behavior, as it is unclear
in which scenarios it would be desirable to paper over missing objects.
In GitGitGadget, I am a heavy user of notes and it wouldn't do any good to
have this behavior: It would lose information.
And even in scenarios where the `notes` ref is fetch shallowly, I would
expect all of the actual notes blobs to be present, and I would _want_ the
`git note edit ...` command to error out when that blob is not found.
Does that reasoning make sense to you?
Ciao,
Johannes
^ permalink raw reply
* Re: Bug: Commit fails when no global email address is set even though --author is used
From: Marcus Tillmanns @ 2024-02-09 7:43 UTC (permalink / raw)
To: phillip.wood@dunelm.org.uk; +Cc: git@vger.kernel.org
In-Reply-To: <51599394-3f75-4b75-a4c0-f13f117e73bc@gmail.com>
Uff, that’s a mean trap. Especially since there is no “—committer” option as far as I can see.
Also the difference between the two error messages is just one (the first) word.
Maybe that could be made more obvious, especially if the user specifies “—author” already.
Thanks for the info!
- Marcus
> On 8. Feb 2024, at 16:50, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> [You don't often get email from phillip.wood123@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Marcus
>
> On 08/02/2024 15:26, Marcus Tillmanns wrote:
>> What did you do before the bug happened? (Steps to reproduce your issue)
>>
>> * Set your machines hostname to a name that does not contain "." (e.g. "ihavenodotinmyhostname")
>> * Make sure you have no name or email configured in your global git config
>> * Create a new repository and "git add" a file
>> * Run: git commit -m "Test" --author "My Name <my@email.com>"
>>
>> What did you expect to happen? (Expected behavior)
>>
>> A commit should be created with author name "My Name", and author email "my@email.com"
>>
>> What happened instead? (Actual behavior)
>>
>> An error is thrown, complaining about not being able to determine the email address
>
> This is expected as "git commit" needs an identity to use for the
> committer as well as for the author. To set the committer you can use
> the GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL environment variables if
> you don't have the relevant config set and git cannot extract a domain
> from your hostname.
>
> Best Wishes
>
> Phillip
^ permalink raw reply
* [PATCH 7/7] t7003: ensure filter-branch prunes reflogs with the reftable backend
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]
In t7003 we conditionally check whether the reflog for branches pruned
by git-filter-branch(1) get deleted based on whether or not we use the
"files" backend. Same as with the preceding commit, this condition was
added because in its initial iteration the "reftable" backend did not
delete reflogs when their corresponding ref was deleted. Since then, the
backend has been aligned to behave the same as the "files" backend
though, which makes this check unnecessary.
Remove it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t7003-filter-branch.sh | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index f6aebe92ff..5ab4d41ee7 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -396,10 +396,7 @@ test_expect_success '--prune-empty is able to prune entire branch' '
git branch prune-entire B &&
git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
test_must_fail git rev-parse refs/heads/prune-entire &&
- if test_have_prereq REFFILES
- then
- test_must_fail git reflog exists refs/heads/prune-entire
- fi
+ test_must_fail git reflog exists refs/heads/prune-entire
'
test_expect_success '--remap-to-ancestor with filename filters' '
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 6/7] t2011: exercise D/F conflicts with HEAD with the reftable backend
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]
Some of the tests in t2011 exercise whether it is possible to move away
from a symbolic HEAD ref whose target ref has a directory-file conflict
with another, preexisting ref. These tests don't use git-symbolic-ref(1)
but manually write HEAD. This is supposedly done to avoid using logic
that we're about to exercise, but it makes it impossible to verify
whether the logic also works for ref backends other than "files".
Refactor the code to use git-symbolic-ref(1) instead so that the tests
work with the "reftable" backend, as well. We already have lots of tests
in t1404 that ensure that both git-update-ref(1) and git-symbolic-ref(1)
work in such a scenario, so it should be safe to rely on it here.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t2011-checkout-invalid-head.sh | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh
index 3c8135831b..04f53b1ea1 100755
--- a/t/t2011-checkout-invalid-head.sh
+++ b/t/t2011-checkout-invalid-head.sh
@@ -29,36 +29,33 @@ test_expect_success REFFILES 'checkout notices failure to lock HEAD' '
test_must_fail git checkout -b other
'
-test_expect_success REFFILES 'create ref directory/file conflict scenario' '
+test_expect_success 'create ref directory/file conflict scenario' '
git update-ref refs/heads/outer/inner main &&
-
- # do not rely on symbolic-ref to get a known state,
- # as it may use the same code we are testing
reset_to_df () {
- echo "ref: refs/heads/outer" >.git/HEAD
+ git symbolic-ref HEAD refs/heads/outer
}
'
-test_expect_success REFFILES 'checkout away from d/f HEAD (unpacked, to branch)' '
+test_expect_success 'checkout away from d/f HEAD (unpacked, to branch)' '
reset_to_df &&
git checkout main
'
-test_expect_success REFFILES 'checkout away from d/f HEAD (unpacked, to detached)' '
+test_expect_success 'checkout away from d/f HEAD (unpacked, to detached)' '
reset_to_df &&
git checkout --detach main
'
-test_expect_success REFFILES 'pack refs' '
+test_expect_success 'pack refs' '
git pack-refs --all --prune
'
-test_expect_success REFFILES 'checkout away from d/f HEAD (packed, to branch)' '
+test_expect_success 'checkout away from d/f HEAD (packed, to branch)' '
reset_to_df &&
git checkout main
'
-test_expect_success REFFILES 'checkout away from d/f HEAD (packed, to detached)' '
+test_expect_success 'checkout away from d/f HEAD (packed, to detached)' '
reset_to_df &&
git checkout --detach main
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 5/7] t1405: remove unneeded cleanup step
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]
In 5e00514745 (t1405: explictly delete reflogs for reftable, 2022-01-31)
we have added a test that explicitly deletes the reflog when not using
the "files" backend. This was required because back then, the "reftable"
backend didn't yet delete reflogs when deleting their corresponding
branches, and thus subsequent tests would fail because some unexpected
reflogs still exist.
The "reftable" backend was eventually changed though so that it behaves
the same as the "files" backend and deletes reflogs when deleting refs.
This was done to make the "reftable" backend behave like the "files"
backend as closely as possible so that it can act as a drop-in
replacement.
The cleanup-style test is thus not required anymore. Remove it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1405-main-ref-store.sh | 6 ------
1 file changed, 6 deletions(-)
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 976bd71efb..1183232a72 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -33,12 +33,6 @@ test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
test_must_fail git rev-parse refs/tags/new-tag --
'
-# In reftable, we keep the reflogs around for deleted refs.
-test_expect_success !REFFILES 'delete-reflog(FOO, refs/tags/new-tag)' '
- $RUN delete-reflog FOO &&
- $RUN delete-reflog refs/tags/new-tag
-'
-
test_expect_success 'rename_refs(main, new-main)' '
git rev-parse main >expected &&
$RUN rename-ref refs/heads/main refs/heads/new-main &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/7] t1404: make D/F conflict tests compatible with reftable backend
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6191 bytes --]
Some of the tests in t1404 exercise whether Git correctly aborts
transactions when there is a directory/file conflict with ref names.
While these tests are all marked to require the "files" backend, they do
in fact apply to the "reftable" backend as well.
This may not make much sense on the surface: D/F conflicts only exist
because the "files" backend uses the filesystem to store loose refs, and
thus the restriction theoretically shouldn't apply to the "reftable"
backend. But for now, the "reftable" backend artificially restricts the
creation of such conflicting refs so that it is a drop-in replacement
for the "files" backend. This also ensures that the "reftable" backend
can easily be used on the server side without causing issues for clients
which only know to use the "files" backend.
The only difference between the "files" and "reftable" backends is a
slightly different error message. Adapt the tests to accomodate for this
difference and remove the REFFILES prerequisite so that we start testing
with both backends.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1404-update-ref-errors.sh | 37 +++++++++++++++++-------------------
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 00b7013705..98e9158bd2 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -92,9 +92,6 @@ df_test() {
else
delname="$delref"
fi &&
- cat >expected-err <<-EOF &&
- fatal: cannot lock ref $SQ$addname$SQ: $SQ$delref$SQ exists; cannot create $SQ$addref$SQ
- EOF
$pack &&
if $add_del
then
@@ -103,7 +100,7 @@ df_test() {
printf "%s\n" "delete $delname" "create $addname $D"
fi >commands &&
test_must_fail git update-ref --stdin <commands 2>output.err &&
- test_cmp expected-err output.err &&
+ grep "fatal:\( cannot lock ref $SQ$addname$SQ:\)\? $SQ$delref$SQ exists; cannot create $SQ$addref$SQ" output.err &&
printf "%s\n" "$C $delref" >expected-refs &&
git for-each-ref --format="%(objectname) %(refname)" $prefix/r >actual-refs &&
test_cmp expected-refs actual-refs
@@ -191,69 +188,69 @@ test_expect_success 'one new ref is a simple prefix of another' '
'
-test_expect_success REFFILES 'D/F conflict prevents add long + delete short' '
+test_expect_success 'D/F conflict prevents add long + delete short' '
df_test refs/df-al-ds --add-del foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents add short + delete long' '
+test_expect_success 'D/F conflict prevents add short + delete long' '
df_test refs/df-as-dl --add-del foo foo/bar
'
-test_expect_success REFFILES 'D/F conflict prevents delete long + add short' '
+test_expect_success 'D/F conflict prevents delete long + add short' '
df_test refs/df-dl-as --del-add foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents delete short + add long' '
+test_expect_success 'D/F conflict prevents delete short + add long' '
df_test refs/df-ds-al --del-add foo foo/bar
'
-test_expect_success REFFILES 'D/F conflict prevents add long + delete short packed' '
+test_expect_success 'D/F conflict prevents add long + delete short packed' '
df_test refs/df-al-dsp --pack --add-del foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents add short + delete long packed' '
+test_expect_success 'D/F conflict prevents add short + delete long packed' '
df_test refs/df-as-dlp --pack --add-del foo foo/bar
'
-test_expect_success REFFILES 'D/F conflict prevents delete long packed + add short' '
+test_expect_success 'D/F conflict prevents delete long packed + add short' '
df_test refs/df-dlp-as --pack --del-add foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents delete short packed + add long' '
+test_expect_success 'D/F conflict prevents delete short packed + add long' '
df_test refs/df-dsp-al --pack --del-add foo foo/bar
'
# Try some combinations involving symbolic refs...
-test_expect_success REFFILES 'D/F conflict prevents indirect add long + delete short' '
+test_expect_success 'D/F conflict prevents indirect add long + delete short' '
df_test refs/df-ial-ds --sym-add --add-del foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents indirect add long + indirect delete short' '
+test_expect_success 'D/F conflict prevents indirect add long + indirect delete short' '
df_test refs/df-ial-ids --sym-add --sym-del --add-del foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents indirect add short + indirect delete long' '
+test_expect_success 'D/F conflict prevents indirect add short + indirect delete long' '
df_test refs/df-ias-idl --sym-add --sym-del --add-del foo foo/bar
'
-test_expect_success REFFILES 'D/F conflict prevents indirect delete long + indirect add short' '
+test_expect_success 'D/F conflict prevents indirect delete long + indirect add short' '
df_test refs/df-idl-ias --sym-add --sym-del --del-add foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents indirect add long + delete short packed' '
+test_expect_success 'D/F conflict prevents indirect add long + delete short packed' '
df_test refs/df-ial-dsp --sym-add --pack --add-del foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents indirect add long + indirect delete short packed' '
+test_expect_success 'D/F conflict prevents indirect add long + indirect delete short packed' '
df_test refs/df-ial-idsp --sym-add --sym-del --pack --add-del foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents add long + indirect delete short packed' '
+test_expect_success 'D/F conflict prevents add long + indirect delete short packed' '
df_test refs/df-al-idsp --sym-del --pack --add-del foo/bar foo
'
-test_expect_success REFFILES 'D/F conflict prevents indirect delete long packed + indirect add short' '
+test_expect_success 'D/F conflict prevents indirect delete long packed + indirect add short' '
df_test refs/df-idlp-ias --sym-add --sym-del --pack --del-add foo/bar foo
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/7] t1400: exercise reflog with gaps with reftable backend
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]
In t1400, we have a test that exercises whether we print a warning
message as expected when the reflog contains entries which have a gap
between the old entry's new object ID and the new entry's old object ID.
While the logic should apply to all ref backends, the test setup writes
into `.git/logs` directly and is thus "files"-backend specific.
Refactor the test to instead use `git reflog delete` to create the gap
and drop the REFFILES prerequisite.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1400-update-ref.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 3294b7ce08..3aeb103d34 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -426,15 +426,15 @@ test_expect_success 'Query "main@{2005-05-28}" (past end of history)' '
rm -f expect
git update-ref -d $m
-test_expect_success REFFILES 'query reflog with gap' '
+test_expect_success 'query reflog with gap' '
test_when_finished "git update-ref -d $m" &&
- git update-ref $m $F &&
- cat >.git/logs/$m <<-EOF &&
- $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
- $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
- $D $F $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
- EOF
+ GIT_COMMITTER_DATE="1117150320 -0500" git update-ref $m $A &&
+ GIT_COMMITTER_DATE="1117150380 -0500" git update-ref $m $B &&
+ GIT_COMMITTER_DATE="1117150480 -0500" git update-ref $m $C &&
+ GIT_COMMITTER_DATE="1117150580 -0500" git update-ref $m $D &&
+ GIT_COMMITTER_DATE="1117150680 -0500" git update-ref $m $F &&
+ git reflog delete $m@{2} &&
git rev-parse --verify "main@{2005-05-26 23:33:01}" >actual 2>stderr &&
echo "$B" >expect &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/7] t0410: enable tests with extensions with non-default repo format
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]
In t0410 we have two tests which exercise how partial clones behave in
the context of a repository with extensions. These tests are marked to
require a default repository using SHA1 and the "files" backend because
we explicitly set the repository format version to 0.
Changing the repository format version to 0 is not needed though. The
"noop" extension is ignored as expected regardless of what the version
is set to, same as the "nonsense" extension leads to failure regardless
of the version.
Stop setting the version so that these tests can execute with SHA256 and
"reftable" repositories.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t0410-partial-clone.sh | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 6b6424b3df..d913f3c453 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -49,24 +49,22 @@ test_expect_success 'convert shallow clone to partial clone' '
test_cmp_config -C client 1 core.repositoryformatversion
'
-test_expect_success SHA1,REFFILES 'convert to partial clone with noop extension' '
+test_expect_success 'convert to partial clone with noop extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
test_commit -C server my_commit2 1 &&
git clone --depth=1 "file://$(pwd)/server" client &&
- test_cmp_config -C client 0 core.repositoryformatversion &&
git -C client config extensions.noop true &&
git -C client fetch --unshallow --filter="blob:none"
'
-test_expect_success SHA1,REFFILES 'converting to partial clone fails with unrecognized extension' '
+test_expect_success 'converting to partial clone fails with unrecognized extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
test_commit -C server my_commit2 1 &&
git clone --depth=1 "file://$(pwd)/server" client &&
- test_cmp_config -C client 0 core.repositoryformatversion &&
git -C client config extensions.nonsense true &&
test_must_fail git -C client fetch --unshallow --filter="blob:none"
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/7] t: move tests exercising the "files" backend
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1707463221.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9305 bytes --]
We still have a bunch of tests scattered across our test suites that
exercise on-disk files of the "files" backend directly:
- t1301 exercises permissions of reflog files when the config
"core.sharedRepository" is set.
- t1400 exercises whether empty directories in the ref store are
handled correctly.
- t3200 exercises what happens when there are symlinks in the ref
store.
- t3400 also exercises what happens when ".git/logs" is a symlink.
All of these are inherently low-level tests specific to the "files"
backend. Move them into "t0600-reffiles-backend.sh" to reflect this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t0600-reffiles-backend.sh | 91 +++++++++++++++++++++++++++++++++++++
t/t1301-shared-repo.sh | 16 -------
t/t1400-update-ref.sh | 36 ---------------
t/t3200-branch.sh | 29 ------------
t/t3400-rebase.sh | 10 ----
5 files changed, 91 insertions(+), 91 deletions(-)
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index e6a5f1868f..485481d6b4 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -381,4 +381,95 @@ test_expect_success 'log diagnoses bogus HEAD symref' '
test_grep broken stderr
'
+test_expect_success 'empty directory removal' '
+ git branch d1/d2/r1 HEAD &&
+ git branch d1/r2 HEAD &&
+ test_path_is_file .git/refs/heads/d1/d2/r1 &&
+ test_path_is_file .git/logs/refs/heads/d1/d2/r1 &&
+ git branch -d d1/d2/r1 &&
+ test_must_fail git show-ref --verify -q refs/heads/d1/d2 &&
+ test_must_fail git show-ref --verify -q logs/refs/heads/d1/d2 &&
+ test_path_is_file .git/refs/heads/d1/r2 &&
+ test_path_is_file .git/logs/refs/heads/d1/r2
+'
+
+test_expect_success 'symref empty directory removal' '
+ git branch e1/e2/r1 HEAD &&
+ git branch e1/r2 HEAD &&
+ git checkout e1/e2/r1 &&
+ test_when_finished "git checkout main" &&
+ test_path_is_file .git/refs/heads/e1/e2/r1 &&
+ test_path_is_file .git/logs/refs/heads/e1/e2/r1 &&
+ git update-ref -d HEAD &&
+ test_must_fail git show-ref --verify -q refs/heads/e1/e2 &&
+ test_must_fail git show-ref --verify -q logs/refs/heads/e1/e2 &&
+ test_path_is_file .git/refs/heads/e1/r2 &&
+ test_path_is_file .git/logs/refs/heads/e1/r2 &&
+ test_path_is_file .git/logs/HEAD
+'
+
+test_expect_success 'directory not created deleting packed ref' '
+ git branch d1/d2/r1 HEAD &&
+ git pack-refs --all &&
+ test_path_is_missing .git/refs/heads/d1/d2 &&
+ git update-ref -d refs/heads/d1/d2/r1 &&
+ test_path_is_missing .git/refs/heads/d1/d2 &&
+ test_path_is_missing .git/refs/heads/d1
+'
+
+test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
+ git branch --create-reflog u &&
+ mv .git/logs/refs/heads/u real-u &&
+ ln -s real-u .git/logs/refs/heads/u &&
+ test_must_fail git branch -m u v
+'
+
+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
+ test_when_finished "rm -rf subdir" &&
+ git init --bare subdir &&
+
+ rm -rfv subdir/refs subdir/objects subdir/packed-refs &&
+ ln -s ../.git/refs subdir/refs &&
+ ln -s ../.git/objects subdir/objects &&
+ ln -s ../.git/packed-refs subdir/packed-refs &&
+
+ git -C subdir rev-parse --absolute-git-dir >subdir.dir &&
+ git rev-parse --absolute-git-dir >our.dir &&
+ ! test_cmp subdir.dir our.dir &&
+
+ git -C subdir log &&
+ git -C subdir branch rename-src &&
+ git rev-parse rename-src >expect &&
+ git -C subdir branch -m rename-src rename-dest &&
+ git rev-parse rename-dest >actual &&
+ test_cmp expect actual &&
+ git branch -D rename-dest
+'
+
+test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' '
+ git checkout main &&
+ mv .git/logs actual_logs &&
+ cmd //c "mklink /D .git\logs ..\actual_logs" &&
+ git rebase -f HEAD^ &&
+ test -L .git/logs &&
+ rm .git/logs &&
+ mv actual_logs .git/logs
+'
+
+test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+ umask 077 &&
+ git config core.sharedRepository group &&
+ git reflog expire --all &&
+ actual="$(ls -l .git/logs/refs/heads/main)" &&
+ case "$actual" in
+ -rw-rw-*)
+ : happy
+ ;;
+ *)
+ echo Ooops, .git/logs/refs/heads/main is not 066x [$actual]
+ false
+ ;;
+ esac
+'
+
test_done
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 8e2c01e760..b1eb5c01b8 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,22 +137,6 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
test_cmp expect actual
'
-test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
- umask 077 &&
- git config core.sharedRepository group &&
- git reflog expire --all &&
- actual="$(ls -l .git/logs/refs/heads/main)" &&
- case "$actual" in
- -rw-rw-*)
- : happy
- ;;
- *)
- echo Ooops, .git/logs/refs/heads/main is not 066x [$actual]
- false
- ;;
- esac
-'
-
test_expect_success POSIXPERM 'forced modes' '
test_when_finished "rm -rf new" &&
mkdir -p templates/hooks &&
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index f18843bf7a..3294b7ce08 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -288,33 +288,6 @@ test_expect_success "set $m (logged by touch)" '
test $A = $(git show-ref -s --verify $m)
'
-test_expect_success REFFILES 'empty directory removal' '
- git branch d1/d2/r1 HEAD &&
- git branch d1/r2 HEAD &&
- test_path_is_file .git/refs/heads/d1/d2/r1 &&
- test_path_is_file .git/logs/refs/heads/d1/d2/r1 &&
- git branch -d d1/d2/r1 &&
- test_must_fail git show-ref --verify -q refs/heads/d1/d2 &&
- test_must_fail git show-ref --verify -q logs/refs/heads/d1/d2 &&
- test_path_is_file .git/refs/heads/d1/r2 &&
- test_path_is_file .git/logs/refs/heads/d1/r2
-'
-
-test_expect_success REFFILES 'symref empty directory removal' '
- git branch e1/e2/r1 HEAD &&
- git branch e1/r2 HEAD &&
- git checkout e1/e2/r1 &&
- test_when_finished "git checkout main" &&
- test_path_is_file .git/refs/heads/e1/e2/r1 &&
- test_path_is_file .git/logs/refs/heads/e1/e2/r1 &&
- git update-ref -d HEAD &&
- test_must_fail git show-ref --verify -q refs/heads/e1/e2 &&
- test_must_fail git show-ref --verify -q logs/refs/heads/e1/e2 &&
- test_path_is_file .git/refs/heads/e1/r2 &&
- test_path_is_file .git/logs/refs/heads/e1/r2 &&
- test_path_is_file .git/logs/HEAD
-'
-
cat >expect <<EOF
$Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000 Initial Creation
$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000 Switch
@@ -1668,13 +1641,4 @@ test_expect_success PIPE 'transaction flushes status updates' '
test_cmp expected actual
'
-test_expect_success REFFILES 'directory not created deleting packed ref' '
- git branch d1/d2/r1 HEAD &&
- git pack-refs --all &&
- test_path_is_missing .git/refs/heads/d1/d2 &&
- git update-ref -d refs/heads/d1/d2/r1 &&
- test_path_is_missing .git/refs/heads/d1/d2 &&
- test_path_is_missing .git/refs/heads/d1
-'
-
test_done
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4..e36f4d15f2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -836,35 +836,6 @@ test_expect_success 'renaming a symref is not allowed' '
test_ref_missing refs/heads/new-topic
'
-test_expect_success SYMLINKS,REFFILES 'git branch -m u v should fail when the reflog for u is a symlink' '
- git branch --create-reflog u &&
- mv .git/logs/refs/heads/u real-u &&
- ln -s real-u .git/logs/refs/heads/u &&
- test_must_fail git branch -m u v
-'
-
-test_expect_success SYMLINKS,REFFILES 'git branch -m with symlinked .git/refs' '
- test_when_finished "rm -rf subdir" &&
- git init --bare subdir &&
-
- rm -rfv subdir/refs subdir/objects subdir/packed-refs &&
- ln -s ../.git/refs subdir/refs &&
- ln -s ../.git/objects subdir/objects &&
- ln -s ../.git/packed-refs subdir/packed-refs &&
-
- git -C subdir rev-parse --absolute-git-dir >subdir.dir &&
- git rev-parse --absolute-git-dir >our.dir &&
- ! test_cmp subdir.dir our.dir &&
-
- git -C subdir log &&
- git -C subdir branch rename-src &&
- git rev-parse rename-src >expect &&
- git -C subdir branch -m rename-src rename-dest &&
- git rev-parse rename-dest >actual &&
- test_cmp expect actual &&
- git branch -D rename-dest
-'
-
test_expect_success 'test tracking setup via --track' '
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 57f1392926..e1c8c5f701 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -424,16 +424,6 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '
test_grep "already used by worktree at" err
'
-test_expect_success REFFILES,MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' '
- git checkout main &&
- mv .git/logs actual_logs &&
- cmd //c "mklink /D .git\logs ..\actual_logs" &&
- git rebase -f HEAD^ &&
- test -L .git/logs &&
- rm .git/logs &&
- mv actual_logs .git/logs
-'
-
test_expect_success 'rebase when inside worktree subdirectory' '
git init main-wt &&
(
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/7] t: drop more REFFILES prereqs
From: Patrick Steinhardt @ 2024-02-09 7:23 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]
Hi,
this patch series is another step towards less tests with the REFFILES
prerequisite. Some of the tests are simply moved into t0600 because they
are inherently exercising low-level behaviour of the "files" backend.
Other tests are adapted so that they work across both the "files" and
the "reftable" backends.
Patrick
Patrick Steinhardt (7):
t: move tests exercising the "files" backend
t0410: enable tests with extensions with non-default repo format
t1400: exercise reflog with gaps with reftable backend
t1404: make D/F conflict tests compatible with reftable backend
t1405: remove unneeded cleanup step
t2011: exercise D/F conflicts with HEAD with the reftable backend
t7003: ensure filter-branch prunes reflogs with the reftable backend
t/t0410-partial-clone.sh | 6 +--
t/t0600-reffiles-backend.sh | 91 ++++++++++++++++++++++++++++++++
t/t1301-shared-repo.sh | 16 ------
t/t1400-update-ref.sh | 50 +++---------------
t/t1404-update-ref-errors.sh | 37 ++++++-------
t/t1405-main-ref-store.sh | 6 ---
t/t2011-checkout-invalid-head.sh | 17 +++---
t/t3200-branch.sh | 29 ----------
t/t3400-rebase.sh | 10 ----
t/t7003-filter-branch.sh | 5 +-
10 files changed, 125 insertions(+), 142 deletions(-)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] ref-filter.c: sort formatted dates by byte value
From: Junio C Hamano @ 2024-02-09 6:31 UTC (permalink / raw)
To: Victoria Dye; +Cc: Victoria Dye via GitGitGadget, git
In-Reply-To: <5ed018da-2150-42d8-995e-59a35a2e3821@github.com>
Victoria Dye <vdye@github.com> writes:
> Junio C Hamano wrote:
>>> I came across a use case for 'git for-each-ref' at $DAYJOB in which I'd
>>> want to sort by a portion of a formatted 'creatordate' (e.g., only the
>>> time of day, sans date). When I tried to run something like 'git
>>> for-each-ref --sort=creatordate:format:%H:%M:%S',
>>
>> Hmph, this indeed is interesting ;-)
>>
>> I wonder if there are other "sort by numeric but the thing could be
>> stringified by the end-user" atoms offered by for-each-ref
>> machinery. IOW, is the timestamp the only thing that needs this
>> fix?
>
> The only non-FIELD_STR atoms other than the date ones are "objectsize" and
> "numparent". "objectsize" has an optional ":disk" modifier, but that doesn't
> change formatting (just the value of the integer printed). "numparent"
> doesn't have any modifiers, it just prints the integer number of parents.
> Otherwise, everything is sorted by string value, so I think only the date
> atoms have this kind of mismatch between formatted value and sort value.
Thanks.
^ permalink raw reply
* Re: [PATCH] Add ideas for GSoC 2024
From: Patrick Steinhardt @ 2024-02-09 6:27 UTC (permalink / raw)
To: Kaartic Sivaraam
Cc: Christian Couder, git, Taylor Blau, Junio C Hamano, Victoria Dye,
Karthik Nayak
In-Reply-To: <F2684AB9-C4F3-43C3-91F2-A6D7D71F4927@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]
On Thu, Feb 08, 2024 at 07:32:50PM +0530, Kaartic Sivaraam wrote:
> Hi Patrick amd Christian,
>
>
> On 6 February 2024 1:43:02 pm IST, Christian Couder <christian.couder@gmail.com> wrote:
> >On Tue, Feb 6, 2024 at 6:51 AM Patrick Steinhardt <ps@pks.im> wrote:
> >> On Tue, Feb 06, 2024 at 12:25:31AM +0530, Kaartic Sivaraam wrote:
> >
> >> I don't quite mind either way. I think overall we have enough tests that
> >> can be converted even if both projects got picked up separately. And the
> >> reftable unit tests are a bit more involved than the other tests given
> >> that their coding style doesn't fit at all into the Git project. So it's
> >> not like they can just be copied over, they definitely need some special
> >> care.
> >>
> >> Also, the technical complexity of the "reftable" backend is rather high,
> >> which is another hurdle to take.
> >>
> >> Which overall makes me lean more towards keeping this as a separate
> >> project now that I think about it.
>
> Makes sense. I suppose we need to capture the distinction more
> clearly in the ideas page.
>
> I've tweaked the doc for the same. Do check it out and feel free to
> suggest any corrections.
>
> Ideas page: https://git.github.io/SoC-2024-Ideas/
Yeah, the clarification looks good to me. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] ref-filter.c: sort formatted dates by byte value
From: Victoria Dye @ 2024-02-09 2:46 UTC (permalink / raw)
To: Junio C Hamano, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <xmqqzfwbps43.fsf@gitster.g>
Junio C Hamano wrote:
>> I came across a use case for 'git for-each-ref' at $DAYJOB in which I'd
>> want to sort by a portion of a formatted 'creatordate' (e.g., only the
>> time of day, sans date). When I tried to run something like 'git
>> for-each-ref --sort=creatordate:format:%H:%M:%S',
>
> Hmph, this indeed is interesting ;-)
>
> I wonder if there are other "sort by numeric but the thing could be
> stringified by the end-user" atoms offered by for-each-ref
> machinery. IOW, is the timestamp the only thing that needs this
> fix?
The only non-FIELD_STR atoms other than the date ones are "objectsize" and
"numparent". "objectsize" has an optional ":disk" modifier, but that doesn't
change formatting (just the value of the integer printed). "numparent"
doesn't have any modifiers, it just prints the integer number of parents.
Otherwise, everything is sorted by string value, so I think only the date
atoms have this kind of mismatch between formatted value and sort value.
>
> Thanks.
^ 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