* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Junio C Hamano @ 2024-02-27 23:25 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <CAO_smVgmaXvyZZ7zp6RCFD_6kpL2pHKC9gMDeg+yXBb9R4rR5w@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
> of <git-compat-util.h> that enforces that we're using C99. Therefore,
> I'm assuming that any compiler that claims to be C99 and passes that
> check at the top of <git-compat-util.h> will support inttypes.h,
> stdint.h, stdbool.h, and other files defined by the C99 standard to
> include types that we need in our .h files are able to be included
> without reservation.
We at the git project is much more conservative than trusting the
compilers and take their "claim" to support a standard at the face
value, though ;-). As our CodingGuidelines say, we honor real world
constraints more than what's written on paper as "standard", and
would ...
> To flip it around: any compiler/platform that's
> missing inttypes.h, or is missing stdint.h, or raises errors if both
> are included, or requires other headers to be included before them
> _isn't a C99 compiler_, and _isn't supported_.
... refrain from taking such a position as much as possible.
> I think the only viable solution to this is to not use these types
> that depend on #defines in the interface available to non-git
> projects.
OK. Now we have that behind us, can we start outlining what kind of
things _are_ exported out in the library to the outside world? The
only example of the C source file that is a client of the library we
have is t/helper/test-stdlib.c but it includes
<abspath.h>
<hex-ll.h>
<parse.h>
<strbuf.h>
<string-list.h>
after including <git-compat-util.h>. Are the services offered by
these five header files the initial set of the library, minimally
viable demonstration? Has somebody already analyzed and enumerated
what kind of system definitions we need to support the interface
these five header files offer?
Once we know that, perhaps the next task would be to create a
<git-absolute-minimum-portability-requirement.h> header by taking a
subset of <git-compat-util.h>, and have test-stdlib.c include that
instead of <git-compat-util.h>. <git-compat-util.h> will of course
include that header to replace the parts it lost to the new header.
It does *not* make it a requirement for client programs to include
the <git-absolute-minimum-portability-requirement.h>, though. They
can include the system headers in the way they like, as long as they
do not let them define symbols in ways contradicting to what our
headers expect. <git-absolute-minimum-portability-requirement.h> is
merely a way for us to tell those who write these client programs
what the system symbols we rely on are.
So, that's one (or two? first to analyse and enumerate, second to
split a new header file out of compat-util) actionable task, I
guess.
^ permalink raw reply
* Re: [PATCH] cmake: adapt to the Git Standard Library
From: Calvin Wan @ 2024-02-27 23:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kyle Lippincott, git, Johannes Schindelin
In-Reply-To: <xmqq8r35o657.fsf@gitster.g>
> > std-lib folks, can you fold this patch (with authorship credits
> > intact) into your series when it gets rerolled from here on?
Will do. Thanks Johannes for the fix!
^ permalink raw reply
* Re: [PATCH] cmake: adapt to the Git Standard Library
From: Junio C Hamano @ 2024-02-27 23:10 UTC (permalink / raw)
To: Kyle Lippincott, Calvin Wan; +Cc: git, Johannes Schindelin
In-Reply-To: <xmqqcyshsqcf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> In the commit "git-std-lib: introduce Git Standard Library" of the
>> `cw/git-std-lib` topic, the Makefile was restructured in a manner that
>> requires the CMake definition to follow suit to be able to build Git.
>>
>> This commit adjusts the CMake definition accordingly.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> cmake: adapt to the Git Standard Library
>>
>> The usual CMake adjustments. This is based on cw/git-std-lib.
>
> Thanks for a fix.
>
> std-lib folks, can you fold this patch (with authorship credits
> intact) into your series when it gets rerolled from here on?
Forgot to Cc: folks who can take care of this for me.
^ permalink raw reply
* Uses of xwrite() in the codebase
From: Junio C Hamano @ 2024-02-27 23:07 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
There aren't too many places that we make calls to xwrite(), so
let's audit all.
I'll show something that looks like patch, but I won't be applying
any of them myself, at least not as a part of this message.
First, some ground rules:
* Calling xwrite() relieves us from worrying about interrupted
system call returning with EINTR or EAGAIN without writing
anything. If we were using write(2), we need to be calling it
again with the same arguments, but xwrite() retries that call for
us. That is the _only_ thing it does.
* Specifically, when write(2) returns after writing less than what
it was asked to (called "short write"), xwrite() returns the
number of bytes written, just like the underlying write(2) does,
and it is not an error.
* Errors xwrite() internally handles are only the ones that are
related to an interrupted system call that needs retrying. Other
errors are responsibility of the caller to handle. If a platform
raises an error when passed a very large buffer to write out,
instead of just making a short write and returning the number of
writes written, the platform port should protect itself from such
an error by limiting MAX_IO_SIZE, so that xwrite() does not even
attempt to make such a large write(2) call.
Now, I'll go through "git grep -W -e 'xwrite('" output and annotate
it.
--------------------
builtin/index-pack.c:final() is prepared to see and handle a short
write(2) itself. It is a correct implementation that does not need
to be touched, but we could replace the loop with a single call to
write_in_full().
diff --git c/builtin/index-pack.c w/builtin/index-pack.c
index a3a37bd215..a54cb07a35 100644
--- c/builtin/index-pack.c
+++ w/builtin/index-pack.c
@@ -1570,13 +1570,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
* Let's just mimic git-unpack-objects here and write
* the last part of the input buffer to stdout.
*/
- while (input_len) {
- err = xwrite(1, input_buffer + input_offset, input_len);
- if (err <= 0)
- break;
- input_len -= err;
- input_offset += err;
- }
+ write_in_full(1, input_buffer + input_offset, input_len);
}
strbuf_release(&rev_index_name);
--------------------
builtin/receive-pack.c:report_message() has a call to xwrite() that
will lead to message truncation if the underlying write(2) returns
early.
diff --git c/builtin/receive-pack.c w/builtin/receive-pack.c
index 56d8a77ed7..4d8ed3f7a3 100644
--- c/builtin/receive-pack.c
+++ w/builtin/receive-pack.c
@@ -456,7 +456,7 @@ 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);
+ write_in_full(2, msg, sz);
}
__attribute__((format (printf, 1, 2)))
--------------------
builtin/repack.c:write_oid() feeds a line with an object name on it
to a subprocess, but it can suffer from a short write(2).
diff --git c/builtin/repack.c w/builtin/repack.c
index ede36328a3..1a516a6bed 100644
--- c/builtin/repack.c
+++ w/builtin/repack.c
@@ -314,8 +314,10 @@ 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);
+ if (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) ||
+ write_in_full(cmd->in, "\n", 1))
+ die(_("failed to feed promisor objects to pack-objects"));
+
return 0;
}
--------------------
builtin/unpack-objects.c:cmd_unpack_objects() has the same xwrite()
loop as we saw in builtin/index-pack.c:final(). It is a correct
implementation that does not need to be touched, but we could
replace the loop with a single call to write_in_full().
diff --git i/builtin/unpack-objects.c w/builtin/unpack-objects.c
index e0a701f2b3..f1c85a00ae 100644
--- i/builtin/unpack-objects.c
+++ w/builtin/unpack-objects.c
@@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
use(the_hash_algo->rawsz);
/* Write the last part of the buffer to stdout */
- while (len) {
- int ret = xwrite(1, buffer + offset, len);
- if (ret <= 0)
- break;
- len -= ret;
- offset += ret;
- }
+ write_in_full(1, buffer + offset, len);
/* All done */
return has_errors;
--------------------
http.c:fwrite_sha1_file() has xwrite() loop that is prepared to see
a short write(2). The loop could be replaced with write_in_full() but
the semantics of the value returned upon failure could become different,
so I'd rather not to touch it.
http.c- do {
http.c: ssize_t retval = xwrite(freq->localfile,
http.c- (char *) ptr + posn, size - posn);
http.c- if (retval < 0)
http.c- return posn / eltsize;
http.c- posn += retval;
http.c- } while (posn < size);
--------------------
sideband.c:demultiplex_sideband() uses xwrite() that does not retry
for its progress eye-candy. write_in_full() may be overkill for
them, I suspect.
sideband.c- strbuf_addch(scratch, *brk);
sideband.c: xwrite(2, scratch->buf, scratch->len);
sideband.c- strbuf_reset(scratch);
sideband.c ...
sideband.c- if (scratch->len) {
sideband.c- strbuf_addch(scratch, '\n');
sideband.c: xwrite(2, scratch->buf, scratch->len);
sideband.c- }
--------------------
streaming.c:stream_blob_to_fd() keeps track of the "hole" at the end
of the output file by refraining from writing series of NULs, and
instead uses lseek() to move the write pointer to 1-byte before the
desired end of file, and then uses xwrite() to write a single NUL at
the end. This should not result in a short write(2), and it handles
write error correctly already, but it would be OK to update it to
write_in_full() for consistency.
diff --git c/streaming.c w/streaming.c
index 10adf625b2..38925ea9fd 100644
--- c/streaming.c
+++ w/streaming.c
@@ -538,7 +538,7 @@ int stream_blob_to_fd(int fd, const struct object_id *oid, struct stream_filter
goto close_and_exit;
}
if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
- xwrite(fd, "", 1) != 1))
+ write_in_full(fd, "", 1) != 1))
goto close_and_exit;
result = 0;
--------------------
t/helper/test-genzeros.c:cmd__genzeros() runs xwrite() and dies if
it got an error. It does handle short write(2) correctly.
(1) If asked to write an infinite stream of NUL, it just feeds
zeros[] array without caring how many NUL bytes have actually been
written by a single xwrite() call. It just stops when it sees an
error.
t/helper/test-genzeros.c- /* Writing out individual NUL bytes is slow... */
t/helper/test-genzeros.c- while (count < 0)
t/helper/test-genzeros.c: if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
t/helper/test-genzeros.c- die_errno("write error");
(2) If asked to write a specific number of NUL bytes, it does a loop
that handles short write(2) correctly.
t/helper/test-genzeros.c- while (count > 0) {
t/helper/test-genzeros.c: n = xwrite(1, zeros,
t/helper/test-genzeros.c- count < ARRAY_SIZE(zeros)
t/helper/test-genzeros.c- ? count : ARRAY_SIZE(zeros));
t/helper/test-genzeros.c-
t/helper/test-genzeros.c- if (n < 0)
t/helper/test-genzeros.c- die_errno("write error");
t/helper/test-genzeros.c-
t/helper/test-genzeros.c- count -= n;
t/helper/test-genzeros.c- }
--------------------
transport-helper.c:disconnect_helper() has a call to xwrite() of a
single byte that does not care about errors, whose rationale is
given in a well written comment.
--------------------
transport-helper.c:udt_do_write() has a call to xwrite() that makes
a non-zero progress per a call to it, and its caller is prepared to
call it until the buffer is drained fully, prepared to handle a
short write(2) correctly. There is nothing to fix there.
--------------------
upload-pack.c:send_client_data() calls unchecked xwrite() on fd #2
to show an error message, but this happens only when the sideband is
not in use, so I do not know offhand how much this matters these
days. When the sideband is in use, the data is passed to
send_sideband() that does write_or_die(), so I guess for consistency
it might want to become write_in_full().
HOWEVER, there is a comment that reads "are we happy to lose stuff
here?" left by 93822c22 (short i/o: fix calls to write to use xwrite
or write_in_full, 2007-01-08), which was an earlier audit of this
exact issue that turned many unchecked xwrite() into write_in_full(),
so, I dunno.
upload-pack.c- if (use_sideband) {
upload-pack.c- send_sideband(1, fd, data, sz, use_sideband);
upload-pack.c- return;
upload-pack.c- }
upload-pack.c- if (fd == 3)
upload-pack.c- /* emergency quit */
upload-pack.c- fd = 2;
upload-pack.c- if (fd == 2) {
upload-pack.c- /* XXX: are we happy to lose stuff here? */
upload-pack.c: xwrite(fd, data, sz);
upload-pack.c- return;
upload-pack.c- }
upload-pack.c- write_or_die(fd, data, sz);
upload-pack.c-}
^ permalink raw reply related
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Kyle Lippincott @ 2024-02-27 22:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <xmqqzfvmhbfs.fsf@gitster.g>
On Mon, Feb 26, 2024 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
I agree with this.
>
> 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.
If I'm understanding this correctly, I agree with it. The .c files
still include <git-compat-util.h>, and don't change. The internal-only
.h files (ones that a pre-built-library consumer doesn't need to even
have in the filesystem) still assume that <git-compat-util.h> was
included, and don't change. <pager.h> falls into this category.
>
> 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.
I agree that these types will be necessary (specifically ssize_t and
int##_t, but less so off_t) in the "external" (used by projects other
than Git) library interfaces.
>
> 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.
I think I'm probably missing the nuance here, and may be making this
discussion much harder because of it. My understanding is that Git is
using C99; is that different from ISO C? There's something at the top
of <git-compat-util.h> that enforces that we're using C99. Therefore,
I'm assuming that any compiler that claims to be C99 and passes that
check at the top of <git-compat-util.h> will support inttypes.h,
stdint.h, stdbool.h, and other files defined by the C99 standard to
include types that we need in our .h files are able to be included
without reservation. To flip it around: any compiler/platform that's
missing inttypes.h, or is missing stdint.h, or raises errors if both
are included, or requires other headers to be included before them
_isn't a C99 compiler_, and _isn't supported_. I'm picking on these
files because I think they will be necessary for the external library
interfaces. I'm intentionally ignoring any file not mentioned in the
C99 standard, because those are platform specific. I acknowledge that
there may be some functionality in these files that's only enabled if
certain #defines are set. Our external interfaces should strive to not
use that functionality, and only do so if we are able to test for this
functionality and refuse to compile if it's not available. I have an
example with uintmax_t below.
>
> 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>.
I think the only viable solution to this is to not use these types
that depend on #defines in the interface available to non-git
projects. We can't set _FILE_OFFSET_BITS in the library's external
(used by non-Git projects) interface header, as there's a high
likelihood that it's either too late (external project #included
something that relies on _FILE_OFFSET_BITS already), or, if not, we
create the "off_t is a different size" problem for their code.
This means that we can't use off_t in these external interface headers
(and in the .c files that support them, if any). We can't use `struct
stat`. We likely need to limit ourselves to just the typedefs from
stdint.h, and probably will need some additional checks that enforce
that we have the types and sizes we expect (ex: I could imagine that
some platforms define uintmax_t as 32-bit. or 128-bit. Either we can't
use it in these external interfaces, or we have to enforce somehow
that the simplest file we can imagine (#include <stdint.h>) gets a
definition of uintmax_t that is the exact same as the one we'd get if
we included <git-compat-util.h>). The external interface headers don't
need to be as platform-compatible as the rest of the git code base,
because not every platform is going to be a supported target for using
the library in non-git projects, especially at first. The external
interface headers _do_ need to be as tolerant and well behaved as
possible when being included by external projects, which I'm asserting
means they need to be self-contained and minimal. If that means these
external interfaces don't get to use off_t at all, so be it. If it
means they can only be included if sizeof(off_t) == 64, and we have a
way of enforcing that at compile time, that's fine with me too. But we
can't #define _FILE_OFFSET_BITS ourselves in this external interface
to get that behavior, because it just doesn't work.
I'm making some assumptions here. I'm assuming that the git binary
uses a different interface to a hypothetical libgitobjstore.a than an
external project would (i.e. that there'd be some
git-obj-store-interface.h that gets included by non-Git projects, but
not by git itself). Is git-std-lib an obvious counterexample to this
assumption? Yes and no. No one (besides Git itself) is going to
include libgitstdlib.a in their project any time soon, so there's no
real "external interface" to define right now. Eventually, having
git-std-lib types in the hypothetical git-obj-store-interface.h _may_
happen, or it may not. I don't know.
...
But I think we're in agreement that pager.h isn't part of
git-std-lib's (currently undefined/non-existent) external interface,
and so doesn't need to be self-contained, and this patch should
probably be dropped?
>
> 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 0/2] builtin/clone: allow remote helpers to detect repo
From: Junio C Hamano @ 2024-02-27 21:33 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Mike Hommey
In-Reply-To: <xmqqcyshr5em.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> Hi,
>>
>> this patch series addresses a regression reported by Mike in Git v2.44
>> where remote helpers cannot access the Git repository anymore when
>> running git-clone(1).
>> ...
>> builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++
>> refs/reftable-backend.c | 1 +
>
> Sorry, but this confuses me. Was a regression really in v2.44.0,
> where refs/reftable-backend.c did not even exist? If so why does a
> fix for it need to touch that file?
>
> Thanks.
I guess [2/2] alone is the fix to be applied directly on top of v2.44.0
and eventually be merged to 'maint' to become v2.44.1 release, while
[1/2] is necessary to adjust reftable backend when such a fix is
merged to more recent codebase that already has the reftable
backend?
^ permalink raw reply
* Re: [PATCH 2/2] builtin/clone: allow remote helpers to detect repo
From: Junio C Hamano @ 2024-02-27 21:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Mike Hommey
In-Reply-To: <9d888adf92e9a8af7c18847219f97d3e595e3e36.1709041721.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Instead, fix this issue by partially initializing the refdb up to the
> point where it becomes discoverable by Git commands.
As you mentioned earlier, this indeed is ugly, but I agree that
there is no other reasonable way out. I am also unsure if this is a
viable workaround in the longer term, but it should do for now.
> + /*
> + * We have a chicken-and-egg situation between initializing the refdb
> + * and spawning transport helpers:
> + *
> + * - Initializing the refdb requires us to know about the object
> + * format. We thus have to spawn the transport helper to learn
> + * about it.
> + * - The transport helper may want to access the Git repository. But
> + * because the refdb has not been initialized, we don't have "HEAD"
> + * or "refs/". Thus, the helper cannot find the Git repository.
> + *
> + * Ideally, we would have structured the helper protocol such that it's
> + * mandatory for the helper to first announce its capabilities without
> + * yet assuming a fully initialized repository. Like that, we could
> + * have added a "lazy-refdb-init" capability that announces whether the
> + * helper is ready to handle not-yet-initialized refdbs. If any helper
> + * didn't support them, we would have fully initialized the refdb with
> + * the SHA1 object format, but later on bailed out if we found out that
> + * the remote repository used a different object format.
> + * But we didn't, and thus we use the following workaround to partially
> + * initialize the repository's refdb such that it can be discovered by
> + * Git commands. To do so, we:
> + *
> + * - Create an invalid HEAD ref pointing at "refs/heads/.invalid".
> + *
> + * - Create the "refs/" directory.
> + *
> + * - Set up the ref storage format and repository version as
> + * required.
"as required" by whom and in what way?
"The code to recognize a repository requires them to be set already,
but they do not have to be the real value---we just assign random
valid values for now, let remote helper do its work and then set the
real values after they are done" would be a plausible interpretation
of the above. Is that what is going on?
> + * This is sufficient for Git commands to discover the Git directory.
> + */
> + initialize_repository_version(GIT_HASH_UNKNOWN,
> + the_repository->ref_storage_format, 1);
OK, so we start with "we do not know it yet" here.
> + strbuf_addf(&buf, "%s/HEAD", git_dir);
> + write_file(buf.buf, "ref: refs/heads/.invalid");
> +
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "%s/refs", git_dir);
> + safe_create_dir(buf.buf, 1);
> +
> /*
> * additional config can be injected with -c, make sure it's included
> * after init_db, which clears the entire config environment.
> @@ -1453,6 +1498,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> free(remote_name);
> strbuf_release(&reflog_msg);
> strbuf_release(&branch_top);
> + strbuf_release(&buf);
> strbuf_release(&key);
> free_refs(mapped_refs);
> free_refs(remote_head_points_at);
> diff --git a/setup.c b/setup.c
> index b69b1cbc2a..e3b76e84b5 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1889,6 +1889,13 @@ void initialize_repository_version(int hash_algo,
> char repo_version_string[10];
> int repo_version = GIT_REPO_VERSION;
>
> + /*
> + * Note that we initialize the repository version to 1 when the ref
> + * storage format is unknown. This is on purpose so that we can add the
> + * correct object format to the config during git-clone(1). The format
> + * version will get adjusted by git-clone(1) once it has learned about
> + * the remote repository's format.
> + */
> if (hash_algo != GIT_HASH_SHA1 ||
> ref_storage_format != REF_STORAGE_FORMAT_FILES)
> repo_version = GIT_REPO_VERSION_READ;
> @@ -1898,7 +1905,7 @@ void initialize_repository_version(int hash_algo,
> "%d", repo_version);
> git_config_set("core.repositoryformatversion", repo_version_string);
>
> - if (hash_algo != GIT_HASH_SHA1)
> + if (hash_algo != GIT_HASH_SHA1 && hash_algo != GIT_HASH_UNKNOWN)
> git_config_set("extensions.objectformat",
> hash_algos[hash_algo].name);
> else if (reinit)
Here we refrain from writing extensions.objectformat in the
repository when we do not know what hash function is being used.
Without this change, we would instead remove extensions.objectformat,
which does not sound too bad, as "reinit" is passed by the new call
we saw above and this "else if (reinit)" will do exactly that anyway.
In any case, after we finish talking with the other side over the
transport, we make another call to initialize_repository_version()
with the real objectformat and everything will be fine.
The ealier description of the implementation idea made it sound
really yucky, but I do not think the solution presented here is too
bad.
> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
> index 1544d6dc6b..bcfb358c51 100755
> --- a/t/t5801/git-remote-testgit
> +++ b/t/t5801/git-remote-testgit
> @@ -12,6 +12,11 @@ url=$2
>
> dir="$GIT_DIR/testgit/$alias"
>
> +if ! git rev-parse --is-inside-git-dir
> +then
> + exit 1
> +fi
> +
> h_refspec="refs/heads/*:refs/testgit/$alias/heads/*"
> t_refspec="refs/tags/*:refs/testgit/$alias/tags/*"
^ permalink raw reply
* RE: [PATCH v2 0/2] Change xwrite() to write_in_full() in builtins.
From: rsbecker @ 2024-02-27 21:11 UTC (permalink / raw)
To: git
In-Reply-To: <20240227150934.7950-1-randall.becker@nexbridge.ca>
Please withdraw this series.
>-----Original Message-----
>From: Randall S. Becker <the.n.e.key@gmail.com>
>Sent: Tuesday, February 27, 2024 10:10 AM
>To: git@vger.kernel.org
>Cc: Randall S. Becker <rsbecker@nexbridge.com>
>Subject: [PATCH v2 0/2] Change xwrite() to write_in_full() in builtins.
>
>From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
>This series replaces xwrite to write_in_full in builtins/. The change is
required to fix critical problems that prevent full writes to be
>processed by on platforms where xwrite may be limited to a platform size
limit. Further changes outside of builtins/ may be required
>but do not appear to be as urgent as this change, which causes test
breakage in t7704. A separate series will be contributed for
>changes outside of builtins/ at a later date.
>
>The change in unpack-objects.c is necessary as len is being passed into
xwrite that exceeds the size supported by the limit in that
>method (56Kb on NonStop ia64).
>
>Randall S. Becker (3):
> builtin/repack.c: change xwrite to write_in_full and report errors.
> builtin/receive-pack.c: change xwrite to write_in_full.
> builtin/unpack-objects.c: change xwrite to write_in_full avoid
> truncation.
>
> builtin/receive-pack.c | 2 +-
> builtin/repack.c | 9 +++++++--
> builtin/unpack-objects.c | 2 +-
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
>--
>2.42.1
^ permalink raw reply
* [PATCH v3] completion: fix __git_complete_worktree_paths
From: Rubén Justo @ 2024-02-27 21:06 UTC (permalink / raw)
To: Git List; +Cc: Patrick Steinhardt
In-Reply-To: <eaf3649e-30cf-4eba-befa-5be826c828a8@gmail.com>
Use __git to invoke "worktree list" in __git_complete_worktree_paths, to
respect any "-C" and "--git-dir" options present on the command line.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
I stumbled upon this in a situation like:
$ git init /tmp/foo && cd /tmp/foo
$ git worktree add --orphan foo_wt
$ git init /tmp/bar && cd /tmp/bar
$ git worktree add --orphan bar_wt
$ cd /tmp/foo
$ git -C /tmp/bar worktree remove <TAB>
... expecting /tmp/bar/wt, but ...
$ git -C /tmp/bar worktree remove /tmp/foo_wt
The function __git was introduced in 1cd23e9e05 (completion: don't use
__gitdir() for git commands, 2017-02-03). It is a small function, so
I'll include here to ease the review of this patch:
# Runs git with all the options given as argument, respecting any
# '--git-dir=<path>' and '-C <path>' options present on the command line
__git ()
{
git ${__git_C_args:+"${__git_C_args[@]}"} \
${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
}
Here are the changes since v2:
"test_completion" already asserts if something is printed on stderr.
Therefore "test_must_be_empty err" does not make sense. Removed. This
makes the test_completion with an empty response, more valuable, IMO.
But I'm open to change it.
The second test_when_finished has been fixed.
Thanks in advance for your review.
Range-diff against v2:
1: 4cc071fb8e ! 1: de5d2ca1d3 completion: fix __git_complete_worktree_paths
@@ t/t9902-completion.sh: test_expect_success '__git_complete_fetch_refspecs - full
+ cd non-repo &&
+ GIT_CEILING_DIRECTORIES="$ROOT" &&
+ export GIT_CEILING_DIRECTORIES &&
-+ test_completion "git worktree remove " "" 2>err &&
-+ test_must_be_empty err
++ test_completion "git worktree remove " ""
+ )
+'
+
+test_expect_success '__git_complete_worktree_paths with -C' '
-+ test_when_finished "rm -rf to_delete" &&
++ test_when_finished "git -C otherrepo worktree remove otherrepo_wt" &&
+ git -C otherrepo worktree add --orphan otherrepo_wt &&
+ run_completion "git -C otherrepo worktree remove " &&
+ grep otherrepo_wt out
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 444b3efa63..86e55dc67f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3571,7 +3571,7 @@ __git_complete_worktree_paths ()
# Generate completion reply from worktree list skipping the first
# entry: it's the path of the main worktree, which can't be moved,
# removed, locked, etc.
- __gitcomp_nl "$(git worktree list --porcelain |
+ __gitcomp_nl "$(__git worktree list --porcelain |
sed -n -e '2,$ s/^worktree //p')"
}
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b16c284181..aae2cbeeea 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1263,6 +1263,29 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
test_cmp expected out
'
+test_expect_success '__git_complete_worktree_paths' '
+ test_when_finished "git worktree remove other_wt" &&
+ git worktree add --orphan other_wt &&
+ run_completion "git worktree remove " &&
+ grep other_wt out
+'
+
+test_expect_success '__git_complete_worktree_paths - not a git repository' '
+ (
+ cd non-repo &&
+ GIT_CEILING_DIRECTORIES="$ROOT" &&
+ export GIT_CEILING_DIRECTORIES &&
+ test_completion "git worktree remove " ""
+ )
+'
+
+test_expect_success '__git_complete_worktree_paths with -C' '
+ test_when_finished "git -C otherrepo worktree remove otherrepo_wt" &&
+ git -C otherrepo worktree add --orphan otherrepo_wt &&
+ run_completion "git -C otherrepo worktree remove " &&
+ grep otherrepo_wt out
+'
+
test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '
test_completion "git switch " <<-\EOF
branch-in-other Z
--
2.44.0.1.g0da3aa8f7f
^ permalink raw reply related
* RE: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.
From: rsbecker @ 2024-02-27 21:05 UTC (permalink / raw)
To: 'Jeff King'
Cc: 'Junio C Hamano', 'Randall S. Becker', git
In-Reply-To: <20240227192530.GD3784114@coredump.intra.peff.net>
On Tuesday, February 27, 2024 2:26 PM, Peff wrote:
>To: rsbecker@nexbridge.com
>Cc: 'Junio C Hamano' <gitster@pobox.com>; 'Randall S. Becker' <the.n.e.key@gmail.com>; git@vger.kernel.org
>Subject: Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.
>
>On Tue, Feb 27, 2024 at 02:04:46PM -0500, rsbecker@nexbridge.com wrote:
>
>> >> 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;
>> [...]
>> I experimented with using write_in_full vs. keeping xwrite. With
>> xwrite in this loop, t7704.9 consistently fails as described in the
>> other thread. With write_in_full, the code works correctly. I assume
>> there are side-effects that are present. This change is critical to
>> having the code work on NonStop. Otherwise git seems to be at risk of
>> actually being seriously broken if unpack does not work correctly. I
>> am happy to have my series ignored as long as the problem is otherwise corrected.
>
>I'm somewhat skeptical that this code is to blame, as it should be run very rarely at all; it is just dumping any content in the pack stream
>after the end of the checksum to stdout. But in normal use by Git, there is no such content in the first place.
>
>If I do this:
>
>diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index e0a701f2b3..affe55035d 100644
>--- a/builtin/unpack-objects.c
>+++ b/builtin/unpack-objects.c
>@@ -680,11 +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);
>- if (ret <= 0)
>- break;
>- len -= ret;
>- offset += ret;
>+ BUG("cruft at the end of the pack!");
> }
>
> /* All done */
>
>then t7704 still passes, as it does not run this code at all. In fact, nothing in the test suite fails. Which is not to say we should get rid of
>those code. If we were writing today we might flag it as an error, but we should keep it for historical compatibility.
>
>But I do not see any bug in the code, and nor do I think it could contribute to a test failure.
I have obviously gone down the wrong path trying to resolve this situation. Please consider this entire series dropped with my apologies for the time-waste.
Unfortunately, I do not have sufficient knowledge of the code to resolve the originally reported problem without further assistance to determine the root case (assuming it still is a problem). Changes in master post-2.44.0 appear to have contributed to resolving the situation, so I am now getting random pass/fail on the test. I'm going to hold 2.44.0 on ia64 and wait for a subsequent release at retest at that time.
Sadly,
--Randall
^ permalink raw reply
* Re: [PATCH 1/2] Subject: [GSOC][RFC PATCH 1/2] Add builtin patterns for JavaScript function detection in userdiff.
From: Sergius Nyah @ 2024-02-27 21:05 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: christian.couder, pk, git
In-Reply-To: <CZG3HO25QLAG.3Q9V03SCO99HL@gmail.com>
On Tue, Feb 27, 2024 at 8:06 PM Ghanshyam Thakkar
<shyamthakkar001@gmail.com> wrote:
>
> On Tue Feb 27, 2024 at 9:32 PM IST, Sergius Nyah wrote:
> > [PATCH 1/2] Subject: [GSOC][RFC PATCH 1/2] Add builtin patterns for JavaScript function detection in userdiff.
>
> I think these prefixes somehow got added twice. In any case, this makes
> it so that "Subject: [GSOC][RFC PATCH 1/2]" is part of the commit log,
> which shoud not be. And the subject can be better written as:
>
> userdiff: add builtin patterns for Javascript
>
Noted, thanks!
> > This patch adds the regular expression for detecting JavaScript functions and expressions in Git diffs. The pattern accurately identifies function declerations, expressions, and definitions inside blocks.
>
> commit message should be wrapped at 72 characters.
> > ---
Thank you for the correction.
> > userdiff.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/userdiff.c b/userdiff.c
> > index e399543823..12e31ff14d 100644
> > --- a/userdiff.c
> > +++ b/userdiff.c
> > @@ -1,7 +1,7 @@
> > #include "git-compat-util.h"
> > #include "config.h"
> > #include "userdiff.h"
> > -#include "attr.h"
> > +#include "attr.h"
>
> This change adds trailing whitespace which is not desired.
>
Okay. Would avoid them in subsequent commits.
> > #include "strbuf.h"
> >
> > static struct userdiff_driver *drivers;
> > @@ -183,6 +183,19 @@ PATTERNS("java",
> > "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
> > "|[-+*/<>%&^|=!]="
> > "|--|\\+\\+|<<=?|>>>?=?|&&|\\|\\|"),
> > +PATTERNS("javascript",
> > + /*
> > + * Looks for lines that start with optional whitespace, followed
> > + * by 'function'* and any characters (for function declarations),
> > + * or valid JavaScript identifiers, equals sign '=', 'function' keyword
> > + * and any characters (for function expressions).
> > + * Also considers functions defined inside blocks with '{...}'.
> > + */
> > + "^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
> > + /* This pattern matches JavaScript identifiers */
> > + "[a-zA-Z_$][0-9a-zA-Z_$]*"
> > + "|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
> > + "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
>
> Majority of the lines above (including comment) have trailing whitespace.
> Consider removing them. Also, tabs should be used for indentation instead
> of spaces. (cf. Documentation/CodingGuidelines)
Well received. Thanks!
>
> > PATTERNS("kotlin",
> > "^[ \t]*(([a-z]+[ \t]+)*(fun|class|interface)[ \t]+.*)$",
> > /* -- */
> > @@ -192,7 +205,7 @@ PATTERNS("kotlin",
> > /* integers and floats */
> > "|[0-9][0-9_]*([.][0-9_]*)?([Ee][-+]?[0-9]+)?[fFlLuU]*"
> > /* floating point numbers beginning with decimal point */
> > - "|[.][0-9][0-9_]*([Ee][-+]?[0-9]+)?[fFlLuU]?"
> > + "|[.][0-9][0-9_]*([Ee][-+]?[0-9]+ )?[fFlLuU]?"
>
> This adds extra whitespace in the middle. Changes not related to the patch
> topic should be avoided.
Sorry about that, I just touched what I shouldn't have.
>
> Also this was attempted before, see [1]. I think you can take some
> inspiration and inputs from that also. Aside from that I think the
> separate patch which adds tests for this can be squashed into this
> one.
>
> [1]:
> https://lore.kernel.org/git/20220403132508.28196-1-a97410985new@gmail.com/
>
> Thanks.
Thank you for all the corrections. I'd submit a patch series with the
correct practices.
^ permalink raw reply
* Re: [PATCH 0/2] builtin/clone: allow remote helpers to detect repo
From: Junio C Hamano @ 2024-02-27 20:58 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Mike Hommey
In-Reply-To: <cover.1709041721.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this patch series addresses a regression reported by Mike in Git v2.44
> where remote helpers cannot access the Git repository anymore when
> running git-clone(1).
> ...
> builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++
> refs/reftable-backend.c | 1 +
Sorry, but this confuses me. Was a regression really in v2.44.0,
where refs/reftable-backend.c did not even exist? If so why does a
fix for it need to touch that file?
Thanks.
^ permalink raw reply
* Re: [PATCH] credential/osxkeychain: store new attributes
From: Junio C Hamano @ 2024-02-27 20:13 UTC (permalink / raw)
To: M Hickford, Bo Anderson; +Cc: M Hickford via GitGitGadget, git, Jeff King
In-Reply-To: <CAGJzqsknN_RmYeT0xcn4cTLcJhsxSOUC6ppRVepxMDf3day5Fw@mail.gmail.com>
M Hickford <mirth.hickford@gmail.com> writes:
> On Wed, 14 Feb 2024 at 22:35, M Hickford <mirth.hickford@gmail.com> wrote:
>>
>> > Thanks. Will queue.
>>
>> A first-time contributor contacted me to say they are working on a
>> more comprehensive patch to credential-osxkeychain, so let's wait for
>> that instead. https://github.com/gitgitgadget/git/pull/1663#issuecomment-1942763116
>
> Please disregard my patch and look at Bo Anderson's instead
> https://lore.kernel.org/git/pull.1667.git.1708212896.gitgitgadget@gmail.com/
Will drop mh/credential-oauth-refresh-token-with-osxkeychain topic.
The other one seemed to have got some reviews and I think the
current status of the series is that it is Bo's turn to respond with
a new iteration of the series.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Kyle Lippincott @ 2024-02-27 20:10 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Calvin Wan, git, Jonathan Tan, phillip.wood123
In-Reply-To: <20240227084529.GJ3263678@coredump.intra.peff.net>
On Tue, Feb 27, 2024 at 12:45 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Feb 26, 2024 at 04:56:28PM -0800, Kyle Lippincott wrote:
>
> > > 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.
>
> Is it always, though? That's what C99 says, but if you have a system
> that does not have inttypes.h in the first place, but does have
> stdint.h, it seems possible that it provides conversion macros elsewhere
> (either via stdint.h, or possibly just as part of stdio.h).
It's of course possible that on some platforms, stdio.h or stdint.h
defines these types (or includes inttypes.h internally, which defines
these types). However, I think that to be "correct" and for a compiler
to claim it supports C99 (and the compiler _must_ claim that because
of the guard in <git-compat-util.h>), inttypes.h must exist, and it
must cause these symbols to appear. If there are platforms that are
claiming to be C99 and inttypes.h doesn't exist or doesn't provide the
symbols it should, I don't think we should try to support them - they
can maintain platform-specific patches for whatever not-actually-C99
language the platform supports. Basically what git for windows is
already doing (presumably for other reasons), as far as I can tell.
>
> So it might be that things have been horribly broken on NO_INTTYPES_H
> systems for a while, and nobody is checking. But I don't think you can
> really say so without looking at such a system.
>
> And looking at config.mak.uname, it looks like Windows is such a system.
> Does it really have inttypes.h and it is getting included from somewhere
> else, making format conversion macros work? Or does it provide those
> macros elsewhere, and really needs stdint? It does look like
> compat/mingw.h includes it, but I think we wouldn't use that for msvc
> builds.
>
> > 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.
>
> It is good to clean up old conditionals if they are no longer
> applicable, as they are a burden to reason about later (as this
> discussion shows). But I am not sure about your "just to demonstrate we
> can".
Yeah, I'm also not convinced the "just to demonstrate we can" has much
value. I was trying to get ahead of future discussions where we claim
it's important to never include stdint.h (because people remember this
discussion and how contentious it was) and think it might misbehave,
and instead just include it in <git-compat-util.h> to prove it
_doesn't_ misbehave, and thus start to allow usage in self-contained
headers when necessary.
> It is good to try it out, but it looks like there is a non-zero
> chance that MSVC on Windows might break. It is probably better to try
> building there or looping in folks who can, rather than just making a
> change and seeing if anybody screams.
I think I miscommunicated here, or had too many assumptions about the
current state of things that I didn't actually verify. When I wrote
"and seeing if the build bots or maintainers identify it as a
continuing issue", I was assuming that we had build bots for all major
platforms (including windows, with however it gets built: mingw or VC
or whatever), and I included the "maintainers" part of it for the long
tail of esoteric platforms that we either don't know about, or can't
have automated builds on for whatever reason. I agree that making
changes that have a high likelihood of breaking supported platforms
(which gets back to that platform support thread that was started a
few weeks ago) should not be done lightly, and it's not reasonable to
make the change and wait for maintainers of these "supported
platforms" to complain. I was relying on the build bots covering the
"supported platforms" and stopping me from even sending such a patch
to the mailing list :)
>
> I think the "win+VS" test in the GitHub Actions CI job might cover this
> case. It is not run by default (because it was considered be mostly
> redundant with the mingw build), but it shouldn't be too hard to enable
> it for a one-off test.
>
> -Peff
^ permalink raw reply
* Re: [PATCH] credential/osxkeychain: store new attributes
From: M Hickford @ 2024-02-27 20:00 UTC (permalink / raw)
To: M Hickford, Junio C Hamano; +Cc: M Hickford via GitGitGadget, git, Jeff King
In-Reply-To: <CAGJzqsmSzMqEG1OU9dH6CORV6=L7qUAFNJSmi41Lqrajf9mSew@mail.gmail.com>
On Wed, 14 Feb 2024 at 22:35, M Hickford <mirth.hickford@gmail.com> wrote:
>
> > Thanks. Will queue.
>
> A first-time contributor contacted me to say they are working on a
> more comprehensive patch to credential-osxkeychain, so let's wait for
> that instead. https://github.com/gitgitgadget/git/pull/1663#issuecomment-1942763116
Please disregard my patch and look at Bo Anderson's instead
https://lore.kernel.org/git/pull.1667.git.1708212896.gitgitgadget@gmail.com/
^ permalink raw reply
* Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.
From: Jeff King @ 2024-02-27 19:25 UTC (permalink / raw)
To: rsbecker; +Cc: 'Junio C Hamano', 'Randall S. Becker', git
In-Reply-To: <03be01da69af$d8366e10$88a34a30$@nexbridge.com>
On Tue, Feb 27, 2024 at 02:04:46PM -0500, rsbecker@nexbridge.com wrote:
> >> 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;
> [...]
> I experimented with using write_in_full vs. keeping xwrite. With xwrite in
> this loop, t7704.9 consistently fails as described in the other thread. With
> write_in_full, the code works correctly. I assume there are side-effects
> that are present. This change is critical to having the code work on
> NonStop. Otherwise git seems to be at risk of actually being seriously
> broken if unpack does not work correctly. I am happy to have my series
> ignored as long as the problem is otherwise corrected.
I'm somewhat skeptical that this code is to blame, as it should be run
very rarely at all; it is just dumping any content in the pack stream
after the end of the checksum to stdout. But in normal use by Git, there
is no such content in the first place.
If I do this:
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index e0a701f2b3..affe55035d 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -680,11 +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);
- if (ret <= 0)
- break;
- len -= ret;
- offset += ret;
+ BUG("cruft at the end of the pack!");
}
/* All done */
then t7704 still passes, as it does not run this code at all. In fact,
nothing in the test suite fails. Which is not to say we should get rid
of those code. If we were writing today we might flag it as an error,
but we should keep it for historical compatibility.
But I do not see any bug in the code, and nor do I think it could
contribute to a test failure.
-Peff
^ permalink raw reply related
* Re: Interest in Future Collaboration for GSoC 2024
From: Kaartic Sivaraam @ 2024-02-27 19:21 UTC (permalink / raw)
To: Akhilesh Kumar Yadav; +Cc: git
In-Reply-To: <39C00ED0-2EA9-4BCA-8662-AAEC5A108D49@gmail.com>
Hi Akhilesh,
Thank you for your interest!
Kindly check out our mentor programming guide[1] and SoC ideas page[2].
Completing microprojects is a requirement for selection. The more active
you are with your contributions, the more you learn and higher the
chance of selection :-)
[1]: https://git.github.io/Mentoring-Program-Guide/
[2]: https://git.github.io/SoC-2024-Ideas/
On 26/02/24 04:40, Akhilesh Kumar Yadav wrote:
>
> I hope this message finds you well. I am writing to express my admiration for the innovative technology that your organization is working on for GSoC 2024. I have been following your work and find it truly amazing.
>
> Currently, I am committed to another organisation and may not be able to contribute to your projects at this time. However, I noticed that your organisation sometimes faces a shortage of proposals. If such a situation arises, please consider this email as an expression of my interest.
>
> I hold a strong interest in your organisation and would be more than willing to submit a comprehensive proposal should the need arise. I have also been an active contribution at open source projects and have decent programming experience.
>
> Thank you. I look forward to the possibility of future interactions.
>
> Best regards,
> Akhilesh.
--
Sivaraam
^ permalink raw reply
* Re: [PATCH v3 04/11] Prepare `paint_down_to_common()` for handling shallow commits
From: Dirk Gouders @ 2024-02-27 19:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git,
Patrick Steinhardt
In-Reply-To: <xmqqcyshu6es.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Currently, that logic pretends that a commit whose parent
>> commit is missing is a root commit (and likewise merge commits
>> with missing parent commits are handled incorrectly, too).
>> However, for the purpose of `--update-shallow` that is exactly
>> what we need to do (and only then).
>
> I suspect that what made it harder to follow in the original
> construct is that we called the behaviour "incorrect" upfront and
> then come back with "that incorrectness is what we want". I wonder
> if it makes it easier to follow by flipping them around.
>
> For the purpose of `--update-shallow`, when some of the parent
> commits of a commit are missing from the repository, we need to
> treat as if the parents of the commit are only the ones that do
> exist in the repository and these missing commits have no
> ancestry relationship with it. If all its parent commits are
> missing, the commit needs to be treated as if it were a root
> commit.
>
> Add a flag to optionally ask for such a behaviour, while
> detecting missing objects as a repository corruption error by
> default ...
>
> or something?
At least to me this is more understandable.
Dirk
^ permalink raw reply
* Re: [PATCH 1/2] Subject: [GSOC][RFC PATCH 1/2] Add builtin patterns for JavaScript function detection in userdiff.
From: Ghanshyam Thakkar @ 2024-02-27 19:06 UTC (permalink / raw)
To: Sergius Nyah, christian.couder, pk; +Cc: git
In-Reply-To: <20240227160253.104011-2-sergiusnyah@gmail.com>
On Tue Feb 27, 2024 at 9:32 PM IST, Sergius Nyah wrote:
> [PATCH 1/2] Subject: [GSOC][RFC PATCH 1/2] Add builtin patterns for JavaScript function detection in userdiff.
I think these prefixes somehow got added twice. In any case, this makes
it so that "Subject: [GSOC][RFC PATCH 1/2]" is part of the commit log,
which shoud not be. And the subject can be better written as:
userdiff: add builtin patterns for Javascript
> This patch adds the regular expression for detecting JavaScript functions and expressions in Git diffs. The pattern accurately identifies function declerations, expressions, and definitions inside blocks.
commit message should be wrapped at 72 characters.
> ---
> userdiff.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/userdiff.c b/userdiff.c
> index e399543823..12e31ff14d 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -1,7 +1,7 @@
> #include "git-compat-util.h"
> #include "config.h"
> #include "userdiff.h"
> -#include "attr.h"
> +#include "attr.h"
This change adds trailing whitespace which is not desired.
> #include "strbuf.h"
>
> static struct userdiff_driver *drivers;
> @@ -183,6 +183,19 @@ PATTERNS("java",
> "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
> "|[-+*/<>%&^|=!]="
> "|--|\\+\\+|<<=?|>>>?=?|&&|\\|\\|"),
> +PATTERNS("javascript",
> + /*
> + * Looks for lines that start with optional whitespace, followed
> + * by 'function'* and any characters (for function declarations),
> + * or valid JavaScript identifiers, equals sign '=', 'function' keyword
> + * and any characters (for function expressions).
> + * Also considers functions defined inside blocks with '{...}'.
> + */
> + "^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
> + /* This pattern matches JavaScript identifiers */
> + "[a-zA-Z_$][0-9a-zA-Z_$]*"
> + "|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
> + "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
Majority of the lines above (including comment) have trailing whitespace.
Consider removing them. Also, tabs should be used for indentation instead
of spaces. (cf. Documentation/CodingGuidelines)
> PATTERNS("kotlin",
> "^[ \t]*(([a-z]+[ \t]+)*(fun|class|interface)[ \t]+.*)$",
> /* -- */
> @@ -192,7 +205,7 @@ PATTERNS("kotlin",
> /* integers and floats */
> "|[0-9][0-9_]*([.][0-9_]*)?([Ee][-+]?[0-9]+)?[fFlLuU]*"
> /* floating point numbers beginning with decimal point */
> - "|[.][0-9][0-9_]*([Ee][-+]?[0-9]+)?[fFlLuU]?"
> + "|[.][0-9][0-9_]*([Ee][-+]?[0-9]+ )?[fFlLuU]?"
This adds extra whitespace in the middle. Changes not related to the patch
topic should be avoided.
Also this was attempted before, see [1]. I think you can take some
inspiration and inputs from that also. Aside from that I think the
separate patch which adds tests for this can be squashed into this
one.
[1]:
https://lore.kernel.org/git/20220403132508.28196-1-a97410985new@gmail.com/
Thanks.
^ permalink raw reply
* RE: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.
From: rsbecker @ 2024-02-27 19:04 UTC (permalink / raw)
To: 'Junio C Hamano', 'Randall S. Becker'; +Cc: git
In-Reply-To: <xmqq1q8xspht.fsf@gitster.g>
On Tuesday, February 27, 2024 1:59 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 if the supplied len value exceeds the supported value.
>> Replacing xwrite with write_in_full corrects this problem. Future
>> optimisations could remove the loop in favour of just calling
write_in_full.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>> builtin/unpack-objects.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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;
>
>Why do we need this with a retry loop that is prepared for short
>write(2) specifically like this?
>
>If xwrite() calls underlying write(2) with too large a value, then your
MAX_IO_SIZE is misconfigured, and the fix should go there, not
>here in a loop that expects a working xwrite() that is allowed to return on
short write(2), I would think.
I experimented with using write_in_full vs. keeping xwrite. With xwrite in
this loop, t7704.9 consistently fails as described in the other thread. With
write_in_full, the code works correctly. I assume there are side-effects
that are present. This change is critical to having the code work on
NonStop. Otherwise git seems to be at risk of actually being seriously
broken if unpack does not work correctly. I am happy to have my series
ignored as long as the problem is otherwise corrected.
^ permalink raw reply
* Re: [PATCH v2 2/3] builtin/receive-pack.c: change xwrite to write_in_full.
From: Junio C Hamano @ 2024-02-27 19:00 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240227150934.7950-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.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> builtin/receive-pack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index db65607485..4277c63d08 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -456,7 +456,7 @@ 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);
> + write_in_full(2, msg, sz);
> }
This change does make sense, as we can see a short write(2) from
xwrite() and this caller is not repeating the call to flush the
remainder after a short write.
>
> __attribute__((format (printf, 1, 2)))
^ permalink raw reply
* Re: [PATCH v2 3/3] builtin/unpack-objects.c: change xwrite to write_in_full avoid truncation.
From: Junio C Hamano @ 2024-02-27 18:58 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240227150934.7950-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 if the supplied
> len value exceeds the supported value. Replacing xwrite with write_in_full
> corrects this problem. Future optimisations could remove the loop in favour
> of just calling write_in_full.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> builtin/unpack-objects.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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;
Why do we need this with a retry loop that is prepared for short
write(2) specifically like this?
If xwrite() calls underlying write(2) with too large a value, then
your MAX_IO_SIZE is misconfigured, and the fix should go there, not
here in a loop that expects a working xwrite() that is allowed to
return on short write(2), I would think.
^ permalink raw reply
* Re: [PATCH v2 1/3] builtin/repack.c: change xwrite to write_in_full and report errors.
From: Junio C Hamano @ 2024-02-27 18:49 UTC (permalink / raw)
To: Randall S. Becker; +Cc: git, Randall S. Becker
In-Reply-To: <20240227150934.7950-2-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 misleads readers to think that maximum single I/O size is
smaller than a single write of oid_to_hex() string on some
platforms. I somehow do not think that is why we want to make this
change.
Rather, the use of these xwrites() are simply wrong regardless of
maximum I/O size of the platforms, as this caller is not prepared to
see xwrite() result in a short write(2), and we do want to write all
bytes we have even in such a case.
You're right to also point out that we attempt to propagate the errors
to the caller (but see below).
> 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;
> }
I think this has already been brought up, but the caller of this
helper does not make such an error stand out enough and instead
makes the resulting repack silently produce wrong result, which
is not an improvement. Perhaps
if (write_in_full(...) ||
write_in_full(...))
die(_("failed to list promisor objects to repack"));
or something?
Thanks.
^ permalink raw reply
* Re: [PATCH] cmake: adapt to the Git Standard Library
From: Junio C Hamano @ 2024-02-27 18:40 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1677.git.1709045045235.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the commit "git-std-lib: introduce Git Standard Library" of the
> `cw/git-std-lib` topic, the Makefile was restructured in a manner that
> requires the CMake definition to follow suit to be able to build Git.
>
> This commit adjusts the CMake definition accordingly.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> cmake: adapt to the Git Standard Library
>
> The usual CMake adjustments. This is based on cw/git-std-lib.
Thanks for a fix.
std-lib folks, can you fold this patch (with authorship credits
intact) into your series when it gets rerolled from here on?
Thanks.
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1677%2Fdscho%2Fcmake-vs-git-std-lib-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1677/dscho/cmake-vs-git-std-lib-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1677
>
> contrib/buildsystems/CMakeLists.txt | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 804629c525b..6f46f8f9070 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -676,10 +676,12 @@ include_directories(${CMAKE_BINARY_DIR})
> #build
> #libgit
> parse_makefile_for_sources(libgit_SOURCES "LIB_OBJS")
> +parse_makefile_for_sources(std_lib_SOURCES "STD_LIB_OBJS")
>
> list(TRANSFORM libgit_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
> +list(TRANSFORM std_lib_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
> list(TRANSFORM compat_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
> -add_library(libgit ${libgit_SOURCES} ${compat_SOURCES})
> +add_library(libgit ${libgit_SOURCES} ${std_lib_SOURCES} ${compat_SOURCES})
>
> #libxdiff
> parse_makefile_for_sources(libxdiff_SOURCES "XDIFF_OBJS")
>
> base-commit: c9e04a1e1f954dd60b1373f75b710f64d34e502b
^ permalink raw reply
* Re: [PATCH] rebase -i: stop setting GIT_CHERRY_PICK_HELP
From: Junio C Hamano @ 2024-02-27 18:38 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget; +Cc: git, Johannes Schindelin, Phillip Wood
In-Reply-To: <pull.1678.git.1709042783847.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Note that we retain the changes in e4301f73fff (sequencer: unset
> GIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case
> GIT_CHERRY_PICK_HELP is set in the environment when "git rebase" is
> run.
Is this comment about this part of the code?
> + const char *msg;
> +
> + if (is_rebase_i(opts))
> + msg = rebase_resolvemsg;
> + else
> + msg = getenv("GIT_CHERRY_PICK_HELP");
Testing is_rebase_i() first means we ignore the environment
unconditionally and use our own message always in "rebase -i", no?
Not that I think we should honor the environment variable and let it
override our message. I just found the description a bit confusing.
> diff --git a/sequencer.h b/sequencer.h
> index dcef7bb99c0..437eabd38af 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -14,6 +14,8 @@ const char *rebase_path_todo(void);
> const char *rebase_path_todo_backup(void);
> const char *rebase_path_dropped(void);
>
> +extern const char *rebase_resolvemsg;
This is more library-ish part of the system than a random file in
the builtin/ directory. This place as the final location for the
string makes sense to me.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox