* Re: [ANNOUNCE] Git v2.44.0
From: Patrick Steinhardt @ 2024-02-24 6:36 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, git, git-packagers
In-Reply-To: <20240224051040.ftuo24smozqugbde@glandium.org>
[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]
On Sat, Feb 24, 2024 at 02:10:40PM +0900, Mike Hommey wrote:
> Hi,
>
> On Fri, Feb 23, 2024 at 09:17:07AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt (139):
> > builtin/clone: create the refdb with the correct object format
>
> I haven't analyzed how/why exactly yet, but I've bisected a regression
> in the behavior of is_git_directory() during a clone to originate from
> this change.
>
> Here's a way to reproduce the problem:
>
> ```
> $ cat > git-remote-foo <<EOF
> #!/bin/sh
> git config --local -l >&2
> exit 1
> EOF
> $ chmod +x git-remote-foo
> $ PATH=$PWD:$PATH git clone foo::bar
> ```
>
> With versions < 2.44.0, it displays the local configuration, e.g.:
> ```
> core.repositoryformatversion=0
> core.filemode=true
> core.bare=false
> core.logallrefupdates=true
> remote.origin.url=foo::bar
> ```
>
> but with 2.44.0, it fails with:
> ```
> fatal: --local can only be used inside a git repository
> ```
Thanks for your report!
This has to be because we now initialize the refdb at a later point. The
problem here was that before my change, we initialized the refdb at a
point when it wasn't clear what the remote actually used as the object
format. The consequence was twofold:
- Cloning a repository with bundles was broken in case the remote uses
the SHA256 object format.
- Cloning into a repository that uses reftables when the remote uses
the SHA256 object format was broken, too.
Both of these have the same root cause: because we didn't connect to the
remote yet we had no idea what object format the remote uses. And as we
initialized the refdb early, it was then initialized with the default
object format, which is SHA1.
The change was to move initialization of the refdb to a later point in
time where we know what object format the remote uses. By necessity,
this has to be _after_ we have connected to the remote, because there is
no way to learn about it without connecting to it.
One consequence of initializing the refdb at a later point in time is
that we have no "HEAD" yet, and a repo without the "HEAD" file is not
considered to be a repo. Thus, git-config(1) would now rightfully fail.
I assume that you discovered it via a remote helper that does something
more interesting than git-config(1). I have to wonder whether we ever
really specified what the environment of a remote helper should look
like when used during cloning. Conceptually it doesn't feel _wrong_ to
have a not-yet-initialized repo during clone.
But on the other hand, regressing functionality like this is of course
bad. I was wondering whether we can get around this issue by setting
e.g. GIT_DIR explicitly when spawning the remote helper, but I don't
think it's as easy as that.
Another idea would be to simply pre-create HEAD regardless of the ref
format, pointing to an invalid ref "refs/heads/.invalid". This is the
same trick we use for the reftable backend, and should likely address
your issue.
I will have a deeper look on Tuesday and send a patch.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Junio C Hamano @ 2024-02-24 7:58 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <CAO_smVh6PyxbnXfo0K1aDjEFPc3jTF4X_grerkxNZJdQe8V3sg@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
> As far as I can tell, we need pager.h because of the `pager_in_use`
> symbol. We need that symbol because of its use in date.c's
> `parse_date_format`. I wonder if we can side step the `#include
> <stdint.h>` concerns by splitting pager.h into pager.h and
> pager_in_use.h, and have pager.h include pager_in_use.h instead. This
> way pager.h (and its [unused] forward declarations) aren't part of
> git-std-lib at all.
Step back a bit. Why do you even need to touch pager.h in the first
place? Whatever thing that needs to define a mock version of
pager_in_use() would need to be able to find out that it is supposed
to take nothing as arguments and return an integer, and it can
include <pager.h> without modification. Just like everybody else,
it has to include <git-compat-util.h> so that the system header that
gives us uintmax_t gets include appropriately in platform-dependent
way, no? Why do we even need to butcher pager.h into two pieces in
the first place?
If you just include <git-compat-util.h> and then <pager.h> in
stubs/pager.c and you're OK, no?
If anything, as I already said, I think it is more reasonable to
tweak what <git-compat-util.h> does. For example, it might be
unwieldy for gitstdlib's purpose that it unconditionally overrides
exit(), in which case it may be OK to introduce some conditional
compilation macros to omit that override when building stub code.
Or even split parts of the <git-compat-util.h> that both Git's use
and gitstdlib's purpose are OK with into a separate header file
<git-compat-core.h>, while leaving (hopefully a very minor) other
parts in <git-compat-util.h> *and* include <git-compat-core.h> in
<git-compat-util.h>. That way, the sources of Git can continue
including <git-compat-util.h> while stub code can include
<git-compat-core.h>, and we will get system library symbols and
system defined types like uintmax_t in a consistent way, both in Git
itself and in gitstdlib.
But once such a sanitization is done on the compat-util header,
other "ordinary" header files that should not have to care about
portability (because they can assume that inclusion of
git-compat-util.h will give them access to system types and symbols
without having to worry about portability issues) and should not
have to include system header files themselves.
At least, that is the idea behind <git-compat-util.h> in the first
place. Including any system headers directly in ordinary headers,
or splitting ordinary headers at an arbitrary and artificial
boundary, should not be necessary. I'd have to say that such
changes are tail wagging the dog.
I do not have sufficient cycles to spend actually splitting
git-compat-util.h into two myself, but as an illustration, here is
how I would tweak cw/git-std-lib topic to make it build without
breaking our headers and including system header files directly.
git-compat-util.h | 2 ++
| 2 --
stubs/misc.c | 4 ++--
| 1 +
4 files changed, 5 insertions(+), 4 deletions(-)
diff --git c/git-compat-util.h w/git-compat-util.h
index 7c2a6538e5..981d526d18 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -1475,12 +1475,14 @@ static inline int is_missing_file_error(int errno_)
int cmd_main(int, const char **);
+#ifndef _GIT_NO_OVERRIDE_EXIT
/*
* Intercept all calls to exit() and route them to trace2 to
* optionally emit a message before calling the real exit().
*/
int common_exit(const char *file, int line, int code);
#define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
+#endif
/*
* You can mark a stack variable with UNLEAK(var) to avoid it being
--git c/pager.h w/pager.h
index 015bca95e3..b77433026d 100644
--- c/pager.h
+++ w/pager.h
@@ -1,8 +1,6 @@
#ifndef PAGER_H
#define PAGER_H
-#include <stdint.h>
-
struct child_process;
const char *git_pager(int stdout_is_tty);
diff --git c/stubs/misc.c w/stubs/misc.c
index 8d80581e39..d0379dcb69 100644
--- c/stubs/misc.c
+++ w/stubs/misc.c
@@ -1,5 +1,5 @@
-#include <assert.h>
-#include <stdlib.h>
+#define _GIT_NO_OVERRIDE_EXIT
+#include <git-compat-util.h>
#ifndef NO_GETTEXT
/*
--git c/stubs/pager.c w/stubs/pager.c
index 4f575cada7..04517aad4c 100644
--- c/stubs/pager.c
+++ w/stubs/pager.c
@@ -1,3 +1,4 @@
+#include <git-compat-util.h>
#include "pager.h"
int pager_in_use(void)
^ permalink raw reply related
* Re: [PATCH v2] rerere: fix crash during clear
From: Marcel Röthke @ 2024-02-24 11:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqplwsx730.fsf@gitster.g>
On 2024-02-19 17:22:43, Junio C Hamano wrote:
> Marcel Röthke <marcel@roethke.info> writes:
>
> > When rerere_clear is called, for instance when aborting a rebase, and
> > the current conflict does not have a pre or postimage recorded git
> > crashes with a SEGFAULT in has_rerere_resolution when accessing the
> > status member of struct rerere_dir.
>
> I had to read this twice before realizing the reason why I found it
> hard to grok was because of a missing comma between "recorded" and
> "git".
fixed
> > This happens because scan_rerere_dir
> > only allocates the status field in struct rerere_dir when a post or
> > preimage was found.
>
> But that is not really the root cause, no? Readers following the
> above text are probably wondering why the preimage was not recorded,
> when a conflict resulted in stopping a mergy-command and invoking
> rerere machinery, before rerere_clear() got called. Is that
> something that usually happen? How? Do we have a reproduction
> sequence of such a state that we can make it into a new test in
> t4200 where we already have tests for "git rerere clear" and its
> friends?
I'm unfortunately not sure how it happened. I do have the initial state
of the repository and I think I know the commands that were executed,
but I could not reproduce it.
I will look into adding a test case though.
^ permalink raw reply
* [GSoC][PATCH 0/1] Use unsigned integral type for collection of bits.
From: Eugenio Gigante @ 2024-02-24 11:26 UTC (permalink / raw)
To: git; +Cc: sunshine, gitster, Eugenio Gigante
One of the suggested microprojects for the GSoC is to pick a field
of signed integral type that is used as as collection of bits, and
change its type to unsigned in case the code does not take advantage
of its MSb.
This patch finds an example in 'builtin/add.c'.
Eugenio Gigante (1):
add: use unsigned type for collection of bits
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
--
2.43.0
^ permalink raw reply
* [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
From: Eugenio Gigante @ 2024-02-24 11:26 UTC (permalink / raw)
To: git; +Cc: sunshine, gitster, Eugenio Gigante
In-Reply-To: <20240224112638.72257-1-giganteeugenio2@gmail.com>
The function 'refresh' in 'builtin/add.c' declares 'flags' as signed,
while the function 'refresh_index' defined in 'read-cache-ll.h' expects an unsigned value.
Since in this case 'flags' represents a bag of bits, whose MSB is not used in special ways,
this commit changes the type of 'flags' to unsigned.
Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
---
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/add.c b/builtin/add.c
index ada7719561..393c10cbcf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
int i, ret = 0;
char *skip_worktree_seen = NULL;
struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
- int flags = REFRESH_IGNORE_SKIP_WORKTREE |
+ unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
(verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
seen = xcalloc(pathspec->nr, 1);
--
2.43.0
^ permalink raw reply related
* Re: Git in GSoC 2024
From: Kaartic Sivaraam @ 2024-02-24 17:34 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Christian Couder, Karthik Nayak, git, Taylor Blau, Junio C Hamano,
Victoria Dye
In-Reply-To: <ZddtyZqX1ME741j4@tanuki>
Hi Patrick, Karthik and Christian
On Thu, Feb 22, 2024 at 9:22 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Feb 22, 2024 at 07:35:33PM +0530, Kaartic Sivaraam wrote:
> > Hi Patrick, Karthik, Christian and all,
> >
> >
> > That's strange. Could you possibly try logging into the Summer of code website [3] directly in an incognito window using your GitLab account?
> >
> > I've previously faced issues with logging into the summer of code website due to an add-on blocking access to other Google domains. So, if you have add-ons that might block resources accessed by the website, could you possibly try disabling them?
> >
> > If you face issues despite all this, the only resort is to write to GSoC support about this issue at gsoc-support@google.com
> >
> > [3]: https://summerofcode.withgoogle.com/
>
> Things work now after a re-login. Kinda strange, but so be it. Thanks!
>
That's good to know! So, I suppose we're all set with adding Org Admins
and mentors for this year :-)
Just in case anyone hasn't come across it before, there are a few good
resources related to GSoC mentorship that Google has put up:
Google Summer of Code Mentor Guide [mentor-guide]
Mentor Roles and Responsibilities [mentor-responsibilities]
Feel free to check them out.
[[ References ]]
[mentor-guide]: https://google.github.io/gsocguides/mentor/index
[mentor-responsibilities]:
https://developers.google.com/open-source/gsoc/help/responsibilities#mentor_responsibilitie
--
Sivaraam
^ permalink raw reply
* Re: [PATCH 1/3] doc: git-rev-parse: enforce command-line description syntax
From: Jean-Noël AVILA @ 2024-02-24 18:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël Avila via GitGitGadget, git
In-Reply-To: <xmqqr0h4h2sz.fsf@gitster.g>
On Thursday, 22 February 2024 17:38:36 CET Junio C Hamano wrote:
> Jean-Noël AVILA <jn.avila@free.fr> writes:
>
> >> So, perhaps we do not have to do a lot of 'word' -> _word_
> >> replacements, hopefully?
>
> > ... At least, we
> > should try to stick as much as possible to the common markup _ for
emphasis.
>
> OK, that clears up my confusion. Thanks.
>
> We do not want to rely on an external party indefinitely maintaining
> what they consider backward compatibility wart, so the mark-up migration
> would need to happen before it becomes too late.
>
> > This would have the added benefit of differentiating single quotes from
> > backticks, because single quotes would completely disappear in the end,
except
> > when a real single quote is needed.
>
> Given enough time, yes. Or we can actively disable AsciiDoctor's
> compatibility mode and/or tweak asciidoc.conf to do the equivalent
> for AsciiDoc, to start early. Until then, we cannot really use "a
> real single quote", right?
The logic for managing single quotes as markup is that there should be a word
boundary at the quote <SPC>'<letter> for opening and <letter>'<SPC> for
closing, whereas for "real single quote" there's no space. This rule is
"natural" when writing in English and writers don't pay attention.
>
> > For the migration to "standard" asciidoc, I would also recommend using "="
> > prefix for titles instead of underlines which require changing two lines
when
> > modifying a title and are a pain for translators in languages with
variable
> > width characters.
>
> I personally strongly prefer the underline format because I care
> about readability of sources, but that is just me. Is that also
> getting deprecated?
>
The underline format is bound to be deprecated. Right now, Asciidoctor detects
this formatting to infer a switch to compat-mode. That's why markup-quote
works as expected with Asciidoctor in current documents.
> Thanks.
>
>
^ permalink raw reply
* [RFD] should "git log --graph -g" work and if so how?
From: Junio C Hamano @ 2024-02-24 19:04 UTC (permalink / raw)
To: git
Outside work, I keep one repository that keeps copies of a subset of
what is available elsewhere (but that is not version controlled in
any way), and in this repository, I do either one of two operations:
* I "fetch" the latest state of the things I keep track of from
that "elsewhere", and then record that state as a "snapshot".
* I update the "subset" I keep track of from that "elsewhere",
download that new part of the subset, and then record the result
as a commit with messages like "add 'foo'" or "drop 'bar'".
One curiosity is that I do not care too much about "snapshot"; the
latest state is often enough, so when the topmost one is a snapshot,
I may "amend" it when making a new "snapshot", instead of making two
or more consecutive "snapshot" commits (when the topmost "snapshot"
is too old, I may choose to add a new shapshot on top of it, but
let's ignore that for simplicity).
if I am doing a snapshot
then
fetch what I've been tracking from "elsewhere"
"git add ."
if the topmost commit is an earlier "snapshot"
then
"git commit --amend -m 'snapshot as of ...'"
else
"git commit -m 'snapshot as of ...'"
fi
elif I am adding new things to be tracked
then
fetch the new part of "elsewhere"
"git add ."
"git commit -m 'add ...'
fi
After I started from empty, started tracking 'foo' and then a few
days later started tracking 'bar', and then started taking snapshots
on 2024-02-22 and took one every day, I may end up with a history
that "git show-branch -g" may give me something like this:
$ git show-branch -g5
! [master@{0}] (24 minutes ago) commit {amend}: snapshot as of 2024-02-24
! [master@{1}] (1 day ago) commit (amend): snapshot as of 2024-02-23
! [master@{2}] (2 days ago) commit: snapshot as of 2024-02-22
! [master@{3}] (3 days ago) commit: add 'bar'
! [master@{4}] (7 days ago) commit: add 'foo'
-----
+ [master@{0}] snapshot as of 2024-02-24
+ [master@{1}] snapshot as of 2024-02-23
+ [master@{2}] snapshot as of 2024-02-22
++++ [master@{3}] add 'bar'
+++++ [master@{4}] add 'foo'
and that output is sort-of readable (if you have seen and know how
to read what show-branch produces, that is), but the command way
predates commit slabs and uses the usual object flag bits, so it is
limited to show only 25 or so commits [*1*].
Now, if I could run
$ git log --oneline --graph -g --since=2024-02-20 --boundary
on the result, such a history might look like this:
* snapshot as of 2024-02-24 (HEAD)
| * snapshot as of 2024-02-23 (HEAD@{1})
|/
| * snapshot as of 2024-02-22 (HEAD@{2})
|/
* add 'bar' (HEAD~1)
o add 'foo' (HEAD~2)
to show the same history.
Unfortunately, "--graph" and "-g" does not mix X-<.
So, the RFD is,
(1) Should "git log" learn a trick to show a history like this in a
readable way? Does it have utility outside this use case of
mine? I am not interested in adding a new feature just for
myself ;-)
(2) The use case requires a solution to look at reflog entries, but
it does not need to "walk" reflog [*2*]. Should such a feature
still be tied to the "-g" flag, or should we want a separate
flag?
(3) What should the UI and the implementation look like? "Show me
what happened to this branch since 2024-02-20, including what
is in reflog" that results in:
- we first enumerate commits on the reflog of this branch that
were made since the given date.
- we then pretend as if all of these commits were given on the
command line of "git log --oneline --graph", with some other
commits that probably are ancestors of these commits marked
as UNINTERESTING.
may be a promising approach to go. In the sample history
depicted above, we would want an equivalent to
$ git log --oneline --graph HEAD HEAD@{1} HEAD@{2} --not HEAD~2
where the positive commits are gathered by inspecting the
reflog for commits newer than 2024-02-20, and then list of
negative commits (HEAD~2 in this case) is somehow computed to
stop the usual "git log" traversal from these positive commits
after showing all of them (and before showing other commits not
in that initial set).
Thoughts?
[Footnotes]
*1* It may be an interesting side project to teach show-branch to
store the bits it uses to paint commits in commit slabs,
instead of using the object flags, to lift this limitation.
I'll not put the l e f t o v e r b i t s label on this item, as
it certainly is an interesting exercise but its usefulness is
rather dubious.
*2* "walking" reflog stresses the fact that HEAD@{2} came
immediately before HEAD@{1} which came immediately before
HEAD@{0} (or HEAD, which are equivalents), but in this use
case, it is equally (if not more) important that the snapshots
taken on 2024-02-22, 2024-02-23, and on 2024-02-24 are more or
less equals, with the latest one a bit more important than
others because it is on the branch while the other ones are not
and merely appear in the reflog.
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.44.0
From: Mike Hommey @ 2024-02-24 19:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, git, git-packagers
In-Reply-To: <ZdmOZRjJ-mClBR02@framework>
On Sat, Feb 24, 2024 at 07:36:21AM +0100, Patrick Steinhardt wrote:
> Thanks for your report!
>
> This has to be because we now initialize the refdb at a later point. The
> problem here was that before my change, we initialized the refdb at a
> point when it wasn't clear what the remote actually used as the object
> format. The consequence was twofold:
>
> - Cloning a repository with bundles was broken in case the remote uses
> the SHA256 object format.
>
> - Cloning into a repository that uses reftables when the remote uses
> the SHA256 object format was broken, too.
>
> Both of these have the same root cause: because we didn't connect to the
> remote yet we had no idea what object format the remote uses. And as we
> initialized the refdb early, it was then initialized with the default
> object format, which is SHA1.
>
> The change was to move initialization of the refdb to a later point in
> time where we know what object format the remote uses. By necessity,
> this has to be _after_ we have connected to the remote, because there is
> no way to learn about it without connecting to it.
>
> One consequence of initializing the refdb at a later point in time is
> that we have no "HEAD" yet, and a repo without the "HEAD" file is not
> considered to be a repo. Thus, git-config(1) would now rightfully fail.
>
> I assume that you discovered it via a remote helper that does something
> more interesting than git-config(1).
Indeed, my own usecase is a remote helper that uses libgit.a and uses
is_git_directory indirectly, but I could imagine other remote helpers that
could be using other git commands that rely on is_git_directory
returning true.
> I have to wonder whether we ever
> really specified what the environment of a remote helper should look
> like when used during cloning. Conceptually it doesn't feel _wrong_ to
> have a not-yet-initialized repo during clone.
How about this: it should look like what you'd get from
`git init $repo`.
> But on the other hand, regressing functionality like this is of course
> bad. I was wondering whether we can get around this issue by setting
> e.g. GIT_DIR explicitly when spawning the remote helper, but I don't
> think it's as easy as that.
GIT_DIR is already set when spawning the remote helper. My remote helper
is using setup_git_directory_gently and uses the value of nongit_ok for
the cases where the executable is used without being wrapped by git (it
provides extra commands), I guess I could use whether GIT_DIR is set as
a workaround.
> Another idea would be to simply pre-create HEAD regardless of the ref
> format, pointing to an invalid ref "refs/heads/.invalid". This is the
> same trick we use for the reftable backend, and should likely address
> your issue.
The interesting thing is that `git init $repo` does give you an invalid
HEAD (and that's what would happen during git clone too), with either
```
ref: refs/heads/master
```
or
```
ref: refs/heads/main
```
depending on configuration.
Mike
^ permalink raw reply
* [PATCH] doc: clarify the wording on <git-compat-util.h> requirement
From: Junio C Hamano @ 2024-02-24 20:22 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott, Calvin Wan, Jonathan Tan, Elijah Newren
The reason why we insist including the compat-util header as the
very first thing is because it is our mechanism to absorb the
differences across platforms, like the order in which system header
files must be included, and C preprocessor feature macros that are
needed to trigger certain features we want out of the systems, and
insulate other headers and sources from such minutiae.
Earlier we tried to clarify the rule in the coding guidelines
document, but the wording was a bit fuzzy that can lead to
misinterpretations like you can include xdiff/xinclude.h only to
avoid having to include git-compat-util.h file even if you have
nothing to do with xdiff implementation, for example. "You do not
have to include more than one of these" were also misleading and
would have been puzzling if you _needed_ to depend on more than one
of these approved headers (answer: you are allowed to include them
all if you need the declarations in them for reasons other than that
you want to avoid including compat-util yourself).
Instead of using the phrase "approved headers", enumerate them as
exceptions, each labeled with intended audiences, to avoid such
misinterpretations. The structure also makes it easier to add new
exceptions, so add the description of "t/unit-tests/test-lib.h"
being an exception only for the unit tests implementation as an
example.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* git-std-lib folks CC'ed to show them where to put their exception
when things start to stabilize; Elijah CC'ed for his 8bff5ca0
(treewide: ensure one of the appropriate headers is sourced
first, 2023-02-24) and bc5c5ec0 (cache.h: remove this
no-longer-used header, 2023-05-16).
Documentation/CodingGuidelines | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 578587a471..b3443dd773 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -446,12 +446,30 @@ For C programs:
detail.
- The first #include in C files, except in platform specific compat/
- implementations and sha1dc/, must be either "git-compat-util.h" or
- one of the approved headers that includes it first for you. (The
- approved headers currently include "builtin.h",
- "t/helper/test-tool.h", "xdiff/xinclude.h", or
- "reftable/system.h".) You do not have to include more than one of
- these.
+ implementations and sha1dc/, must be "git-compat-util.h". In
+ addition:
+
+ - the implementation of the built-in commands in the "builtin/"
+ directory that include "builtin.h" for the cmd_foo() prototype
+ definition
+
+ - the test helper programs in the "t/helper/" directory that include
+ "t/helper/test-tool.h" for the cmd__foo() prototype definition
+
+ - the xdiff implementation in the "xdiff/" directory that includes
+ "xdiff/xinclude.h" for the xdiff machinery internals
+
+ - the unit test programs in "t/unit-tests/" directory that include
+ "test-lib.h" that gives them the unit-tests framework
+
+ - the source files that implement reftable in the "reftable/"
+ directory that include "reftable/system.h" for the reftable
+ internals
+
+ are allowed to assume that their header file includes
+ "git-compat-util.h", and they do not have to include
+ "git-compat-util.h" themselves. These headers must be the first
+ header file to be "#include"d in them, though.
- A C file must directly include the header files that declare the
functions and the types it uses, except for the functions and types
^ permalink raw reply related
* [PATCH] compat: drop inclusion of <git-compat-util.h>
From: Junio C Hamano @ 2024-02-24 20:32 UTC (permalink / raw)
To: git
These two header files are included from ordinary source files that
already include <git-compat-util.h> as the first header file as they
should. There is no need to include the compat-util in these
headers.
"make hdr-check" is not affected, as it is designed to assume that
what <git-compat-util.h> offers is available to everybody without
being included.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* There is an obvious alternative that goes in the complete
opposite direction possible, to update "make hdr-check" to ensure
that things that are depended upon in each header file
(e.g. pager.h refers to uintmax_t) are brought in by the header
file to include the compat-util in it, i.e.
diff --git c/Makefile w/Makefile
index 78e874099d..d7b360f15e 100644
--- c/Makefile
+++ w/Makefile
@@ -3259,7 +3259,7 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
HCC = $(HCO:hco=hcc)
%.hcc: %.h
- @echo '#include "git-compat-util.h"' >$@
+ @echo '/* #include "git-compat-util.h" */' >$@
@echo '#include "$<"' >>$@
$(HCO): %.hco: %.hcc FORCE
which would require in a noisy diff to add inclusion of
git-compat-util.h to many header files. For purposes of folks
who may want to carve out only pieces of our source tree, such an
approach might work better, but for that to happen and yield any
useful result, I suspect that compat-util header needs to be
split into "compatibility essentials" and other "it is convenient
if these are available everywhere, even though they do not have
much to do with hiding system dependencies from the sources"
parts first.
compat/compiler.h | 1 -
compat/disk.h | 1 -
2 files changed, 2 deletions(-)
diff --git c/compat/compiler.h w/compat/compiler.h
index 10dbb65937..e9ad9db84f 100644
--- c/compat/compiler.h
+++ w/compat/compiler.h
@@ -1,7 +1,6 @@
#ifndef COMPILER_H
#define COMPILER_H
-#include "git-compat-util.h"
#include "strbuf.h"
#ifdef __GLIBC__
diff --git c/compat/disk.h w/compat/disk.h
index 6c979c27d8..23bc1bef86 100644
--- c/compat/disk.h
+++ w/compat/disk.h
@@ -1,7 +1,6 @@
#ifndef COMPAT_DISK_H
#define COMPAT_DISK_H
-#include "git-compat-util.h"
#include "abspath.h"
#include "gettext.h"
^ permalink raw reply related
* [PATCH 2/6] parse-options: set arg of abbreviated option lazily
From: René Scharfe @ 2024-02-24 21:29 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Postpone setting the opt pointer until we're about to call get_value(),
which uses it. There's no point in setting it eagerly for every
abbreviated candidate option, which may turn out to be ambiguous.
Removing this assignment from the loop doesn't noticeably improve the
performance, but allows further simplification.
---
parse-options.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index e4ce33ea48..056c6b30e9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -391,8 +391,6 @@ static enum parse_opt_result parse_long_opt(
ambiguous_option = abbrev_option;
ambiguous_flags = abbrev_flags;
}
- if (*arg_end)
- p->opt = arg_end + 1;
abbrev_option = options;
abbrev_flags = flags ^ opt_flags;
continue;
@@ -441,8 +439,11 @@ static enum parse_opt_result parse_long_opt(
abbrev_option->long_name);
return PARSE_OPT_HELP;
}
- if (abbrev_option)
+ if (abbrev_option) {
+ if (*arg_end)
+ p->opt = arg_end + 1;
return get_value(p, abbrev_option, abbrev_flags);
+ }
return PARSE_OPT_UNKNOWN;
}
--
2.44.0
^ permalink raw reply related
* [PATCH 3/6] parse-options: factor out register_abbrev() and struct parsed_option
From: René Scharfe @ 2024-02-24 21:29 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Add a function, register_abbrev(), for storing the necessary details for
remembering an abbreviated and thus potentially ambiguous option. Call
it instead of sharing the code using goto, to make the control flow more
explicit.
Conveniently collect these details in the new struct parsed_option to
reduce the number of necessary function arguments.
---
parse-options.c | 83 +++++++++++++++++++++++++++++--------------------
1 file changed, 49 insertions(+), 34 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 056c6b30e9..398ebaef14 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -350,14 +350,40 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
return 0;
}
+struct parsed_option {
+ const struct option *option;
+ enum opt_parsed flags;
+};
+
+static void register_abbrev(struct parse_opt_ctx_t *p,
+ const struct option *option, enum opt_parsed flags,
+ struct parsed_option *abbrev,
+ struct parsed_option *ambiguous)
+{
+ if (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)
+ return;
+ if (abbrev->option &&
+ !is_alias(p, abbrev->option, option)) {
+ /*
+ * If this is abbreviated, it is
+ * ambiguous. So when there is no
+ * exact match later, we need to
+ * error out.
+ */
+ ambiguous->option = abbrev->option;
+ ambiguous->flags = abbrev->flags;
+ }
+ abbrev->option = option;
+ abbrev->flags = flags;
+}
+
static enum parse_opt_result parse_long_opt(
struct parse_opt_ctx_t *p, const char *arg,
const struct option *options)
{
const char *arg_end = strchrnul(arg, '=');
- const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
- enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
- int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
+ struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };
+ struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
@@ -377,31 +403,20 @@ static enum parse_opt_result parse_long_opt(
rest = NULL;
if (!rest) {
/* abbreviated? */
- if (allow_abbrev &&
- !strncmp(long_name, arg, arg_end - arg)) {
-is_abbreviated:
- if (abbrev_option &&
- !is_alias(p, abbrev_option, options)) {
- /*
- * If this is abbreviated, it is
- * ambiguous. So when there is no
- * exact match later, we need to
- * error out.
- */
- ambiguous_option = abbrev_option;
- ambiguous_flags = abbrev_flags;
- }
- abbrev_option = options;
- abbrev_flags = flags ^ opt_flags;
+ if (!strncmp(long_name, arg, arg_end - arg)) {
+ register_abbrev(p, options, flags ^ opt_flags,
+ &abbrev, &ambiguous);
continue;
}
/* negation allowed? */
if (options->flags & PARSE_OPT_NONEG)
continue;
/* negated and abbreviated very much? */
- if (allow_abbrev && starts_with("no-", arg)) {
+ if (starts_with("no-", arg)) {
flags |= OPT_UNSET;
- goto is_abbreviated;
+ register_abbrev(p, options, flags ^ opt_flags,
+ &abbrev, &ambiguous);
+ continue;
}
/* negated? */
if (!starts_with(arg, "no-"))
@@ -409,12 +424,12 @@ static enum parse_opt_result parse_long_opt(
flags |= OPT_UNSET;
if (!skip_prefix(arg + 3, long_name, &rest)) {
/* abbreviated and negated? */
- if (allow_abbrev &&
- !strncmp(long_name, arg + 3,
+ if (!strncmp(long_name, arg + 3,
arg_end - arg - 3))
- goto is_abbreviated;
- else
- continue;
+ register_abbrev(p, options,
+ flags ^ opt_flags,
+ &abbrev, &ambiguous);
+ continue;
}
}
if (*rest) {
@@ -425,24 +440,24 @@ static enum parse_opt_result parse_long_opt(
return get_value(p, options, flags ^ opt_flags);
}
- if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+ if (disallow_abbreviated_options && (ambiguous.option || abbrev.option))
die("disallowed abbreviated or ambiguous option '%.*s'",
(int)(arg_end - arg), arg);
- if (ambiguous_option) {
+ if (ambiguous.option) {
error(_("ambiguous option: %s "
"(could be --%s%s or --%s%s)"),
arg,
- (ambiguous_flags & OPT_UNSET) ? "no-" : "",
- ambiguous_option->long_name,
- (abbrev_flags & OPT_UNSET) ? "no-" : "",
- abbrev_option->long_name);
+ (ambiguous.flags & OPT_UNSET) ? "no-" : "",
+ ambiguous.option->long_name,
+ (abbrev.flags & OPT_UNSET) ? "no-" : "",
+ abbrev.option->long_name);
return PARSE_OPT_HELP;
}
- if (abbrev_option) {
+ if (abbrev.option) {
if (*arg_end)
p->opt = arg_end + 1;
- return get_value(p, abbrev_option, abbrev_flags);
+ return get_value(p, abbrev.option, abbrev.flags);
}
return PARSE_OPT_UNKNOWN;
}
--
2.44.0
^ permalink raw reply related
* [PATCH 0/6] parse-options: long option parsing fixes and cleanup
From: René Scharfe @ 2024-02-24 21:29 UTC (permalink / raw)
To: git
Fix two corner cases in patches 1 and 4, refactor the code in patches 2
and 3 to prepare the second fix, bonus patches 5 and 6 simplify the code
and improve readability.
parse-options: recognize abbreviated negated option with arg
parse-options: set arg of abbreviated option lazily
parse-options: factor out register_abbrev() and struct parsed_option
parse-options: detect ambiguous self-negation
parse-options: normalize arg and long_name before comparison
parse-options: rearrange long_name matching code
parse-options.c | 137 ++++++++++++++++++----------------
t/t0040-parse-options.sh | 16 ++++
t/t1502-rev-parse-parseopt.sh | 11 +++
3 files changed, 100 insertions(+), 64 deletions(-)
--
2.44.0
^ permalink raw reply
* [PATCH 1/6] parse-options: recognize abbreviated negated option with arg
From: René Scharfe @ 2024-02-24 21:29 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Giving an argument to an option that doesn't take one causes Git to
report that error specifically:
$ git rm --dry-run=bogus
error: option `dry-run' takes no value
The same is true when the option is negated or abbreviated:
$ git rm --no-dry-run=bogus
error: option `no-dry-run' takes no value
$ git rm --dry=bogus
error: option `dry-run' takes no value
Not so when doing both, though:
$ git rm --no-dry=bogus
error: unknown option `no-dry=bogus'
usage: git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
(Rest of the usage message omitted.)
Improve consistency and usefulness of the error message by recognizing
abbreviated negated options even if they have a (most likely bogus)
argument. With this patch we get:
$ git rm --no-dry=bogus
error: option `no-dry-run' takes no value
---
parse-options.c | 5 +++--
t/t0040-parse-options.sh | 16 ++++++++++++++++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 63a99dea6e..e4ce33ea48 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -391,7 +391,7 @@ static enum parse_opt_result parse_long_opt(
ambiguous_option = abbrev_option;
ambiguous_flags = abbrev_flags;
}
- if (!(flags & OPT_UNSET) && *arg_end)
+ if (*arg_end)
p->opt = arg_end + 1;
abbrev_option = options;
abbrev_flags = flags ^ opt_flags;
@@ -412,7 +412,8 @@ static enum parse_opt_result parse_long_opt(
if (!skip_prefix(arg + 3, long_name, &rest)) {
/* abbreviated and negated? */
if (allow_abbrev &&
- starts_with(long_name, arg + 3))
+ !strncmp(long_name, arg + 3,
+ arg_end - arg - 3))
goto is_abbreviated;
else
continue;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ec974867e4..8bb2a8b453 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -210,6 +210,22 @@ test_expect_success 'superfluous value provided: boolean' '
test_cmp expect actual
'
+test_expect_success 'superfluous value provided: boolean, abbreviated' '
+ cat >expect <<-\EOF &&
+ error: option `yes'\'' takes no value
+ EOF
+ test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+ test-tool parse-options --ye=hi 2>actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ error: option `no-yes'\'' takes no value
+ EOF
+ test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+ test-tool parse-options --no-ye=hi 2>actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'superfluous value provided: cmdmode' '
cat >expect <<-\EOF &&
error: option `mode1'\'' takes no value
--
2.44.0
^ permalink raw reply related
* [PATCH 6/6] parse-options: rearrange long_name matching code
From: René Scharfe @ 2024-02-24 21:29 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Move the code for handling a full match of long_name first and get rid
of negations. Reduce the indent of the code for matching abbreviations
and remove unnecessary curly braces. Combine the checks for whether
negation is allowed and whether arg is "n", "no" or "no-" because they
belong together and avoid a continue statement. The result is shorter,
more readable code.
---
parse-options.c | 37 +++++++++++++++----------------------
1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index d45efa4e5c..30b9e68f8a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -413,30 +413,23 @@ static enum parse_opt_result parse_long_opt(
if (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)
continue;
- if (!skip_prefix(arg_start, long_name, &rest))
- rest = NULL;
- if (!rest) {
- /* abbreviated? */
- if (!strncmp(long_name, arg_start, arg_end - arg_start)) {
- register_abbrev(p, options, flags ^ opt_flags,
- &abbrev, &ambiguous);
- }
- /* negation allowed? */
- if (options->flags & PARSE_OPT_NONEG)
+ if (skip_prefix(arg_start, long_name, &rest)) {
+ if (*rest == '=')
+ p->opt = rest + 1;
+ else if (*rest)
continue;
- /* negated and abbreviated very much? */
- if (starts_with("no-", arg)) {
- register_abbrev(p, options, OPT_UNSET ^ opt_flags,
- &abbrev, &ambiguous);
- }
- continue;
+ return get_value(p, options, flags ^ opt_flags);
}
- if (*rest) {
- if (*rest != '=')
- continue;
- p->opt = rest + 1;
- }
- return get_value(p, options, flags ^ opt_flags);
+
+ /* abbreviated? */
+ if (!strncmp(long_name, arg_start, arg_end - arg_start))
+ register_abbrev(p, options, flags ^ opt_flags,
+ &abbrev, &ambiguous);
+
+ /* negated and abbreviated very much? */
+ if (allow_unset && starts_with("no-", arg))
+ register_abbrev(p, options, OPT_UNSET ^ opt_flags,
+ &abbrev, &ambiguous);
}
if (disallow_abbreviated_options && (ambiguous.option || abbrev.option))
--
2.44.0
^ permalink raw reply related
* [PATCH 5/6] parse-options: normalize arg and long_name before comparison
From: René Scharfe @ 2024-02-24 21:29 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Strip "no-" from arg and long_name before comparing them. This way we
no longer have to repeat the comparison with an offset of 3 for negated
arguments.
Note that we must not modify the "flags" value, which tracks whether arg
is negated, inside the loop. When registering "--n", "--no" or "--no-"
as abbreviation for any negative option, we used to OR it with OPT_UNSET
and end the loop. We can simply hard-code OPT_UNSET and leave flags
unchanged instead.
---
parse-options.c | 44 ++++++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 008c0f32cf..d45efa4e5c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -382,28 +382,42 @@ static enum parse_opt_result parse_long_opt(
const struct option *options)
{
const char *arg_end = strchrnul(arg, '=');
+ const char *arg_start = arg;
+ enum opt_parsed flags = OPT_LONG;
+ int arg_starts_with_no_no = 0;
struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };
struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };
+ if (skip_prefix(arg_start, "no-", &arg_start)) {
+ if (skip_prefix(arg_start, "no-", &arg_start))
+ arg_starts_with_no_no = 1;
+ else
+ flags |= OPT_UNSET;
+ }
+
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
- enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;
+ enum opt_parsed opt_flags = OPT_LONG;
+ int allow_unset = !(options->flags & PARSE_OPT_NONEG);
if (options->type == OPTION_SUBCOMMAND)
continue;
if (!long_name)
continue;
- if (!starts_with(arg, "no-") &&
- !(options->flags & PARSE_OPT_NONEG) &&
- skip_prefix(long_name, "no-", &long_name))
+ if (skip_prefix(long_name, "no-", &long_name))
opt_flags |= OPT_UNSET;
+ else if (arg_starts_with_no_no)
+ continue;
+
+ if (((flags ^ opt_flags) & OPT_UNSET) && !allow_unset)
+ continue;
- if (!skip_prefix(arg, long_name, &rest))
+ if (!skip_prefix(arg_start, long_name, &rest))
rest = NULL;
if (!rest) {
/* abbreviated? */
- if (!strncmp(long_name, arg, arg_end - arg)) {
+ if (!strncmp(long_name, arg_start, arg_end - arg_start)) {
register_abbrev(p, options, flags ^ opt_flags,
&abbrev, &ambiguous);
}
@@ -412,24 +426,10 @@ static enum parse_opt_result parse_long_opt(
continue;
/* negated and abbreviated very much? */
if (starts_with("no-", arg)) {
- flags |= OPT_UNSET;
- register_abbrev(p, options, flags ^ opt_flags,
+ register_abbrev(p, options, OPT_UNSET ^ opt_flags,
&abbrev, &ambiguous);
- continue;
- }
- /* negated? */
- if (!starts_with(arg, "no-"))
- continue;
- flags |= OPT_UNSET;
- if (!skip_prefix(arg + 3, long_name, &rest)) {
- /* abbreviated and negated? */
- if (!strncmp(long_name, arg + 3,
- arg_end - arg - 3))
- register_abbrev(p, options,
- flags ^ opt_flags,
- &abbrev, &ambiguous);
- continue;
}
+ continue;
}
if (*rest) {
if (*rest != '=')
--
2.44.0
^ permalink raw reply related
* [PATCH 4/6] parse-options: detect ambiguous self-negation
From: René Scharfe @ 2024-02-24 21:29 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Git currently does not detect the ambiguity of an option that starts
with "no" like --notes and its negated form if given just --n or --no.
All Git commands with such options have other negatable options, and
we detect the ambiguity with them, so that's currently only a potential
problem for scripts that use git rev-parse --parseopt.
Let's fix it nevertheless, as there's no need for that confusion. To
detect the ambiguity we have to loosen the check in register_abbrev(),
as an option is considered an alias of itself. Add non-matching
negation flags as a criterion to recognize an option being ambiguous
with its negated form.
And we need to keep going after finding a non-negated option as an
abbreviated candidate and perform the negation checks in the same
loop.
---
parse-options.c | 3 +--
t/t1502-rev-parse-parseopt.sh | 11 +++++++++++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 398ebaef14..008c0f32cf 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -363,7 +363,7 @@ static void register_abbrev(struct parse_opt_ctx_t *p,
if (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)
return;
if (abbrev->option &&
- !is_alias(p, abbrev->option, option)) {
+ !(abbrev->flags == flags && is_alias(p, abbrev->option, option))) {
/*
* If this is abbreviated, it is
* ambiguous. So when there is no
@@ -406,7 +406,6 @@ static enum parse_opt_result parse_long_opt(
if (!strncmp(long_name, arg, arg_end - arg)) {
register_abbrev(p, options, flags ^ opt_flags,
&abbrev, &ambiguous);
- continue;
}
/* negation allowed? */
if (options->flags & PARSE_OPT_NONEG)
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index f0737593c3..b754b9fd74 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -322,4 +322,15 @@ check_invalid_long_option optionspec-neg --no-positive-only
check_invalid_long_option optionspec-neg --negative
check_invalid_long_option optionspec-neg --no-no-negative
+test_expect_success 'ambiguous: --no matches both --noble and --no-noble' '
+ cat >spec <<-\EOF &&
+ some-command [options]
+ --
+ noble The feudal switch.
+ EOF
+ test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+ git rev-parse --parseopt -- <spec 2>err --no &&
+ grep "error: ambiguous option: no (could be --noble or --no-noble)" err
+'
+
test_done
--
2.44.0
^ permalink raw reply related
* Re: [PATCH 0/6] parse-options: long option parsing fixes and cleanup
From: René Scharfe @ 2024-02-24 21:42 UTC (permalink / raw)
To: git
In-Reply-To: <20240224212953.44026-1-l.s.r@web.de>
Am 24.02.24 um 22:29 schrieb René Scharfe:
> Fix two corner cases in patches 1 and 4, refactor the code in patches 2
> and 3 to prepare the second fix, bonus patches 5 and 6 simplify the code
> and improve readability.
Forgot to sign-off, d'oh! Will wait for feedback and sign-off in v2..
>
> parse-options: recognize abbreviated negated option with arg
> parse-options: set arg of abbreviated option lazily
> parse-options: factor out register_abbrev() and struct parsed_option
> parse-options: detect ambiguous self-negation
> parse-options: normalize arg and long_name before comparison
> parse-options: rearrange long_name matching code
>
> parse-options.c | 137 ++++++++++++++++++----------------
> t/t0040-parse-options.sh | 16 ++++
> t/t1502-rev-parse-parseopt.sh | 11 +++
> 3 files changed, 100 insertions(+), 64 deletions(-)
>
> --
> 2.44.0
>
^ permalink raw reply
* [PATCH] fetch: convert strncmp() with strlen() to starts_with()
From: René Scharfe @ 2024-02-24 21:47 UTC (permalink / raw)
To: Git List
Using strncmp() and strlen() to check whether a string starts with
another one requires repeating the prefix candidate. Use starts_with()
instead, which reduces repetition and is more readable.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/fetch.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3aedfd1bb6..0a7a1a3476 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -448,9 +448,8 @@ static void filter_prefetch_refspec(struct refspec *rs)
continue;
if (!rs->items[i].dst ||
(rs->items[i].src &&
- !strncmp(rs->items[i].src,
- ref_namespace[NAMESPACE_TAGS].ref,
- strlen(ref_namespace[NAMESPACE_TAGS].ref)))) {
+ starts_with(rs->items[i].src,
+ ref_namespace[NAMESPACE_TAGS].ref))) {
int j;
free(rs->items[i].src);
--
2.44.0
^ permalink raw reply related
* Re: [PATCH] compat: drop inclusion of <git-compat-util.h>
From: Kyle Lippincott @ 2024-02-24 22:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwmqtli18.fsf@gitster.g>
On Sat, Feb 24, 2024 at 12:33 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> These two header files are included from ordinary source files that
> already include <git-compat-util.h> as the first header file as they
> should. There is no need to include the compat-util in these
> headers.
>
> "make hdr-check" is not affected, as it is designed to assume that
> what <git-compat-util.h> offers is available to everybody without
> being included.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * There is an obvious alternative that goes in the complete
> opposite direction possible, to update "make hdr-check" to ensure
> that things that are depended upon in each header file
> (e.g. pager.h refers to uintmax_t) are brought in by the header
> file to include the compat-util in it, i.e.
>
> diff --git c/Makefile w/Makefile
> index 78e874099d..d7b360f15e 100644
> --- c/Makefile
> +++ w/Makefile
> @@ -3259,7 +3259,7 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
> HCC = $(HCO:hco=hcc)
>
> %.hcc: %.h
> - @echo '#include "git-compat-util.h"' >$@
> + @echo '/* #include "git-compat-util.h" */' >$@
> @echo '#include "$<"' >>$@
>
> $(HCO): %.hco: %.hcc FORCE
>
> which would require in a noisy diff to add inclusion of
> git-compat-util.h to many header files. For purposes of folks
> who may want to carve out only pieces of our source tree, such an
> approach might work better, but for that to happen and yield any
> useful result, I suspect that compat-util header needs to be
> split into "compatibility essentials" and other "it is convenient
> if these are available everywhere, even though they do not have
> much to do with hiding system dependencies from the sources"
> parts first.
>
> compat/compiler.h | 1 -
> compat/disk.h | 1 -
> 2 files changed, 2 deletions(-)
LG, thanks. I agree with the direction from this patch: I think we
should not be including git-compat-util.h in header files, whether
they're "internal only" or part of the "external interface" of a
library. It does far too much and makes too many assumptions about
when it's being included. I plan on writing up some more thoughts on
this on a different thread, but it's taking some time to get those
into a shareable state.
>
> diff --git c/compat/compiler.h w/compat/compiler.h
> index 10dbb65937..e9ad9db84f 100644
> --- c/compat/compiler.h
> +++ w/compat/compiler.h
> @@ -1,7 +1,6 @@
> #ifndef COMPILER_H
> #define COMPILER_H
>
> -#include "git-compat-util.h"
> #include "strbuf.h"
>
> #ifdef __GLIBC__
> diff --git c/compat/disk.h w/compat/disk.h
> index 6c979c27d8..23bc1bef86 100644
> --- c/compat/disk.h
> +++ w/compat/disk.h
> @@ -1,7 +1,6 @@
> #ifndef COMPAT_DISK_H
> #define COMPAT_DISK_H
>
> -#include "git-compat-util.h"
> #include "abspath.h"
> #include "gettext.h"
>
>
^ permalink raw reply
* [PATCH v2] doc: clarify the wording on <git-compat-util.h> requirement
From: Junio C Hamano @ 2024-02-24 22:36 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott, Calvin Wan, Jonathan Tan, Elijah Newren
In-Reply-To: <xmqq4jdxmx2e.fsf@gitster.g>
The reason why we require the <git-compat-util.h> file to be the
first header file to be included is because it insulates other
header files and source files from platform differences, like which
system header files must be included in what order, and what C
preprocessor feature macros must be defined to trigger certain
features we want out of the system.
We tried to clarify the rule in the coding guidelines document, but
the wording was a bit fuzzy that can lead to misinterpretations like
you can include <xdiff/xinclude.h> only to avoid having to include
<git-compat-util.h> even if you have nothing to do with the xdiff
implementation, for example. "You do not have to include more than
one of these" was also misleading and would have been puzzling if
you _needed_ to depend on more than one of these approved headers
(answer: you are allowed to include them all if you need the
declarations in them for reasons other than that you want to avoid
including compat-util yourself).
Instead of using the phrase "approved headers", enumerate them as
exceptions, each labeled with its intended audiences, to avoid such
misinterpretations. The structure also makes it easier to add new
exceptions, so add the description of "t/unit-tests/test-lib.h"
being an exception only for the unit tests implementation as an
example.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The most notable change since the first one is that the reason
why the requirement exists is added to the document, not just
telling them what to do but also telling them why.
Also the path to "test-lib.h" used in the unit-test framework has
been spelled out relative to the root of the working tree, like
all other header files.
Range-diff:
1: dde3a79470 ! 1: b9f3d36e9a doc: clarify the wording on <git-compat-util.h> requirement
@@ Metadata
## Commit message ##
doc: clarify the wording on <git-compat-util.h> requirement
- The reason why we insist including the compat-util header as the
- very first thing is because it is our mechanism to absorb the
- differences across platforms, like the order in which system header
- files must be included, and C preprocessor feature macros that are
- needed to trigger certain features we want out of the systems, and
- insulate other headers and sources from such minutiae.
+ The reason why we require the <git-compat-util.h> file to be the
+ first header file to be included is because it insulates other
+ header files and source files from platform differences, like which
+ system header files must be included in what order, and what C
+ preprocessor feature macros must be defined to trigger certain
+ features we want out of the system.
- Earlier we tried to clarify the rule in the coding guidelines
- document, but the wording was a bit fuzzy that can lead to
- misinterpretations like you can include xdiff/xinclude.h only to
- avoid having to include git-compat-util.h file even if you have
- nothing to do with xdiff implementation, for example. "You do not
- have to include more than one of these" were also misleading and
- would have been puzzling if you _needed_ to depend on more than one
- of these approved headers (answer: you are allowed to include them
- all if you need the declarations in them for reasons other than that
- you want to avoid including compat-util yourself).
+ We tried to clarify the rule in the coding guidelines document, but
+ the wording was a bit fuzzy that can lead to misinterpretations like
+ you can include <xdiff/xinclude.h> only to avoid having to include
+ <git-compat-util.h> even if you have nothing to do with the xdiff
+ implementation, for example. "You do not have to include more than
+ one of these" was also misleading and would have been puzzling if
+ you _needed_ to depend on more than one of these approved headers
+ (answer: you are allowed to include them all if you need the
+ declarations in them for reasons other than that you want to avoid
+ including compat-util yourself).
Instead of using the phrase "approved headers", enumerate them as
- exceptions, each labeled with intended audiences, to avoid such
+ exceptions, each labeled with its intended audiences, to avoid such
misinterpretations. The structure also makes it easier to add new
exceptions, so add the description of "t/unit-tests/test-lib.h"
being an exception only for the unit tests implementation as an
@@ Documentation/CodingGuidelines: For C programs:
- "t/helper/test-tool.h", "xdiff/xinclude.h", or
- "reftable/system.h".) You do not have to include more than one of
- these.
-+ implementations and sha1dc/, must be "git-compat-util.h". In
-+ addition:
++ implementations and sha1dc/, must be "git-compat-util.h". This
++ header file insulates other header files and source files from
++ platform differences, like which system header files must be
++ included in what order, and what C preprocessor feature macros must
++ be defined to trigger certain features we expect out of the system.
++
++ In addition:
+
+ - the implementation of the built-in commands in the "builtin/"
+ directory that include "builtin.h" for the cmd_foo() prototype
@@ Documentation/CodingGuidelines: For C programs:
+ "xdiff/xinclude.h" for the xdiff machinery internals
+
+ - the unit test programs in "t/unit-tests/" directory that include
-+ "test-lib.h" that gives them the unit-tests framework
++ "t/unit-tests/test-lib.h" that gives them the unit-tests
++ framework
+
+ - the source files that implement reftable in the "reftable/"
+ directory that include "reftable/system.h" for the reftable
+ internals
+
-+ are allowed to assume that their header file includes
-+ "git-compat-util.h", and they do not have to include
-+ "git-compat-util.h" themselves. These headers must be the first
++ are allowed to assume that they do not have to include
++ "git-compat-util.h" themselves, as it is included as the first
++ '#include' in these header files. These headers must be the first
+ header file to be "#include"d in them, though.
- A C file must directly include the header files that declare the
Documentation/CodingGuidelines | 36 ++++++++++++++++++++++++++++------
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 578587a471..291b2898a2 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -446,12 +446,36 @@ For C programs:
detail.
- The first #include in C files, except in platform specific compat/
- implementations and sha1dc/, must be either "git-compat-util.h" or
- one of the approved headers that includes it first for you. (The
- approved headers currently include "builtin.h",
- "t/helper/test-tool.h", "xdiff/xinclude.h", or
- "reftable/system.h".) You do not have to include more than one of
- these.
+ implementations and sha1dc/, must be "git-compat-util.h". This
+ header file insulates other header files and source files from
+ platform differences, like which system header files must be
+ included in what order, and what C preprocessor feature macros must
+ be defined to trigger certain features we expect out of the system.
+
+ In addition:
+
+ - the implementation of the built-in commands in the "builtin/"
+ directory that include "builtin.h" for the cmd_foo() prototype
+ definition
+
+ - the test helper programs in the "t/helper/" directory that include
+ "t/helper/test-tool.h" for the cmd__foo() prototype definition
+
+ - the xdiff implementation in the "xdiff/" directory that includes
+ "xdiff/xinclude.h" for the xdiff machinery internals
+
+ - the unit test programs in "t/unit-tests/" directory that include
+ "t/unit-tests/test-lib.h" that gives them the unit-tests
+ framework
+
+ - the source files that implement reftable in the "reftable/"
+ directory that include "reftable/system.h" for the reftable
+ internals
+
+ are allowed to assume that they do not have to include
+ "git-compat-util.h" themselves, as it is included as the first
+ '#include' in these header files. These headers must be the first
+ header file to be "#include"d in them, though.
- A C file must directly include the header files that declare the
functions and the types it uses, except for the functions and types
--
2.44.0
^ permalink raw reply related
* Re: [PATCH] doc: clarify the wording on <git-compat-util.h> requirement
From: Kyle Lippincott @ 2024-02-24 22:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Calvin Wan, Jonathan Tan, Elijah Newren
In-Reply-To: <xmqq4jdxmx2e.fsf@gitster.g>
On Sat, Feb 24, 2024 at 12:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> The reason why we insist including the compat-util header as the
> very first thing is because it is our mechanism to absorb the
> differences across platforms, like the order in which system header
> files must be included, and C preprocessor feature macros that are
> needed to trigger certain features we want out of the systems, and
> insulate other headers and sources from such minutiae.
>
> Earlier we tried to clarify the rule in the coding guidelines
> document, but the wording was a bit fuzzy that can lead to
> misinterpretations like you can include xdiff/xinclude.h only to
> avoid having to include git-compat-util.h file even if you have
> nothing to do with xdiff implementation, for example. "You do not
> have to include more than one of these" were also misleading and
> would have been puzzling if you _needed_ to depend on more than one
> of these approved headers (answer: you are allowed to include them
> all if you need the declarations in them for reasons other than that
> you want to avoid including compat-util yourself).
>
> Instead of using the phrase "approved headers", enumerate them as
> exceptions, each labeled with intended audiences, to avoid such
> misinterpretations. The structure also makes it easier to add new
> exceptions, so add the description of "t/unit-tests/test-lib.h"
> being an exception only for the unit tests implementation as an
> example.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * git-std-lib folks CC'ed to show them where to put their exception
> when things start to stabilize; Elijah CC'ed for his 8bff5ca0
> (treewide: ensure one of the appropriate headers is sourced
> first, 2023-02-24) and bc5c5ec0 (cache.h: remove this
> no-longer-used header, 2023-05-16).
>
> Documentation/CodingGuidelines | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> index 578587a471..b3443dd773 100644
> --- c/Documentation/CodingGuidelines
> +++ w/Documentation/CodingGuidelines
> @@ -446,12 +446,30 @@ For C programs:
> detail.
>
> - The first #include in C files, except in platform specific compat/
> - implementations and sha1dc/, must be either "git-compat-util.h" or
> - one of the approved headers that includes it first for you. (The
> - approved headers currently include "builtin.h",
> - "t/helper/test-tool.h", "xdiff/xinclude.h", or
> - "reftable/system.h".) You do not have to include more than one of
> - these.
> + implementations and sha1dc/, must be "git-compat-util.h". In
> + addition:
This "In addition" ties to the "are allowed to" 19 lines below, which
was confusing for me until I intentionally ignored the "In addition",
continued reading, and finally caught the other piece of it. Perhaps
either `Exceptions:`, or something like `The following cases are
allowed to assume that their header file includes "git-compat-util.h",
and they do not have to include "git-compat-util.h" themselves:` -- I
have a slight preference for the latter form, but I worry that the
"These headers must be the first header file to be "#include"d in
them" bit will be missed. So maybe if we went with the latter version,
we change each bullet point to include that qualification. Example: -
the implementation of the built-in commands in the "builtin/"
directory that include "builtin.h" as the first header". I don't know
if we need the reasoning why you'd #include these files in the bullets
below, which is why I didn't include it here. I'm assuming there's a
concern about implying that builtin/foo.c should include builtin.h
instead of git-compat-util.h (even if foo.c doesn't use cmd_foo()?).
> +
> + - the implementation of the built-in commands in the "builtin/"
> + directory that include "builtin.h" for the cmd_foo() prototype
> + definition
> +
> + - the test helper programs in the "t/helper/" directory that include
> + "t/helper/test-tool.h" for the cmd__foo() prototype definition
> +
> + - the xdiff implementation in the "xdiff/" directory that includes
> + "xdiff/xinclude.h" for the xdiff machinery internals
> +
> + - the unit test programs in "t/unit-tests/" directory that include
> + "test-lib.h" that gives them the unit-tests framework
> +
> + - the source files that implement reftable in the "reftable/"
> + directory that include "reftable/system.h" for the reftable
> + internals
> +
> + are allowed to assume that their header file includes
> + "git-compat-util.h", and they do not have to include
> + "git-compat-util.h" themselves. These headers must be the first
> + header file to be "#include"d in them, though.
>
> - A C file must directly include the header files that declare the
> functions and the types it uses, except for the functions and types
^ permalink raw reply
* Re: [PATCH] doc: clarify the wording on <git-compat-util.h> requirement
From: Junio C Hamano @ 2024-02-24 22:54 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git, Calvin Wan, Jonathan Tan, Elijah Newren
In-Reply-To: <CAO_smVg4E4bENU18tiv8xnscnk46i=GW=Kq+xKvO1Nf7qCGy_A@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
> This "In addition" ties to the "are allowed to" 19 lines below, which
> was confusing for me until I intentionally ignored the "In addition",
> continued reading, and finally caught the other piece of it. Perhaps
> either `Exceptions:`, or something like `The following cases are
> allowed to assume that their header file includes "git-compat-util.h",
> and they do not have to include "git-compat-util.h" themselves:` -- I
> have a slight preference for the latter form, but I worry that the
> "These headers must be the first header file to be "#include"d in
> them" bit will be missed.
I'd appreciate people to help figuring out what the preamble should
read like to make it easier to follow.
> ... I don't know
> if we need the reasoning why you'd #include these files in the bullets
> below, which is why I didn't include it here. I'm assuming there's a
> concern about implying that builtin/foo.c should include builtin.h
> instead of git-compat-util.h (even if foo.c doesn't use cmd_foo()?).
It is more about helping folks new to the codebase understand the
reasoning behind the convention. As whoever implements "git foo" as
a built-in command is supposed to
- declare cmd_foo() in builtin.h
- add builtin/foo.c, define cmd_foo() there, and include builtin.h
- add "foo" and "cmd_foo" to git.c:commands[].
it is natural to expect any and all builtin/foo.c to include
builtin.h (hence it makes it convenient to allow an exception by
including compat-util in builtin.h to give everybody in builtin/
indirect access to compat-util).
But those who are not yet familar with the structure of the system
may not understand why it is natural. So, addition of "why is this
header allowed to be a substitute for which source files? Because
these source files are supposed to be including that header anyway"
is an important part of this patch.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/6] parse-options: factor out register_abbrev() and struct parsed_option
From: Junio C Hamano @ 2024-02-24 23:13 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20240224212953.44026-4-l.s.r@web.de>
René Scharfe <l.s.r@web.de> writes:
> static enum parse_opt_result parse_long_opt(
> struct parse_opt_ctx_t *p, const char *arg,
> const struct option *options)
> {
> const char *arg_end = strchrnul(arg, '=');
> - const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
> - enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
> - int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
> + struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };
> + struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };
There is this "allow_abbrev" thing ...
> ...
> if (!skip_prefix(arg + 3, long_name, &rest)) {
> /* abbreviated and negated? */
> - if (allow_abbrev &&
> - !strncmp(long_name, arg + 3,
> + if (!strncmp(long_name, arg + 3,
> arg_end - arg - 3))
> - goto is_abbreviated;
> - else
> - continue;
> + register_abbrev(p, options,
> + flags ^ opt_flags,
> + &abbrev, &ambiguous);
> + continue;
> }
> }
... whose use goes away completely from the loop. We still call
register_abbrev(), but the helper function becomes no-op when
p->flags had KEEP_UNKNOWN_OPT bit set, so everything cancels out.
OK.
^ 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