* Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
From: Junio C Hamano @ 2024-02-26 22:58 UTC (permalink / raw)
To: Christian Couder; +Cc: Eugenio Gigante, git, sunshine
In-Reply-To: <CAP8UFD3qR8E0gvUQtzzkLPWv4Db45kFS4pEqHKQr5siciVJ-zQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante
> <giganteeugenio2@gmail.com> wrote:
>>
>> 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.
>
> It's not clear from the patch that refresh() passes 'flags' as an
> argument to refresh_index(), so it might help reviewers a bit if you
> could tell that.
Perhaps.
>> 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.
>
> We prefer to use "let's change this and that" or just "change this and
> that" rather than "this commit changes this and that", see
> https://git-scm.com/docs/SubmittingPatches/#imperative-mood.
Very true.
> It might help if you could add a bit more explanation about why it's a
> good thing to use an unsigned variable instead of a signed one. For
> example you could say that it documents that we are not doing anything
> funny with the MSB.
But doesn't the proposed log message already say so?
In any case, it would very much help to fold long lines and have a
blank line in between paragraphs to make the log message more
readable.
Thanks, both.
>
>> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
>> ---
>> builtin/add.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> The patch looks correct, thanks!
^ permalink raw reply
* RE: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.
From: rsbecker @ 2024-02-26 23:02 UTC (permalink / raw)
To: 'Randall S. Becker', git
In-Reply-To: <20240226220539.3494-3-randall.becker@nexbridge.ca>
On Monday, February 26, 2024 5:06 PM, I wrote:
>From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
>This change is required because some platforms do not support file writes
of arbitrary sizes (e.g, NonStop). xwrite ends up truncating
>the output to the maximum single I/O size possible for the destination
device.
>
>Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>---
> builtin/receive-pack.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index
db65607485..5064f3d300 100644
>--- a/builtin/receive-pack.c
>+++ b/builtin/receive-pack.c
>@@ -455,8 +455,9 @@ static void report_message(const char *prefix, const
char *err, va_list params)
>
> if (use_sideband)
> send_sideband(1, 2, msg, sz, use_sideband);
>- else
>- xwrite(2, msg, sz);
>+ else {
>+ write_in_full(2, msg, sz);
>+ }
> }
>
> __attribute__((format (printf, 1, 2)))
>--
>2.42.1
This needs to be fixed, so the {} after the else is removed. Will be in v2.
^ permalink raw reply
* [PATCH v3] doc: clarify the wording on <git-compat-util.h> requirement
From: Junio C Hamano @ 2024-02-26 23:28 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott, Calvin Wan, Jonathan Tan, Elijah Newren
In-Reply-To: <xmqqh6hxlcae.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>
---
* Updated the leading phrase introducing the list of exceptions.
I think this is now clear enough to be ready for 'next'?
Range-diff:
1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on <git-compat-util.h> requirement
@@ 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". This
++ 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.
++ A collorary to this is that C files should not directly include
++ system header files themselves.
+
-+ In addition:
++ There are some exceptions, because certain group of files that
++ implement an API all have to include the same header file that
++ defines the API and it is convenient to include <git-compat-util.h>
++ there. Namely:
+
+ - the implementation of the built-in commands in the "builtin/"
+ directory that include "builtin.h" for the cmd_foo() prototype
-+ definition
++ definition,
+
+ - the test helper programs in the "t/helper/" directory that include
-+ "t/helper/test-tool.h" for the cmd__foo() prototype definition
++ "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
++ "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
++ framework, and
+
+ - the source files that implement reftable in the "reftable/"
+ directory that include "reftable/system.h" for the reftable
-+ internals
++ internals,
+
+ are allowed to assume that they do not have to include
-+ "git-compat-util.h" themselves, as it is included as the first
++ <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.
Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++-----
1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 578587a471..806979f75b 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -446,12 +446,41 @@ 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.
+ A collorary to this is that C files should not directly include
+ system header files themselves.
+
+ There are some exceptions, because certain group of files that
+ implement an API all have to include the same header file that
+ defines the API and it is convenient to include <git-compat-util.h>
+ there. Namely:
+
+ - 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, and
+
+ - 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 v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.
From: rsbecker @ 2024-02-26 23:30 UTC (permalink / raw)
To: 'Taylor Blau', 'Randall S. Becker'; +Cc: git
In-Reply-To: <Zd0S7aUIG1bhGkaX@nand.local>
On Monday, February 26, 2024 5:39 PM, Taylor Blau wrote:
>On Mon, Feb 26, 2024 at 05:05:35PM -0500, Randall S. Becker wrote:
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> This change is required because some platforms do not support file
>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating
>> the output to the maximum single I/O size possible for the destination device.
>
>Hmm. I'm not sure I understand what NonStop's behavior is here...
>
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c index
>> a3a37bd215..f80b8d101a 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1571,7 +1571,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>> * the last part of the input buffer to stdout.
>> */
>> while (input_len) {
>> - err = xwrite(1, input_buffer + input_offset, input_len);
>> + err = write_in_full(1, input_buffer + input_offset, input_len);
>> if (err <= 0)
>> break;
>> input_len -= err;
>> --
>> 2.42.1
>
>The code above loops while input_len is non-zero, and correctly decrements it by the number of bytes written by xwrite() after each
>iteration.
>
>Assuming that xwrite()/write(2) works how I think it does on NonStop, I'm not sure I understand why this change is necessary.
After thinking about it, I'm going to revert the change in this file, so it will not be in v2. I'm a bit uncomfortable with having the write sizes in global, so will drop this bit.
^ permalink raw reply
* Re: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.
From: Junio C Hamano @ 2024-02-26 23:46 UTC (permalink / raw)
To: rsbecker; +Cc: 'Taylor Blau', 'Randall S. Becker', git
In-Reply-To: <026b01da6906$4d96f530$e8c4df90$@nexbridge.com>
<rsbecker@nexbridge.com> writes:
>>The code above loops while input_len is non-zero, and correctly
>>decrements it by the number of bytes written by xwrite() after
>>each iteration.
>>
>>Assuming that xwrite()/write(2) works how I think it does on
>>NonStop, I'm not sure I understand why this change is necessary.
>
> NonStop has a limited SSIZE_MAX. xwrite only handles that much so
> anything beyond that gets dropped (not in the above code but in
> other builtins)
xwrite() caps a single write attempt to MAX_IO_SIZE and can return a
short-write, so anything beyound MAX_IO_SIZE will not even be sent
to the underlying write(2). There is a heuristic based on the value
of SSIZE_MAX to define MAX_IO_SIZE in <git-compat-util.h>, and if
the value given by that heuristics is too large for your platform,
you can tweak your own MAX_IO_SIZE (see the comments in that header
file).
The caller of xwrite() must be prepared to see a write return with
value less than the length it used to call the function, either
because of this MAX_IO_SIZE cut-off, or because of the underlying
write(2) returning after a short write. As long as the caller is
prepared, like Taylor pointed out, I am not sure why you'd need to
change it.
^ permalink raw reply
* Re: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.
From: Junio C Hamano @ 2024-02-26 23:50 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240226220539.3494-3-randall.becker@nexbridge.ca>
"Randall S. Becker" <the.n.e.key@gmail.com> writes:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> This change is required because some platforms do not support file writes of
> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
> maximum single I/O size possible for the destination device.
As msg[] here is 4k on-stack buffer, if the I/O size is small
enough, the above may happen, and I think write-in-full is warranted
here. If your I/O must be done in 1k chunks, it would be very slow
to run things like writing a pack stream to clone any non-toy
projects, though X-<.
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> builtin/receive-pack.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index db65607485..5064f3d300 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -455,8 +455,9 @@ static void report_message(const char *prefix, const char *err, va_list params)
>
> if (use_sideband)
> send_sideband(1, 2, msg, sz, use_sideband);
> - else
> - xwrite(2, msg, sz);
> + else {
> + write_in_full(2, msg, sz);
> + }
> }
>
> __attribute__((format (printf, 1, 2)))
^ permalink raw reply
* Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.
From: Junio C Hamano @ 2024-02-26 23:54 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240226220539.3494-4-randall.becker@nexbridge.ca>
"Randall S. Becker" <the.n.e.key@gmail.com> writes:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> This change is required because some platforms do not support file writes of
> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
> maximum single I/O size possible for the destination device. The result of
> write_in_full() is also passed to the caller, which was previously ignored.
This one smells more like a theoretical issue than realistic, in
that these writes are done only with .hexsz (either 40 or 64) bytes
oid string, or a single byte "\n", for either of which it is hard to
imagine that it is even remotely close to platform "maximum single
I/O size".
But we'd need to look for the error return anyway, so switching to
write_in_full() while we are doing so is also good.
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> builtin/repack.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ede36328a3..932d24c60b 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,
> struct packed_git *pack UNUSED,
> uint32_t pos UNUSED, void *data)
> {
> + int err;
> struct child_process *cmd = data;
>
> if (cmd->in == -1) {
> @@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid,
> die(_("could not start pack-objects to repack promisor objects"));
> }
>
> - xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> - xwrite(cmd->in, "\n", 1);
> + err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
> + if (err <= 0)
> + return err;
> + err = write_in_full(cmd->in, "\n", 1);
> + if (err <= 0)
> + return err;
> return 0;
> }
^ permalink raw reply
* Re: [PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.
From: Junio C Hamano @ 2024-02-26 23:56 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240226220539.3494-5-randall.becker@nexbridge.ca>
"Randall S. Becker" <the.n.e.key@gmail.com> writes:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> This change is required because some platforms do not support file writes of
> arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the
> maximum single I/O size possible for the destination device.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> builtin/unpack-objects.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
The same comment as [1/4]. Perhaps your MAX_IO_SIZE should be tuned
downwards, so that xwrite() works as it was designed to work.
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index e0a701f2b3..6935c4574e 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
>
> /* Write the last part of the buffer to stdout */
> while (len) {
> - int ret = xwrite(1, buffer + offset, len);
> + int ret = write_in_full(1, buffer + offset, len);
> if (ret <= 0)
> break;
> len -= ret;
^ permalink raw reply
* Re: [PATCH v3] doc: clarify the wording on <git-compat-util.h> requirement
From: Kyle Lippincott @ 2024-02-26 23:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Calvin Wan, Jonathan Tan, Elijah Newren
In-Reply-To: <xmqqle76kdpr.fsf_-_@gitster.g>
On Mon, Feb 26, 2024 at 3:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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>
> ---
>
> * Updated the leading phrase introducing the list of exceptions.
> I think this is now clear enough to be ready for 'next'?
>
> Range-diff:
> 1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on <git-compat-util.h> requirement
> @@ 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". This
> ++ 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.
> ++ A collorary to this is that C files should not directly include
> ++ system header files themselves.
> +
> -+ In addition:
> ++ There are some exceptions, because certain group of files that
> ++ implement an API all have to include the same header file that
> ++ defines the API and it is convenient to include <git-compat-util.h>
> ++ there. Namely:
> +
> + - the implementation of the built-in commands in the "builtin/"
> + directory that include "builtin.h" for the cmd_foo() prototype
> -+ definition
> ++ definition,
> +
> + - the test helper programs in the "t/helper/" directory that include
> -+ "t/helper/test-tool.h" for the cmd__foo() prototype definition
> ++ "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
> ++ "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
> ++ framework, and
> +
> + - the source files that implement reftable in the "reftable/"
> + directory that include "reftable/system.h" for the reftable
> -+ internals
> ++ internals,
> +
> + are allowed to assume that they do not have to include
> -+ "git-compat-util.h" themselves, as it is included as the first
> ++ <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.
>
>
> Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++-----
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 578587a471..806979f75b 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -446,12 +446,41 @@ 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.
> + A collorary to this is that C files should not directly include
> + system header files themselves.
> +
> + There are some exceptions, because certain group of files that
> + implement an API all have to include the same header file that
> + defines the API and it is convenient to include <git-compat-util.h>
> + there. Namely:
> +
> + - 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, and
> +
> + - 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
>
Looks good
^ permalink raw reply
* RE: [PATCH v1 1/4] builtin/index-pack.c: change xwrite to write_in_full to allow large sizes.
From: rsbecker @ 2024-02-27 0:12 UTC (permalink / raw)
To: 'Junio C Hamano'
Cc: 'Taylor Blau', 'Randall S. Becker', git
In-Reply-To: <xmqqa5nmkcuz.fsf@gitster.g>
On Monday, February 26, 2024 6:47 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>>>The code above loops while input_len is non-zero, and correctly
>>>decrements it by the number of bytes written by xwrite() after each
>>>iteration.
>>>
>>>Assuming that xwrite()/write(2) works how I think it does on NonStop,
>>>I'm not sure I understand why this change is necessary.
>>
>> NonStop has a limited SSIZE_MAX. xwrite only handles that much so
>> anything beyond that gets dropped (not in the above code but in other
>> builtins)
>
>xwrite() caps a single write attempt to MAX_IO_SIZE and can return a
short-write, so anything beyound MAX_IO_SIZE will not even be
>sent to the underlying write(2). There is a heuristic based on the value
of SSIZE_MAX to define MAX_IO_SIZE in <git-compat-util.h>,
>and if the value given by that heuristics is too large for your platform,
you can tweak your own MAX_IO_SIZE (see the comments in
>that header file).
>
>The caller of xwrite() must be prepared to see a write return with value
less than the length it used to call the function, either because
>of this MAX_IO_SIZE cut-off, or because of the underlying
>write(2) returning after a short write. As long as the caller is prepared,
like Taylor pointed out, I am not sure why you'd need to change
>it.
I understand. I was involved in xwrite() a few years ago. The problem is
that users of xwrite() did not account for that and t7704.9 failed as a
result. These changes did fix the issue. I am not sure how to proceed based
on the above, however. Continue or recode the callers (which is part of what
this does)?
^ permalink raw reply
* RE: [PATCH v1 2/4] builtin/receive-pack.c: change xwrite to write_in_full to allow large sizes.
From: rsbecker @ 2024-02-27 0:15 UTC (permalink / raw)
To: 'Junio C Hamano', 'Randall S. Becker'; +Cc: git
In-Reply-To: <xmqq34tekcoo.fsf@gitster.g>
On Monday, February 26, 2024 6:51 PM, Junio C Hamano wrote:
>"Randall S. Becker" <the.n.e.key@gmail.com> writes:
>
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> This change is required because some platforms do not support file
>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating
>> the output to the maximum single I/O size possible for the destination
device.
>
>As msg[] here is 4k on-stack buffer, if the I/O size is small enough, the
above may happen, and I think write-in-full is warranted here. If
>your I/O must be done in 1k chunks, it would be very slow to run things
like writing a pack stream to clone any non-toy projects,
>though X-<.
On the x86 platform, we get a size large enough not to trigger the failure
in t7704. However, on ia64, the limit is 56Kb, which apparently does. I'm
hoping no one else has a 1Kb limit - although some TCP stacks might
experience it. Either way, truncating a package is bad. Fortunately the I/O
subsystem on NonStop is very fast (basically DMA) between process memory
space.
>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>> builtin/receive-pack.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index
>> db65607485..5064f3d300 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -455,8 +455,9 @@ static void report_message(const char *prefix,
>> const char *err, va_list params)
>>
>> if (use_sideband)
>> send_sideband(1, 2, msg, sz, use_sideband);
>> - else
>> - xwrite(2, msg, sz);
>> + else {
>> + write_in_full(2, msg, sz);
>> + }
>> }
>>
>> __attribute__((format (printf, 1, 2)))
^ permalink raw reply
* RE: [PATCH v1 4/4] builtin/unpack-objects.c: change xwrite to write_in_full to allow large sizes.
From: rsbecker @ 2024-02-27 0:18 UTC (permalink / raw)
To: 'Junio C Hamano', 'Randall S. Becker'; +Cc: git
In-Reply-To: <xmqqr0gyixuu.fsf@gitster.g>
On Monday, February 26, 2024 6:56 PM, Junio C Hamano wrote:
>"Randall S. Becker" <the.n.e.key@gmail.com> writes:
>
>> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>>
>> This change is required because some platforms do not support file
>> writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating
>> the output to the maximum single I/O size possible for the destination
device.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>> builtin/unpack-objects.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>The same comment as [1/4]. Perhaps your MAX_IO_SIZE should be tuned
downwards, so that xwrite() works as it was designed to
>work.
I am considering undoing this one, other than ensuring that the error code
is checked and returned. The MAX_IO_SIZE is sufficient. I think the actual
fail was in the original repack.c not this one.
>
>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index
>> e0a701f2b3..6935c4574e 100644
>> --- a/builtin/unpack-objects.c
>> +++ b/builtin/unpack-objects.c
>> @@ -680,7 +680,7 @@ int cmd_unpack_objects(int argc, const char
>> **argv, const char *prefix UNUSED)
>>
>> /* Write the last part of the buffer to stdout */
>> while (len) {
>> - int ret = xwrite(1, buffer + offset, len);
>> + int ret = write_in_full(1, buffer + offset, len);
>> if (ret <= 0)
>> break;
>> len -= ret;
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Junio C Hamano @ 2024-02-27 0:20 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <CAO_smVi76TbmHd5w2rpBEEYbaw46SNTrekFHE-ohDC6-=dk6DA@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
> The condition added 13 years ago was, IMHO, backwards from what it
> should have been. The intent is to have stdint.h included. We should
> include stdint.h. I suspect that 17 years is enough time for that
> platform to start conforming to what is now a 25 year old standard,
> and I don't know how we can verify that and have this stop being a
> haunted graveyard without just trying it and seeing if the build bots
> or maintainers identify it as a continuing issue.
The nightmare of Solaris might be luckily behind us, but the world
does not only run Linux and GNU libc, and it is not just <stdint.h>
vs <inttypes.h>. This is about general code hygiene.
> If it's still an
> issue (and only if), we should reintroduce a conditional, but invert
> it: if there's no stdint.h, THEN include inttypes.h.
But it would give the wrong order in general in the modern world,
where <inttypes.h> is supposed to include <stdint.h> and extends it.
We use inttypes.h by default because the C standard already talks
about it, and fall back to stdint.h when the platform lacks it. But
what I suspect is that nobody compiles with NO_INTTYPES_H and we
would unknowingly (but not "unintentionally") start using the
extended types that are only available in <inttypes.h> but not in
<stdint.h> sometime in the future. It might already have happened,
but I do not know. I haven't compiled with NO_INTTYPES_H for some
time (to experiment), and I haven't met a platform that actually
requires NO_INTTYPES_H defined to build. Once after such a change
is made without anybody being knowingly breaking some rare platform,
if nobody complains, we can just drop the support to allow us to
limit ourselves to <stdint.h>, but since we hear nobody complaining,
we should be OK with the current rule of including system header
files that is embodied in <git-compat-util.h> header file.
In any case, your sources should not include a standard library
header directly yourself, period. Instead let <git-compat-util.h>
take care of the details of how we need to obtain what we need out
of the system on various platforms.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Kyle Lippincott @ 2024-02-27 0:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <xmqqle76iwqw.fsf@gitster.g>
On Mon, Feb 26, 2024 at 4:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Lippincott <spectral@google.com> writes:
>
> > The condition added 13 years ago was, IMHO, backwards from what it
> > should have been. The intent is to have stdint.h included. We should
> > include stdint.h. I suspect that 17 years is enough time for that
> > platform to start conforming to what is now a 25 year old standard,
> > and I don't know how we can verify that and have this stop being a
> > haunted graveyard without just trying it and seeing if the build bots
> > or maintainers identify it as a continuing issue.
>
> The nightmare of Solaris might be luckily behind us, but the world
> does not only run Linux and GNU libc, and it is not just <stdint.h>
> vs <inttypes.h>. This is about general code hygiene.
>
> > If it's still an
> > issue (and only if), we should reintroduce a conditional, but invert
> > it: if there's no stdint.h, THEN include inttypes.h.
>
> But it would give the wrong order in general in the modern world,
> where <inttypes.h> is supposed to include <stdint.h> and extends it.
>
> We use inttypes.h by default because the C standard already talks
> about it, and fall back to stdint.h when the platform lacks it. But
> what I suspect is that nobody compiles with NO_INTTYPES_H and we
> would unknowingly (but not "unintentionally") start using the
> extended types that are only available in <inttypes.h> but not in
> <stdint.h> sometime in the future. It might already have happened,
It has. We use PRIuMAX, which is from inttypes.h. I think it's only
"accidentally" working if anyone uses NO_INTTYPES_H. I changed my
stance halfway through this investigation in my previous email, I
apologize for not going back and editing it to make it clear at the
beginning that I'd done so. My current stance is that
<git-compat-util.h> should be either (a) including only inttypes.h
(since it includes stdint.h), or (b) including both inttypes.h and
stdint.h (in either order), just to demonstrate that we can.
> but I do not know. I haven't compiled with NO_INTTYPES_H for some
> time (to experiment), and I haven't met a platform that actually
> requires NO_INTTYPES_H defined to build. Once after such a change
> is made without anybody being knowingly breaking some rare platform,
> if nobody complains, we can just drop the support to allow us to
> limit ourselves to <stdint.h>, but since we hear nobody complaining,
> we should be OK with the current rule of including system header
> files that is embodied in <git-compat-util.h> header file.
>
> In any case, your sources should not include a standard library
> header directly yourself, period. Instead let <git-compat-util.h>
> take care of the details of how we need to obtain what we need out
> of the system on various platforms.
I disagree with this statement. We _can't_ use a magic compatibility
header file in the library interfaces, for the reasons I outlined
further below in my previous message. For those headers, the ones that
might be included by code that's not under the Git project's control,
they need to be self-contained, minimal, and maximally compatible.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH v3 00/14] FSMonitor edge cases on case-insensitive file systems
From: Junio C Hamano @ 2024-02-27 1:40 UTC (permalink / raw)
To: Jeff Hostetler via GitGitGadget
Cc: git, Patrick Steinhardt, Jeff Hostetler,
Torsten Bögershausen, Jeff Hostetler
In-Reply-To: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Here is version 3. I think I have addressed the remaining comments.
>
> I cleaned up the test code to use the test_expect_failure at the beginning
> and squashed in the test_expect_success version of tests into the final
> commit in the series.
>
> I moved the invalidate_ce_fsm() commit earlier in the series, so that the
> final commit actually uses all of the up-to-this-point changes to fix the
> problem.
>
> I converted a few "should not happens" to BUG()s.
>
> Thanks to everyone for their time and attention reviewing this. Jeff
>
> Jeff Hostetler (14):
> name-hash: add index_dir_find()
> t7527: add case-insensitve test for FSMonitor
> fsmonitor: refactor refresh callback on directory events
> fsmonitor: clarify handling of directory events in callback helper
> fsmonitor: refactor refresh callback for non-directory events
> dir: create untracked_cache_invalidate_trimmed_path()
> fsmonitor: refactor untracked-cache invalidation
> fsmonitor: move untracked-cache invalidation into helper functions
> fsmonitor: return invalidated cache-entry count on directory event
> fsmonitor: remove custom loop from non-directory path handler
> fsmonitor: return invalided cache-entry count on non-directory event
> fsmonitor: trace the new invalidated cache-entry count
> fsmonitor: refactor bit invalidation in refresh callback
> fsmonitor: support case-insensitive events
>
> dir.c | 20 +++
> dir.h | 7 +
> fsmonitor.c | 312 +++++++++++++++++++++++++++++------
> name-hash.c | 9 +-
> name-hash.h | 7 +-
> t/t7527-builtin-fsmonitor.sh | 223 +++++++++++++++++++++++++
> 6 files changed, 522 insertions(+), 56 deletions(-)
Much nicer. Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Junio C Hamano @ 2024-02-27 2:45 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <CAO_smVjeNYFTgh4MZjRM9U1coY=UJxo-E6bD9OfdS1A1Xf6vcQ@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
>> In any case, your sources should not include a standard library
>> header directly yourself, period. Instead let <git-compat-util.h>
>> take care of the details of how we need to obtain what we need out
>> of the system on various platforms.
>
> I disagree with this statement. We _can't_ use a magic compatibility
> header file in the library interfaces, for the reasons I outlined
> further below in my previous message. For those headers, the ones that
> might be included by code that's not under the Git project's control,
> they need to be self-contained, minimal, and maximally compatible.
Note that I am not talking about your random outside program that
happens to link with gitstdlib.a; it would want to include a header
file <gitstdlib.h> that comes with the library.
Earlier I suggested that you may want to take a subset of
<git-compat-util.h>, because <git-compat-util.h> may have a lot more
than what is minimally necessary to allow our sources to be
insulated from details of platform dependence. You can think of
that subset as a good starting point to build the <gitstdlib.h>
header file to be given to the library customers.
But the sources that go to the library, as gitstdlib.a is supposed
to serve as a subset of gitlib.a to our internal codebase when
building the git binary, should still follow our header inclusion
rules.
Because we would want to make sure that the sources that are made
into gitstdlib.a, the sources to the rest of libgit.a, and the
sources to the rest of git, all agree on what system features we ask
from the system, feature macros that must be defined to certain
values before we include system library files (like _XOPEN_SOURCE
and _FILE_OFFSET_BITS) must be defined consistently across all of
these three pieces. One way to do so may be to ensure that the
definition of them would be migrated to <gitstdlib.h> when we
separate a subset out of <git-compat-util.h> to it (and of course,
we make <git-compat-util.h> to include <gitstdlib.h> so that it
would be still sufficient for our in-tree users to include the
<git-compat-util.h>)
<gitstdlib.h> may have to expose an API function that uses some
extended types only available by including system header files,
e.g. some function may return ssize_t as its value or take an off_t
value as its argument.
If our header should include system headers to make these types
available to our definitions is probably open to discussion. It is
harder to do so portably, unless your world is limited to POSIX.1
and ISO C, than making it the responsibility of library users.
But if the platform headers and libraries support feature macros
that allows you to tweak these sizes (e.g. the size of off_t may be
controlled by setting the _FILE_OFFSET_BITS to an appropriate
value), it may be irresponsible to leave that to the library users,
as they MUST make sure to define such feature macros exactly the
same way as we define for our code, which currently is done in
<git-compat-util.h>, before they include their system headers to
obtain off_t so that they can use <gitstdlib.h>.
So the rules for library clients (random outside programs that
happen to link with gitstdlib.a) may not be that they must include
<git-compat-util.h> as the first thing, but they probably still have
to include <gitstdlib.h> fairly early before including any of their
system headers, I would suspect, unless they are willing to accept
such responsibility fully to ensure they compile the same way as the
gitstdlib library, I would think.
^ permalink raw reply
* Re: [PATCH v3] doc: clarify the wording on <git-compat-util.h> requirement
From: Elijah Newren @ 2024-02-27 4:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kyle Lippincott, Calvin Wan, Jonathan Tan
In-Reply-To: <xmqqle76kdpr.fsf_-_@gitster.g>
On Mon, Feb 26, 2024 at 3:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
Makes sense.
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * Updated the leading phrase introducing the list of exceptions.
> I think this is now clear enough to be ready for 'next'?
>
> Range-diff:
> 1: 2e7082d2d2 ! 1: 470f33a078 doc: clarify the wording on <git-compat-util.h> requirement
> @@ 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". This
> ++ 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.
> ++ A collorary to this is that C files should not directly include
> ++ system header files themselves.
> +
> -+ In addition:
> ++ There are some exceptions, because certain group of files that
> ++ implement an API all have to include the same header file that
> ++ defines the API and it is convenient to include <git-compat-util.h>
> ++ there. Namely:
> +
> + - the implementation of the built-in commands in the "builtin/"
> + directory that include "builtin.h" for the cmd_foo() prototype
> -+ definition
> ++ definition,
> +
> + - the test helper programs in the "t/helper/" directory that include
> -+ "t/helper/test-tool.h" for the cmd__foo() prototype definition
> ++ "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
> ++ "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
> ++ framework, and
> +
> + - the source files that implement reftable in the "reftable/"
> + directory that include "reftable/system.h" for the reftable
> -+ internals
> ++ internals,
> +
> + are allowed to assume that they do not have to include
> -+ "git-compat-util.h" themselves, as it is included as the first
> ++ <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.
>
>
> Documentation/CodingGuidelines | 41 +++++++++++++++++++++++++++++-----
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 578587a471..806979f75b 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -446,12 +446,41 @@ 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.
> + A collorary to this is that C files should not directly include
> + system header files themselves.
> +
> + There are some exceptions, because certain group of files that
> + implement an API all have to include the same header file that
> + defines the API and it is convenient to include <git-compat-util.h>
> + there. Namely:
> +
> + - 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, and
> +
> + - 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
I like this latest version.
^ permalink raw reply
* Re: [PATCH v3] doc: clarify the wording on <git-compat-util.h> requirement
From: Junio C Hamano @ 2024-02-27 5:35 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, Kyle Lippincott, Calvin Wan, Jonathan Tan
In-Reply-To: <CABPp-BHbuvLSSr2186XqiHFQD=NUYess5zdwG4bqY6FzQfG8oA@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> I like this latest version.
Thanks.
^ permalink raw reply
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Junio C Hamano @ 2024-02-27 6:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Christian Couder
In-Reply-To: <20240217054048.GD539459@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> diff --git a/Documentation/git.txt b/Documentation/git.txt
>> index 2f1cb3ef4e..be2829003d 100644
>> --- a/Documentation/git.txt
>> +++ b/Documentation/git.txt
>> @@ -183,6 +183,8 @@ If you just want to run git as if it was started in `<path>` then use
>> Do not fetch missing objects from the promisor remote on
>> demand. Useful together with `git cat-file -e <object>` to
>> see if the object is locally available.
>> + This is equivalent to setting the `GIT_NO_LAZY_FETCH`
>> + environment variable to `1`.
>
> As with the other patch, I'd suggest adding an entry to the list of
> environment variables later in the manpage.
Thanks, done.
We'll have to make a separate patch to add GIT_NO_REPLACE_OBJECTS to
the list, by the way (#leftoverbits), which I used as a model to
find what needs to be updated.
>> --- a/environment.h
>> +++ b/environment.h
>> @@ -35,6 +35,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);
>> #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
>> #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
>> #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
>> +#define NO_LAZY_FETCH_ENVIRONMENT "GIT_NO_LAZY_FETCH"
>> #define GIT_REPLACE_REF_BASE_ENVIRONMENT "GIT_REPLACE_REF_BASE"
>> #define GITATTRIBUTES_FILE ".gitattributes"
>> #define INFOATTRIBUTES_FILE "info/attributes"
>
> A small nit, but maybe worth keeping the two replace-related variables
> next to each other, rather than sticking the new one in the middle?
OK.
>> diff --git a/git.c b/git.c
>> index 28e8bf7497..d11d4dc77b 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -189,6 +189,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>> *envchanged = 1;
>> } else if (!strcmp(cmd, "--no-lazy-fetch")) {
>> fetch_if_missing = 0;
>> + setenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 1);
>> + if (envchanged)
>> + *envchanged = 1;
>> } else if (!strcmp(cmd, "--no-replace-objects")) {
>> disable_replace_refs();
>> setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
>
> I _suspect_ this makes the fetch_if_missing call redundant, since we
> should be parsing these before doing any repo setup that would trigger
> the code that reads the environment variable.
True. Again, we'd need to clean-up the NO_REPLACE_OBJECTS codepath
as well---the disable_replace_refs() call should also be redundant,
which I mimicked to add the no-lazy-fetch codepath (#leftoverbits).
> This should probably also be xsetenv(), though as you can see in the
> context we are not very consistent about using it. :) But certainly if
> we failed to set it I would prefer to see an error rather than
> accidentally lazy-fetching.
I'd probably leave it outside the scope of this series; as nobody
uses xsetenv() in git.c currently, it would be a clean-up topic on
its own (#leftoverbits).
>> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
>> index 5b7bee888d..59629cea1f 100755
>> --- a/t/t0410-partial-clone.sh
>> +++ b/t/t0410-partial-clone.sh
>> @@ -665,6 +665,15 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '
>> git -C partial.git rev-list --objects --missing=print HEAD >out &&
>> grep "[?]$FILE_HASH" out &&
>>
>> + # The no-lazy-fetch mechanism prevents Git from fetching
>> + test_must_fail env GIT_NO_LAZY_FETCH=1 \
>> + git -C partial.git cat-file -e "$FILE_HASH" &&
>> + test_must_fail git --no-lazy-fetch -C partial.git cat-file -e "$FILE_HASH" &&
>> +
>> + # Sanity check that the file is still missing
>> + git -C partial.git rev-list --objects --missing=print HEAD >out &&
>> + grep "[?]$FILE_HASH" out &&
>
> OK, we exercise it by setting the variable directly. A more interesting
> one might be:
>
> git -c alias.foo='!git cat-file' --no-lazy-fetch ...
>
> which should fail without the patch.
True, again. I'll test all three variants (i.e. environment,
command line option to "git", forced subprocess turning command line
option to "git" into environment).
Thanks.
----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH v2 3/3] git: extend --no-lazy-fetch to work across subprocesses
Modeling after how the `--no-replace-objects` option is made usable
across subprocess spawning (e.g., cURL based remote helpers are
spawned as a separate process while running "git fetch"), allow the
`--no-lazy-fetch` option to be passed across process boundaries.
Do not model how the value of GIT_NO_REPLACE_OBJECTS environment
variable is ignored, though. Just use the usual git_env_bool() to
allow "export GIT_NO_LAZY_FETCH=0" and "unset GIT_NO_LAZY_FETCH"
to be equivalents.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff:
1: 00e202b679 ! 1: e0764f2e21 git: extend --no-lazy-fetch to work across subprocesses
@@ Documentation/git.txt: If you just want to run git as if it was started in `<pat
--literal-pathspecs::
Treat pathspecs literally (i.e. no globbing, no pathspec magic).
+@@ Documentation/git.txt: for full details.
+ Setting this Boolean environment variable to true will cause Git to treat all
+ pathspecs as case-insensitive.
+
++`GIT_NO_LAZY_FETCH`::
++ Setting this Boolean environment variable to true tells Git
++ not to lazily fetch missing objects from the promisor remote
++ on demand.
++
+ `GIT_REFLOG_ACTION`::
+ When a ref is updated, reflog entries are created to keep
+ track of the reason why the ref was updated (which is
## environment.c ##
@@ environment.c: const char * const local_repo_env[] = {
@@ environment.c: void setup_git_env(const char *git_dir)
## environment.h ##
@@ environment.h: const char *getenv_safe(struct strvec *argv, const char *name);
- #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
#define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
#define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
-+#define NO_LAZY_FETCH_ENVIRONMENT "GIT_NO_LAZY_FETCH"
#define GIT_REPLACE_REF_BASE_ENVIRONMENT "GIT_REPLACE_REF_BASE"
++#define NO_LAZY_FETCH_ENVIRONMENT "GIT_NO_LAZY_FETCH"
#define GITATTRIBUTES_FILE ".gitattributes"
#define INFOATTRIBUTES_FILE "info/attributes"
+ #define ATTRIBUTE_MACRO_PREFIX "[attr]"
## git.c ##
@@ git.c: static int handle_options(const char ***argv, int *argc, int *envchanged)
@@ t/t0410-partial-clone.sh: test_expect_success 'lazy-fetch when accessing object
+ # The no-lazy-fetch mechanism prevents Git from fetching
+ test_must_fail env GIT_NO_LAZY_FETCH=1 \
+ git -C partial.git cat-file -e "$FILE_HASH" &&
++
++ # The same with command line option to "git"
+ test_must_fail git --no-lazy-fetch -C partial.git cat-file -e "$FILE_HASH" &&
+
++ # The same, forcing a subprocess via an alias
++ test_must_fail git --no-lazy-fetch -C partial.git \
++ -c alias.foo="!git cat-file" foo -e "$FILE_HASH" &&
++
+ # Sanity check that the file is still missing
+ git -C partial.git rev-list --objects --missing=print HEAD >out &&
+ grep "[?]$FILE_HASH" out &&
Documentation/git.txt | 7 +++++++
environment.c | 4 ++++
environment.h | 1 +
git.c | 3 +++
t/t0410-partial-clone.sh | 15 +++++++++++++++
5 files changed, 30 insertions(+)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2f1cb3ef4e..6fbaa9dd2b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -183,6 +183,8 @@ If you just want to run git as if it was started in `<path>` then use
Do not fetch missing objects from the promisor remote on
demand. Useful together with `git cat-file -e <object>` to
see if the object is locally available.
+ This is equivalent to setting the `GIT_NO_LAZY_FETCH`
+ environment variable to `1`.
--literal-pathspecs::
Treat pathspecs literally (i.e. no globbing, no pathspec magic).
@@ -896,6 +898,11 @@ for full details.
Setting this Boolean environment variable to true will cause Git to treat all
pathspecs as case-insensitive.
+`GIT_NO_LAZY_FETCH`::
+ Setting this Boolean environment variable to true tells Git
+ not to lazily fetch missing objects from the promisor remote
+ on demand.
+
`GIT_REFLOG_ACTION`::
When a ref is updated, reflog entries are created to keep
track of the reason why the ref was updated (which is
diff --git a/environment.c b/environment.c
index 9e37bf58c0..afad78a3f8 100644
--- a/environment.c
+++ b/environment.c
@@ -136,6 +136,7 @@ const char * const local_repo_env[] = {
GRAFT_ENVIRONMENT,
INDEX_ENVIRONMENT,
NO_REPLACE_OBJECTS_ENVIRONMENT,
+ NO_LAZY_FETCH_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
@@ -207,6 +208,9 @@ void setup_git_env(const char *git_dir)
shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
if (shallow_file)
set_alternate_shallow_file(the_repository, shallow_file, 0);
+
+ if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
+ fetch_if_missing = 0;
}
int is_bare_repository(void)
diff --git a/environment.h b/environment.h
index e5351c9dd9..5cec19cecc 100644
--- a/environment.h
+++ b/environment.h
@@ -36,6 +36,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);
#define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
#define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
#define GIT_REPLACE_REF_BASE_ENVIRONMENT "GIT_REPLACE_REF_BASE"
+#define NO_LAZY_FETCH_ENVIRONMENT "GIT_NO_LAZY_FETCH"
#define GITATTRIBUTES_FILE ".gitattributes"
#define INFOATTRIBUTES_FILE "info/attributes"
#define ATTRIBUTE_MACRO_PREFIX "[attr]"
diff --git a/git.c b/git.c
index 28e8bf7497..d11d4dc77b 100644
--- a/git.c
+++ b/git.c
@@ -189,6 +189,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--no-lazy-fetch")) {
fetch_if_missing = 0;
+ setenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 1);
+ if (envchanged)
+ *envchanged = 1;
} else if (!strcmp(cmd, "--no-replace-objects")) {
disable_replace_refs();
setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 5b7bee888d..c282851af7 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -665,6 +665,21 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '
git -C partial.git rev-list --objects --missing=print HEAD >out &&
grep "[?]$FILE_HASH" out &&
+ # The no-lazy-fetch mechanism prevents Git from fetching
+ test_must_fail env GIT_NO_LAZY_FETCH=1 \
+ git -C partial.git cat-file -e "$FILE_HASH" &&
+
+ # The same with command line option to "git"
+ test_must_fail git --no-lazy-fetch -C partial.git cat-file -e "$FILE_HASH" &&
+
+ # The same, forcing a subprocess via an alias
+ test_must_fail git --no-lazy-fetch -C partial.git \
+ -c alias.foo="!git cat-file" foo -e "$FILE_HASH" &&
+
+ # Sanity check that the file is still missing
+ git -C partial.git rev-list --objects --missing=print HEAD >out &&
+ grep "[?]$FILE_HASH" out &&
+
git -C full cat-file -s "$FILE_HASH" >expect &&
test-tool partial-clone object-info partial.git "$FILE_HASH" >actual &&
test_cmp expect actual &&
--
2.44.0-35-ga2082dbdd3
^ permalink raw reply related
* [PATCH] rebase: fix typo in autosquash documentation
From: Richard Macklin via GitGitGadget @ 2024-02-27 6:32 UTC (permalink / raw)
To: git; +Cc: Richard Macklin, Richard Macklin
From: Richard Macklin <code@rmacklin.dev>
This is a minor follow-up to cb00f524df (rebase: rewrite
--(no-)autosquash documentation, 2023-11-14) to fix a typo introduced in
that commit.
Signed-off-by: Richard Macklin <code@rmacklin.dev>
---
rebase: fix typo in autosquash documentation
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1676%2Frmacklin%2Ffix-typo-in-rebase-documentation-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1676/rmacklin/fix-typo-in-rebase-documentation-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1676
Documentation/git-rebase.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 06206521fc3..e7e725044db 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -607,7 +607,7 @@ The recommended way to create commits with squash markers is by using the
linkgit:git-commit[1], which take the target commit as an argument and
automatically fill in the subject line of the new commit from that.
+
-Settting configuration variable `rebase.autoSquash` to true enables
+Setting configuration variable `rebase.autoSquash` to true enables
auto-squashing by default for interactive rebase. The `--no-autosquash`
option can be used to override that setting.
+
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
--
gitgitgadget
^ permalink raw reply related
* subscribe
From: delyan @ 2024-02-27 6:52 UTC (permalink / raw)
To: fstests+unsubscribe, fstests, git+subscribe, git+unsubscribe, git,
git-commits-head+subscribe, git-commits-head+unsubscribe,
git-commits-head, initramfs+subscribe, initramfs+unsubscribe,
initramfs, io-uring+subscribe, io-uring+unsubscribe, io-uring,
kernel-janitors+subscribe, kernel-janitors+unsubscribe,
kernel-janitors
subscribe
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.44.0
From: Patrick Steinhardt @ 2024-02-27 7:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Mike Hommey, git, git-packagers
In-Reply-To: <xmqqbk84jwfy.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]
On Sun, Feb 25, 2024 at 09:16:49AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > That's ultimately the reason why I don't want HEAD to look like a proper
> > ref. But doing the "refs/heads/.invalid" workaround shouldn't be too bad,
> > I guess.
>
> Isn't the reason why reftable backend initializes refs/heads to be a
> regular file exactly because we want to reject an attempt to create
> such a file on the filesystem, though?
Yeah, "refs/heads" being a file is also part of this mechanism. But that
wouldn't help a client that e.g. uses git-symbolic-ref(1) while assuming
the old "files" backend, because they would now get a plausible value of
whatever value we have in there if we were to initialize it e.g. with
"refs/heads/main".
That's why we have both mechanisms -- it's a bit like defense in depth.
One could go even further and make "HEAD" contain complete garbage,
only, so that anybody who was trying to read it would fail immediately.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] submodule: use strvec_pushf() for --submodule-prefix
From: Patrick Steinhardt @ 2024-02-27 7:31 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <8cd983fb-32b9-41c6-a9e7-a485b190488c@web.de>
[-- Attachment #1: Type: text/plain, Size: 3023 bytes --]
On Sun, Feb 25, 2024 at 07:24:39PM +0100, René Scharfe wrote:
> Add the option --submodule-prefix and its argument directly using
> strvec_pushf() instead of via a detour through a strbuf. This is
> shorter, easier to read and doesn't require any explicit cleanup
> afterwards.
This looks obviously good to me. Thanks for this nice simplification!
Patrick
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> submodule.c | 19 ++++---------------
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 213da79f66..40f13a3685 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1687,8 +1687,6 @@ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
> task = get_fetch_task_from_changed(spf, err);
>
> if (task) {
> - struct strbuf submodule_prefix = STRBUF_INIT;
> -
> child_process_init(cp);
> cp->dir = task->repo->gitdir;
> prepare_submodule_repo_env_in_gitdir(&cp->env);
> @@ -1698,15 +1696,11 @@ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
> strvec_pushv(&cp->args, task->git_args.v);
> strvec_pushv(&cp->args, spf->args.v);
> strvec_push(&cp->args, task->default_argv);
> - strvec_push(&cp->args, "--submodule-prefix");
> + strvec_pushf(&cp->args, "--submodule-prefix=%s%s/",
> + spf->prefix, task->sub->path);
>
> - strbuf_addf(&submodule_prefix, "%s%s/",
> - spf->prefix,
> - task->sub->path);
> - strvec_push(&cp->args, submodule_prefix.buf);
> *task_cb = task;
>
> - strbuf_release(&submodule_prefix);
> string_list_insert(&spf->seen_submodule_names, task->sub->name);
> return 1;
> }
> @@ -1714,12 +1708,8 @@ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
> if (spf->oid_fetch_tasks_nr) {
> struct fetch_task *task =
> spf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];
> - struct strbuf submodule_prefix = STRBUF_INIT;
> spf->oid_fetch_tasks_nr--;
>
> - strbuf_addf(&submodule_prefix, "%s%s/",
> - spf->prefix, task->sub->path);
> -
> child_process_init(cp);
> prepare_submodule_repo_env_in_gitdir(&cp->env);
> cp->git_cmd = 1;
> @@ -1728,8 +1718,8 @@ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
> strvec_init(&cp->args);
> strvec_pushv(&cp->args, spf->args.v);
> strvec_push(&cp->args, "on-demand");
> - strvec_push(&cp->args, "--submodule-prefix");
> - strvec_push(&cp->args, submodule_prefix.buf);
> + strvec_pushf(&cp->args, "--submodule-prefix=%s%s/",
> + spf->prefix, task->sub->path);
>
> /* NEEDSWORK: have get_default_remote from submodule--helper */
> strvec_push(&cp->args, "origin");
> @@ -1737,7 +1727,6 @@ static int get_next_submodule(struct child_process *cp, struct strbuf *err,
> append_oid_to_argv, &cp->args);
>
> *task_cb = task;
> - strbuf_release(&submodule_prefix);
> return 1;
> }
>
> --
> 2.44.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/5] for-each-ref: add '--include-root-refs' option
From: Patrick Steinhardt @ 2024-02-27 7:39 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster, phillip.wood123
In-Reply-To: <20240223100112.44127-1-karthik.188@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5944 bytes --]
On Fri, Feb 23, 2024 at 11:01:07AM +0100, Karthik Nayak wrote:
> This is the fifth version of my patch series to print root refs
> in git-for-each-ref(1).
>
> With the introduction of the reftable backend, it becomes ever
> so important to provide the necessary tooling for printing all refs
> associated with a worktree.
>
> While regular refs stored within the "refs/" namespace are currently
> supported by multiple commands like git-for-each-ref(1),
> git-show-ref(1). Neither support printing root refs within the worktree.
>
> This patch series is a follow up to the RFC/discussion we had earlier on
> the list [1].
>
> The first 4 commits add the required functionality to ensure we can print
> all refs (regular, pseudo, HEAD). The 5th commit modifies the
> git-for-each-ref(1) command to add the "--include-root-refs" command which
> will include HEAD and pseudorefs with regular "refs/" refs.
>
> [1]: https://lore.kernel.org/git/20231221170715.110565-1-karthik.188@gmail.com/#t
>
> Changes from v4:
> 1. Fixed erratic whitespace
> 2. Remove braces from single line block
> 3. Starting the comments with a capital and also adding more context.
> 4. Removed a duplicate check.
>
> Thanks for the reviews.
>
> Range diff against v4:
The range-diff looks as expected, so this patch series looks good to me.
Thanks!
Patrick
> 1: 98130a7ad7 ! 1: 6016042965 refs: introduce `is_pseudoref()` and `is_headref()`
> @@ refs.c: static int is_pseudoref_syntax(const char *refname)
> +
> + if (ends_with(refname, "_HEAD")) {
> + refs_resolve_ref_unsafe(refs, refname,
> -+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> -+ &oid, NULL);
> -+ return !is_null_oid(&oid);
> ++ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> ++ &oid, NULL);
> ++ return !is_null_oid(&oid);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
> + if (!strcmp(refname, irregular_pseudorefs[i])) {
> + refs_resolve_ref_unsafe(refs, refname,
> -+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> -+ &oid, NULL);
> ++ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> ++ &oid, NULL);
> + return !is_null_oid(&oid);
> + }
> +
> 2: 060ab08af5 = 2: acaa014841 refs: extract out `loose_fill_ref_dir_regular_file()`
> 3: 40d2375ad9 = 3: f51c5bc307 refs: introduce `refs_for_each_include_root_refs()`
> 4: b4b9435505 = 4: 7c004db6e7 ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
> 5: ee99ac41ae ! 5: 53f6c0a6db for-each-ref: add new option to include root refs
> @@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const
> filter.name_patterns = argv;
> }
>
> -+ if (include_root_refs) {
> ++ if (include_root_refs)
> + flags |= FILTER_REFS_ROOT_REFS;
> -+ }
> +
> filter.match_as_path = 1;
> - filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format);
> @@ ref-filter.c: static int for_each_fullref_in_pattern(struct ref_filter *filter,
> void *cb_data)
> {
> + if (filter->kind == FILTER_REFS_KIND_MASK) {
> -+ /* in this case, we want to print all refs including root refs. */
> ++ /* In this case, we want to print all refs including root refs. */
> + return refs_for_each_include_root_refs(get_main_ref_store(the_repository),
> + cb, cb_data);
> + }
> @@ ref-filter.c: static struct ref_array_item *apply_ref_filter(const char *refname
>
> /* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
> kind = filter_ref_kind(filter, refname);
> +- if (!(kind & filter->kind))
> +
> + /*
> -+ * When printing HEAD with all other refs, we want to apply the same formatting
> -+ * rules as the other refs, so we simply ask it to be treated as a pseudoref.
> ++ * Generally HEAD refs are printed with special description denoting a rebase,
> ++ * detached state and so forth. This is useful when only printing the HEAD ref
> ++ * But when it is being printed along with other pseudorefs, it makes sense to
> ++ * keep the formatting consistent. So we mask the type to act like a pseudoref.
> + */
> + if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD)
> + kind = FILTER_REFS_PSEUDOREFS;
> + else if (!(kind & filter->kind))
> -+ return NULL;
> -+
> - if (!(kind & filter->kind))
> return NULL;
>
> + if (!filter_pattern_match(filter, refname))
> @@ ref-filter.c: static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
> ret = for_each_fullref_in("refs/tags/", fn, cb_data);
> else if (filter->kind & FILTER_REFS_REGULAR)
>
>
> Karthik Nayak (5):
> refs: introduce `is_pseudoref()` and `is_headref()`
> refs: extract out `loose_fill_ref_dir_regular_file()`
> refs: introduce `refs_for_each_include_root_refs()`
> ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
> for-each-ref: add new option to include root refs
>
> Documentation/git-for-each-ref.txt | 5 +-
> builtin/for-each-ref.c | 10 ++-
> ref-filter.c | 30 ++++++-
> ref-filter.h | 7 +-
> refs.c | 48 +++++++++++
> refs.h | 9 ++
> refs/files-backend.c | 127 +++++++++++++++++++++--------
> refs/refs-internal.h | 6 ++
> refs/reftable-backend.c | 11 ++-
> t/t6302-for-each-ref-filter.sh | 31 +++++++
> 10 files changed, 237 insertions(+), 47 deletions(-)
>
> --
> 2.43.GIT
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] git: extend --no-lazy-fetch to work across subprocesses
From: Jeff King @ 2024-02-27 7:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqqfrxexx15.fsf@gitster.g>
On Mon, Feb 26, 2024 at 10:04:54PM -0800, Junio C Hamano wrote:
> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH v2 3/3] git: extend --no-lazy-fetch to work across subprocesses
> [...]
This looks pretty reasonable to me, and the lines you drew for
#leftoverbits all seemed liked good spots.
The only thing I noticed in the patch was this (which I think is not
even new in this round):
> diff --git a/environment.c b/environment.c
> index 9e37bf58c0..afad78a3f8 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -136,6 +136,7 @@ const char * const local_repo_env[] = {
> GRAFT_ENVIRONMENT,
> INDEX_ENVIRONMENT,
> NO_REPLACE_OBJECTS_ENVIRONMENT,
> + NO_LAZY_FETCH_ENVIRONMENT,
> GIT_REPLACE_REF_BASE_ENVIRONMENT,
> GIT_PREFIX_ENVIRONMENT,
> GIT_SHALLOW_FILE_ENVIRONMENT,
This will clear the environment variable when we move between
repositories. I can see an argument for it, and certainly that's how
GIT_NO_REPLACE_OBJECTS works.
But I can also see an argument that this is not a property of the
_repository_, but of the request. For example, if I run "git
--no-lazy-fetch log" and we cross into a submodule to do a diff, should
that submodule also avoid lazy-fetching? I'd think so, but I think your
patch would restore the defaults when we "enter" the submodule repo.
There's some prior art there, I think, in how GIT_CEILING_DIRECTORIES
works, or even something like "git --literal-pathspecs", neither of
which appear in local_repo_env.
-Peff
^ 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