* [PATCH 0/4] t/lib-httpd ssl fixes
From: Jeff King @ 2023-02-01 11:35 UTC (permalink / raw)
To: git; +Cc: Todd Zullinger
While chasing down a possible HTTP/2 problem in our test suite (which
turns out not to be a Git bug, I think), I tried running the tests with
LIB_HTTPD_SSL=1, as TLS affects HTTP/2 upgrade.
Sadly, apache would not start at all with our ssl config. It looks like
this has probably been broken for many years, depending on your apache
and openssl versions.
The final two patches here fix ssl problems I found. The first two
patches drop support for older apache. This yields some minor cleanups,
and makes the ssl fixes slightly easier. I've cc'd Todd as the last
person to express support for Apache 2.2, in 2017. I'm hoping even
CentOS has moved on by now, but we'll see. :)
[1/4]: t/lib-httpd: bump required apache version to 2.2
[2/4]: t/lib-httpd: bump required apache version to 2.4
[3/4]: t/lib-httpd: drop SSLMutex config
[4/4]: t/lib-httpd: increase ssl key size to 2048 bits
t/lib-httpd.sh | 11 +++++++----
t/lib-httpd/apache.conf | 31 ++++---------------------------
t/lib-httpd/ssl.cnf | 2 +-
3 files changed, 12 insertions(+), 32 deletions(-)
-Peff
^ permalink raw reply
* Re: Stability of git-archive, breaking (?) the Github universe, and a possible solution
From: demerphq @ 2023-02-01 11:34 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: brian m. carlson, Konstantin Ryabitsev, Eli Schwartz, Git List
In-Reply-To: <230201.86pmatr9mj.gmgdl@evledraar.gmail.com>
On Wed, 1 Feb 2023 at 11:26, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> That would be going above & beyond what's needed IMO, but still a lot
> easier than the daunting task of writing a specification that exactly
> described GNU gzip's current behavior, to the point where you could
> clean-room implement it and be guaranteed byte-for-byte compatibility.
Why does it have to be gzip? It is not that hard to come up with a
relatively good compression algorithm that is stable if you aren't
expecting super fast performance or super good compression. If all you
need is good enough but stability is a hard requirement then
algorithms like LZW are available (it has been out of patent since
~2003), and produce reasonable results. If people want a stable
archive then they might have to use some tool that git provides to
decompress and they might not get the best compression ratios, nor
speed, but they would get stability. You can write a decent LZW
implementation in a few hundred lines of code. With a bit of care you
could implement it in a way that allows you to compute the true hash
digest of the compressed data without actually decompressing it as
well, which would address some of the concerns that brian raised with
regard to security I think.
Why does this email remind me of that old canard that any sufficiently
advanced piece of software gains the ability to send emails? :-)
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
^ permalink raw reply
* [PATCH 2/2] imap-send: not define USE_CURL_FOR_IMAP_SEND in Makefile
From: Jiang Xin @ 2023-02-01 11:31 UTC (permalink / raw)
To: Git List, Junio C Hamano, Bernhard Reiter, Remi Pommarel
Cc: Jiang Xin, Jiang Xin
In-Reply-To: <20230201113133.10195-1-worldhello.net@gmail.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
The definition and using of macro "USE_CURL_FOR_IMAP_SEND" are at
different locations. It is defined in Makefile and is used in file
"imap-send.c". Even though we have fixed the mismatched "curl_config"
issue in Makefile in the previous commit, moving the definition of the
macro "USE_CURL_FOR_IMAP_SEND" to souce code "imap-send.c" seems more
nature and may help us to use curl in imap-send by force in future by
removing "USE_CURL_FOR_IMAP_SEND".
The side effect of this change is that the "git-imap-send" program may
be larger than necessary if we have a lower version of libcurl
installed.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
Makefile | 11 ++---------
contrib/buildsystems/CMakeLists.txt | 3 ---
imap-send.c | 29 +++++++++++++++++------------
3 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/Makefile b/Makefile
index f4eaf22523..83721216fc 100644
--- a/Makefile
+++ b/Makefile
@@ -1621,15 +1621,8 @@ else
ifndef NO_EXPAT
PROGRAM_OBJS += http-push.o
endif
- curl_check := $(shell (echo 072200; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p)
- ifeq "$(curl_check)" "072200"
- USE_CURL_FOR_IMAP_SEND = YesPlease
- endif
- ifdef USE_CURL_FOR_IMAP_SEND
- BASIC_CFLAGS += -DUSE_CURL_FOR_IMAP_SEND
- IMAP_SEND_BUILDDEPS = http.o
- IMAP_SEND_LDFLAGS += $(CURL_LIBCURL)
- endif
+ IMAP_SEND_BUILDDEPS = http.o
+ IMAP_SEND_LDFLAGS += $(CURL_LIBCURL)
ifndef NO_EXPAT
ifdef EXPATDIR
BASIC_CFLAGS += -I$(EXPATDIR)/include
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2f6e0197ff..d508db4d29 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -622,9 +622,6 @@ if(NOT CURL_FOUND)
message(WARNING "git-http-push and git-http-fetch will not be built")
else()
list(APPEND PROGRAMS_BUILT git-http-fetch git-http-push git-imap-send git-remote-http)
- if(CURL_VERSION_STRING VERSION_GREATER_EQUAL 7.34.0)
- add_compile_definitions(USE_CURL_FOR_IMAP_SEND)
- endif()
endif()
if(NOT EXPAT_FOUND)
diff --git a/imap-send.c b/imap-send.c
index a50af56b82..c0a2c2b4e6 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -30,20 +30,25 @@
#if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)
typedef void *SSL;
#endif
-#ifdef USE_CURL_FOR_IMAP_SEND
+#ifdef NO_CURL
+#define USE_CURL_FOR_IMAP_SEND 0
+#else
#include "http.h"
-#endif
-
-#if defined(USE_CURL_FOR_IMAP_SEND)
-/* Always default to curl if it's available. */
-#define USE_CURL_DEFAULT 1
+/*
+ * Since version 7.30.0, libcurl's API has been able to communicate with
+ * IMAP servers, and curl's CURLOPT_LOGIN_OPTIONS (enabling IMAP
+ * authentication) parameter is available if curl's version is >= 7.34.0,
+ * Always use curl if there is a matching libcurl.
+ */
+#if LIBCURL_VERSION_NUM >= 0x072200
+#define USE_CURL_FOR_IMAP_SEND 1
#else
-/* We don't have curl, so continue to use the historical implementation */
-#define USE_CURL_DEFAULT 0
+#define USE_CURL_FOR_IMAP_SEND 0
+#endif
#endif
static int verbosity;
-static int use_curl = USE_CURL_DEFAULT;
+static int use_curl = USE_CURL_FOR_IMAP_SEND;
static const char * const imap_send_usage[] = { "git imap-send [-v] [-q] [--[no-]curl] < <mbox>", NULL };
@@ -1396,7 +1401,7 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
return 0;
}
-#ifdef USE_CURL_FOR_IMAP_SEND
+#if USE_CURL_FOR_IMAP_SEND
static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
{
CURL *curl;
@@ -1531,7 +1536,7 @@ int cmd_main(int argc, const char **argv)
if (argc)
usage_with_options(imap_send_usage, imap_send_options);
-#ifndef USE_CURL_FOR_IMAP_SEND
+#if !USE_CURL_FOR_IMAP_SEND
if (use_curl) {
warning("--curl not supported in this build");
use_curl = 0;
@@ -1580,7 +1585,7 @@ int cmd_main(int argc, const char **argv)
if (server.tunnel)
return append_msgs_to_imap(&server, &all_msgs, total);
-#ifdef USE_CURL_FOR_IMAP_SEND
+#if USE_CURL_FOR_IMAP_SEND
if (use_curl)
return curl_append_msgs_to_imap(&server, &all_msgs, total);
#endif
--
2.38.2.109.g8b8c02ffae.agit.6.7.7.dev
^ permalink raw reply related
* [PATCH 1/2] Makefile: not use mismatched curl_config to check version
From: Jiang Xin @ 2023-02-01 11:31 UTC (permalink / raw)
To: Git List, Junio C Hamano, Bernhard Reiter, Remi Pommarel
Cc: Jiang Xin, Jiang Xin
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
We may install different versions of curl, E.g.:
* A system default curl, which version is below 7.34.0, is installed
in "/usr", and the "curl_config" program is located in "/usr/bin/".
* A higher version of curl is installed in "/opt/git/embedded/", and
the "curl_config" program is located in "/opt/git/embedded/bin/".
If we add the path "/opt/git/embedded/bin" in search PATH, and install
git using command "make && sudo make install", the source code may be
compiled twice.
This is because when we run "make" using normal user account, make will
call "/opt/git/embedded/bin/curl_config" to check curl version, and will
set variable USE_CURL_FOR_IMAP_SEND. But when we call "make install"
using root user's account, we call the system default version of
curl_config to check curl version, and will lead to a different
"GIT-CFLAGS" file, and will recompile all source code again.
Append "$(CURLDIR)/bin" before the "CURL_CONFIG" variable to use the
specific "curl_config" program we want to check curl version, we will
get the correct "CURL_CFLAGS" and "CURL_LDFLAGS" variables, and we can
also have a stable "GIT-CFLAGS" file to prevent recompile when running
"sudo make install".
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/Makefile b/Makefile
index 45bd6ac9c3..f4eaf22523 100644
--- a/Makefile
+++ b/Makefile
@@ -1597,6 +1597,7 @@ else
# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
CURL_CFLAGS = -I$(CURLDIR)/include
CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
+ CURL_CONFIG := $(CURLDIR)/bin/$(CURL_CONFIG)
else
CURL_CFLAGS =
CURL_LIBCURL =
--
2.38.2.109.g8b8c02ffae.agit.6.7.7.dev
^ permalink raw reply related
* Re: Git over HTTP; have flexible SASL authentication
From: Jeff King @ 2023-02-01 11:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Rick van Rein, git, Matthew John Cheetham
In-Reply-To: <xmqq7cx7aov7.fsf@gitster.g>
On Fri, Jan 27, 2023 at 09:06:36AM -0800, Junio C Hamano wrote:
> Rick van Rein <rick@openfortress.nl> writes:
>
> > Git providers are inventing proprietary extensions to HTTP authentication
> > for Git. It seems smarter to use SASL for this purpose, possibly allowing
> > the client a choice and authentication ringback to the client's own domain.
>
> To adopt things like this, the work to extend how to make extensible
> what is on WWW-Authenticate in the thread that contains this recent
> message https://lore.kernel.org/git/Y9LvFMzriAWUsS58@coredump.intra.peff.net/
> may be relevant, perhaps?
It's relevant, but I think there's a ways to go. That is just about
passing WWW-Authenticate headers to helpers, which can then try to make
sense of them. But Git would still only understand getting back a
username/password from the helper, and passing it along to curl. And
hopefully we'd do it all through curl's SASL support, and not invent our
own handling.
I'm not sure what all that might might look like. I'm sure Matthew has
probably thought about it, so I'll let him say something more
intelligent. :)
-Peff
^ permalink raw reply
* [PATCH] clean: flush after each line
From: Orgad Shaneh via GitGitGadget @ 2023-02-01 10:09 UTC (permalink / raw)
To: git; +Cc: Orgad Shaneh, Orgad Shaneh
From: Orgad Shaneh <orgads@gmail.com>
Some platforms don't automatically flush after \n, and this causes delay
of the output, and also sometimes incomplete file names appear until the
next chunk is flushed.
Reported here: https://github.com/git-for-windows/git/issues/3706
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
clean: flush after each line
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1447%2Forgads%2Fclean-flush-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1447/orgads/clean-flush-v1
Pull-Request: https://github.com/git/git/pull/1447
builtin/clean.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index b2701a28158..f3de8170f9a 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -270,8 +270,10 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
if (!*dir_gone && !quiet) {
int i;
- for (i = 0; i < dels.nr; i++)
+ for (i = 0; i < dels.nr; i++) {
printf(dry_run ? _(msg_would_remove) : _(msg_remove), dels.items[i].string);
+ fflush(stdout);
+ }
}
out:
strbuf_release(&realpath);
@@ -544,6 +546,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
clean_print_color(CLEAN_COLOR_ERROR);
printf(_("Huh (%s)?\n"), (*ptr)->buf);
clean_print_color(CLEAN_COLOR_RESET);
+ fflush(stdout);
continue;
}
base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473
--
gitgitgadget
^ permalink raw reply related
* Re: Stability of git-archive, breaking (?) the Github universe, and a possible solution
From: Ævar Arnfjörð Bjarmason @ 2023-02-01 9:40 UTC (permalink / raw)
To: brian m. carlson; +Cc: Konstantin Ryabitsev, Eli Schwartz, Git List
In-Reply-To: <Y9mXB1LaYSUJBlwF@tapette.crustytoothpaste.net>
On Tue, Jan 31 2023, brian m. carlson wrote:
> As far as whether other people want to implement consistent compression,
> they are welcome to also write a spec and implement it. I personally
> feel that's too hard to get right and am not planning on working on it.
"A spec" here seems like overkill to me, so far on that front we've been
shelling out to gzip(1), and the breakage/event that triggered this
thread is rectified by starting to do that again by default.
It means that someone writing a clean-room implementation of git would
likely run into the same issue, if they used e.g. the Go language and a
native Go implementation of deflate.
But so what? We don't need to make promises for all potential git
implementations, just this one. So we could add a blurb like this to the
docs:
As people have come to rely on the exact "deflate"
implementation "git archive" promises to invoke the system's
"gzip" binary by default, under the assumption that its output
is stable. If that's no longer the case you'll need to complain
to whoever maintains your local "gzip".
If we wanted to be even more helpful we could bunde and ship an old
version of GNU gzip with our sources, and either default to that, or
offer it as a "--stable" implementation of deflate.
That would be going above & beyond what's needed IMO, but still a lot
easier than the daunting task of writing a specification that exactly
described GNU gzip's current behavior, to the point where you could
clean-room implement it and be guaranteed byte-for-byte compatibility.
^ permalink raw reply
* [PATCH v2] credential: new attribute password_expiry_utc
From: M Hickford via GitGitGadget @ 2023-02-01 9:39 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Jeff King, Cheetham, Dennington, M Hickford,
M Hickford
In-Reply-To: <pull.1443.git.git.1674914650588.gitgitgadget@gmail.com>
From: M Hickford <mirth.hickford@gmail.com>
Some passwords have an expiry date known at generation. This may be
years away for a personal access token or hours for an OAuth access
token.
Currently the credential protocol has no expiry attribute. When multiple
helpers are configured, `credential fill` tries each helper in turn
until it has a username and password, returning early.
When a storage helper and a credential-generating helper are configured
together, the credential is necessarily stored without expiry, so
`credential fill` may later return an expired credential from storage.
```
[credential]
helper = storage # eg. cache or osxkeychain
helper = generate # eg. oauth
```
An improvement is to introduce a password expiry attribute to the
credential protocol. If the expiry date has passed, `credential fill`
ignores the password attribute, so subsequent helpers can generate a
fresh credential. This is backwards compatible -- no change in
behaviour with helpers that discard the expiry attribute.
Note that the expiry logic is entirely within the credential layer.
Compatible helpers store and retrieve the new attribute like any other.
This keeps the helper contract simple.
This patch adds support for the new attribute to cache.
Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16
Future ideas: make it possible for a storage helper to provide OAuth
refresh token to subsequent helpers.
https://github.com/gitgitgadget/git/pull/1394
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential: new attribute password_expiry_utc
Some passwords have an expiry date known at generation. This may be
years away for a personal access token or hours for an OAuth access
token.
Currently the credential protocol has no expiry attribute. When multiple
helpers are configured, credential fill tries each helper in turn until
it has a username and password, returning early.
When a storage helper and a credential-generating helper are configured
together, the credential is necessarily stored without expiry, so
credential fill may later return an expired credential from storage.
[credential]
helper = storage # eg. cache or osxkeychain
helper = generate # eg. oauth
An improvement is to introduce a password expiry attribute to the
credential protocol. If the password has expired, credential fill no
longer returns early, so subsequent helpers can generate a fresh
credential. This is backwards compatible -- no change in behaviour with
helpers that discard the expiry attribute.
Note that the expiry logic is entirely within the credential layer.
Compatible helpers store and retrieve the new attribute like any other.
This keeps the helper contract simple.
This patch adds support for the new attribute to cache.
Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16
Future ideas: make it possible for a storage helper to provide OAuth
refresh token to subsequent helpers.
https://github.com/gitgitgadget/git/pull/1394
Questions for reviewers:
* Does the behaviour implemented match the documentation? (I'm not
famiiliar with C)
* Any edge cases?
* How to test in t0300-credentials.sh ?
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v2
Pull-Request: https://github.com/git/git/pull/1443
Range-diff vs v1:
1: 184b0aa6514 ! 1: b9ee729ee4d credential: new attribute password_expiry_utc
@@ Metadata
## Commit message ##
credential: new attribute password_expiry_utc
- If password has expired, credential fill no longer returns early,
- so later helpers can generate a fresh credential. This is backwards
- compatible -- no change in behaviour with helpers that discard the
- expiry attribute. The expiry logic is entirely in the git credential
- layer; compatible helpers simply store and return the expiry
- attribute verbatim.
+ Some passwords have an expiry date known at generation. This may be
+ years away for a personal access token or hours for an OAuth access
+ token.
- Store new attribute in cache.
+ Currently the credential protocol has no expiry attribute. When multiple
+ helpers are configured, `credential fill` tries each helper in turn
+ until it has a username and password, returning early.
+
+ When a storage helper and a credential-generating helper are configured
+ together, the credential is necessarily stored without expiry, so
+ `credential fill` may later return an expired credential from storage.
+
+ ```
+ [credential]
+ helper = storage # eg. cache or osxkeychain
+ helper = generate # eg. oauth
+ ```
+
+ An improvement is to introduce a password expiry attribute to the
+ credential protocol. If the expiry date has passed, `credential fill`
+ ignores the password attribute, so subsequent helpers can generate a
+ fresh credential. This is backwards compatible -- no change in
+ behaviour with helpers that discard the expiry attribute.
+
+ Note that the expiry logic is entirely within the credential layer.
+ Compatible helpers store and retrieve the new attribute like any other.
+ This keeps the helper contract simple.
+
+ This patch adds support for the new attribute to cache.
+
+ Example usage in a credential-generating helper
+ https://github.com/hickford/git-credential-oauth/pull/16
+
+ Future ideas: make it possible for a storage helper to provide OAuth
+ refresh token to subsequent helpers.
+ https://github.com/gitgitgadget/git/pull/1394
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
@@ Documentation/git-credential.txt: Git understands the following attributes:
+`password_expiry_utc`::
+
-+ If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970.
++ If password is a personal access token or OAuth access token, it may have an
++ expiry date. When getting credentials from a helper, `git credential fill`
++ ignores the password attribute if the expiry date has passed. Storage
++ helpers should store this attribute if possible. Helpers should not
++ implement expiry logic themselves. Represented as Unix time UTC, seconds
++ since 1970.
+
`url`::
@@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE
if (e) {
fprintf(out, "username=%s\n", e->item.username);
fprintf(out, "password=%s\n", e->item.password);
-+ if (e->item.password_expiry_utc != 0) {
-+ fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc);
-+ }
++ if (e->item.password_expiry_utc != TIME_MAX)
++ fprintf(out, "password_expiry_utc=%"PRItime"\n",
++ e->item.password_expiry_utc);
}
}
else if (!strcmp(action.buf, "exit")) {
@@ credential.c
#include "prompt.h"
#include "sigchain.h"
#include "urlmatch.h"
-+#include <time.h>
++#include "git-compat-util.h"
void credential_init(struct credential *c)
{
-@@ credential.c: void credential_clear(struct credential *c)
- free(c->path);
- free(c->username);
- free(c->password);
-+ c->password_expiry_utc = 0;
- string_list_clear(&c->helpers, 0);
+@@ credential.c: static void credential_getpass(struct credential *c)
+ if (!c->username)
+ c->username = credential_ask_one("Username", c,
+ PROMPT_ASKPASS|PROMPT_ECHO);
+- if (!c->password)
++ if (!c->password || c->password_expiry_utc < time(NULL)) {
++ c->password_expiry_utc = TIME_MAX;
+ c->password = credential_ask_one("Password", c,
+ PROMPT_ASKPASS);
++ }
+ }
+
+ int credential_read(struct credential *c, FILE *fp)
+ {
+ struct strbuf line = STRBUF_INIT;
- credential_init(c);
++ int password_updated = 0;
++ timestamp_t this_password_expiry = TIME_MAX;
++
+ while (strbuf_getline(&line, fp) != EOF) {
+ char *key = line.buf;
+ char *value = strchr(key, '=');
+@@ credential.c: int credential_read(struct credential *c, FILE *fp)
+ } else if (!strcmp(key, "password")) {
+ free(c->password);
+ c->password = xstrdup(value);
++ password_updated = 1;
+ } else if (!strcmp(key, "protocol")) {
+ free(c->protocol);
+ c->protocol = xstrdup(value);
@@ credential.c: int credential_read(struct credential *c, FILE *fp)
} else if (!strcmp(key, "path")) {
free(c->path);
c->path = xstrdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
-+ // TODO: ignore if can't parse integer
-+ c->password_expiry_utc = atoi(value);
++ this_password_expiry = parse_timestamp(value, NULL, 10);
++ if (this_password_expiry == 0 || errno) {
++ this_password_expiry = TIME_MAX;
++ }
} else if (!strcmp(key, "url")) {
credential_from_url(c, value);
} else if (!strcmp(key, "quit")) {
- c->quit = !!git_config_bool("quit", value);
- }
-+
-+ // if expiry date has passed, ignore password and expiry fields
-+ if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {
-+ trace_printf(_("Password has expired.\n"));
-+ FREE_AND_NULL(c->username);
-+ FREE_AND_NULL(c->password);
-+ c->password_expiry_utc = 0;
-+ }
+@@ credential.c: int credential_read(struct credential *c, FILE *fp)
+ */
+ }
+
++ if (password_updated)
++ c->password_expiry_utc = this_password_expiry;
+
- /*
- * Ignore other lines; we don't know what they mean, but
- * this future-proofs us when later versions of git do
+ strbuf_release(&line);
+ return 0;
+ }
@@ credential.c: void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path, 0);
credential_write_item(fp, "username", c->username, 0);
credential_write_item(fp, "password", c->password, 0);
-+ if (c->password_expiry_utc != 0) {
-+ int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc);
-+ char* str = malloc( length + 1 );
-+ snprintf( str, length + 1, "%ld", c->password_expiry_utc );
-+ credential_write_item(fp, "password_expiry_utc", str, 0);
-+ free(str);
++ if (c->password_expiry_utc != TIME_MAX) {
++ char *s = xstrfmt("%"PRItime, c->password_expiry_utc);
++ credential_write_item(fp, "password_expiry_utc", s, 0);
++ free(s);
+ }
}
static int run_credential_helper(struct credential *c,
+@@ credential.c: void credential_fill(struct credential *c)
+
+ for (i = 0; i < c->helpers.nr; i++) {
+ credential_do(c, c->helpers.items[i].string, "get");
+- if (c->username && c->password)
++ if (c->username && c->password && time(NULL) < c->password_expiry_utc)
+ return;
+ if (c->quit)
+ die("credential helper '%s' told us to quit",
## credential.h ##
@@ credential.h: struct credential {
char *protocol;
char *host;
char *path;
-+ time_t password_expiry_utc;
++ timestamp_t password_expiry_utc;
};
#define CREDENTIAL_INIT { \
+ .helpers = STRING_LIST_INIT_DUP, \
++ .password_expiry_utc = TIME_MAX, \
+ }
+
+ /* Initialize a credential structure, setting all fields to empty. */
Documentation/git-credential.txt | 9 +++++++++
builtin/credential-cache--daemon.c | 3 +++
credential.c | 24 ++++++++++++++++++++++--
credential.h | 2 ++
4 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..667c4d80e26 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -144,6 +144,15 @@ Git understands the following attributes:
The credential's password, if we are asking it to be stored.
+`password_expiry_utc`::
+
+ If password is a personal access token or OAuth access token, it may have an
+ expiry date. When getting credentials from a helper, `git credential fill`
+ ignores the password attribute if the expiry date has passed. Storage
+ helpers should store this attribute if possible. Helpers should not
+ implement expiry logic themselves. Represented as Unix time UTC, seconds
+ since 1970.
+
`url`::
When this special attribute is read by `git credential`, the
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index f3c89831d4a..338058be7f9 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
if (e) {
fprintf(out, "username=%s\n", e->item.username);
fprintf(out, "password=%s\n", e->item.password);
+ if (e->item.password_expiry_utc != TIME_MAX)
+ fprintf(out, "password_expiry_utc=%"PRItime"\n",
+ e->item.password_expiry_utc);
}
}
else if (!strcmp(action.buf, "exit")) {
diff --git a/credential.c b/credential.c
index f6389a50684..354fa1652a9 100644
--- a/credential.c
+++ b/credential.c
@@ -7,6 +7,7 @@
#include "prompt.h"
#include "sigchain.h"
#include "urlmatch.h"
+#include "git-compat-util.h"
void credential_init(struct credential *c)
{
@@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)
if (!c->username)
c->username = credential_ask_one("Username", c,
PROMPT_ASKPASS|PROMPT_ECHO);
- if (!c->password)
+ if (!c->password || c->password_expiry_utc < time(NULL)) {
+ c->password_expiry_utc = TIME_MAX;
c->password = credential_ask_one("Password", c,
PROMPT_ASKPASS);
+ }
}
int credential_read(struct credential *c, FILE *fp)
{
struct strbuf line = STRBUF_INIT;
+ int password_updated = 0;
+ timestamp_t this_password_expiry = TIME_MAX;
+
while (strbuf_getline(&line, fp) != EOF) {
char *key = line.buf;
char *value = strchr(key, '=');
@@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)
} else if (!strcmp(key, "password")) {
free(c->password);
c->password = xstrdup(value);
+ password_updated = 1;
} else if (!strcmp(key, "protocol")) {
free(c->protocol);
c->protocol = xstrdup(value);
@@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)
} else if (!strcmp(key, "path")) {
free(c->path);
c->path = xstrdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
+ this_password_expiry = parse_timestamp(value, NULL, 10);
+ if (this_password_expiry == 0 || errno) {
+ this_password_expiry = TIME_MAX;
+ }
} else if (!strcmp(key, "url")) {
credential_from_url(c, value);
} else if (!strcmp(key, "quit")) {
@@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)
*/
}
+ if (password_updated)
+ c->password_expiry_utc = this_password_expiry;
+
strbuf_release(&line);
return 0;
}
@@ -269,6 +284,11 @@ void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path, 0);
credential_write_item(fp, "username", c->username, 0);
credential_write_item(fp, "password", c->password, 0);
+ if (c->password_expiry_utc != TIME_MAX) {
+ char *s = xstrfmt("%"PRItime, c->password_expiry_utc);
+ credential_write_item(fp, "password_expiry_utc", s, 0);
+ free(s);
+ }
}
static int run_credential_helper(struct credential *c,
@@ -342,7 +362,7 @@ void credential_fill(struct credential *c)
for (i = 0; i < c->helpers.nr; i++) {
credential_do(c, c->helpers.items[i].string, "get");
- if (c->username && c->password)
+ if (c->username && c->password && time(NULL) < c->password_expiry_utc)
return;
if (c->quit)
die("credential helper '%s' told us to quit",
diff --git a/credential.h b/credential.h
index f430e77fea4..935b28a70f1 100644
--- a/credential.h
+++ b/credential.h
@@ -126,10 +126,12 @@ struct credential {
char *protocol;
char *host;
char *path;
+ timestamp_t password_expiry_utc;
};
#define CREDENTIAL_INIT { \
.helpers = STRING_LIST_INIT_DUP, \
+ .password_expiry_utc = TIME_MAX, \
}
/* Initialize a credential structure, setting all fields to empty. */
base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68
--
gitgitgadget
^ permalink raw reply related
* pack-objects memory use observations [was: [PATCH] delta-islands: free island-related data after use]
From: Eric Wong @ 2023-02-01 9:20 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <Y3UvhsRC9uCXJJ8P@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Wed, Nov 16, 2022 at 10:50:13AM +0000, Eric Wong wrote:
> > Will try to hunt down more memory savings in the nearish future.
>
> Yes, you've probably noticed that pack-objects does not distinguish much
> between what is necessary for the various phases. A few obvious things
> to look at:
>
> 1. After the write phase, you can probably ditch the island bitmaps,
> too. In many repacks we're basically done then, but if you're
> generating bitmaps, that happens afterwards in the same process.
Also, island_marks oid_map gets pretty big itself (300M?), and
realloc gets painful when resizing a big khash especially on
non-GNU/Linux systems without MREMAP_MAYMOVE realloc. Currently
experimenting with tweaks to make oidtree handle kh_oid_map
functionality to avoid resizes...[1]
> 2. The object traversal for pack-objects is done in-process these
> days. But after it finishes, I suspect that we do not generally
> need those object structs anymore, because all of the book-keeping
> is done in the bit object_entry array in packing_data.
pdata->objects is 1.4G for me atm after many hours (still going).
I think packing_data could be split to avoid reallocs, but that
might need to touch a lot of code...
I need to get better data on my next attempts. I suspect gcc
-O2 is throwing off mwrap-perl[2]+addr2line and I need to
rebuild w/ -O0.
[1] WIP oidtree map, but I feel like I forgot C, again :<
diff --git a/delta-islands.c b/delta-islands.c
index 90c0d6958f..9e824d7a0d 100644
--- a/delta-islands.c
+++ b/delta-islands.c
@@ -18,11 +18,13 @@
#include "pack-objects.h"
#include "delta-islands.h"
#include "oid-array.h"
+#include "oidtree.h"
#include "config.h"
KHASH_INIT(str, const char *, void *, 1, kh_str_hash_func, kh_str_hash_equal)
-static kh_oid_map_t *island_marks;
+struct oidtree island_marks_storage;
+static struct oidtree *island_marks;
static unsigned island_counter;
static unsigned island_counter_core;
@@ -93,7 +95,7 @@ static int island_bitmap_get(struct island_bitmap *self, uint32_t i)
int in_same_island(const struct object_id *trg_oid, const struct object_id *src_oid)
{
- khiter_t trg_pos, src_pos;
+ struct island_bitmap *trg, *src;
/* If we aren't using islands, assume everything goes together. */
if (!island_marks)
@@ -103,37 +105,30 @@ int in_same_island(const struct object_id *trg_oid, const struct object_id *src_
* If we don't have a bitmap for the target, we can delta it
* against anything -- it's not an important object
*/
- trg_pos = kh_get_oid_map(island_marks, *trg_oid);
- if (trg_pos >= kh_end(island_marks))
+ trg = oidtree_get(island_marks, trg_oid);
+ if (!trg)
return 1;
/*
* if the source (our delta base) doesn't have a bitmap,
* we don't want to base any deltas on it!
*/
- src_pos = kh_get_oid_map(island_marks, *src_oid);
- if (src_pos >= kh_end(island_marks))
+ src = oidtree_get(island_marks, src_oid);
+ if (!src)
return 0;
- return island_bitmap_is_subset(kh_value(island_marks, trg_pos),
- kh_value(island_marks, src_pos));
+ return island_bitmap_is_subset(trg, src);
}
int island_delta_cmp(const struct object_id *a, const struct object_id *b)
{
- khiter_t a_pos, b_pos;
- struct island_bitmap *a_bitmap = NULL, *b_bitmap = NULL;
+ struct island_bitmap *a_bitmap, *b_bitmap;
if (!island_marks)
return 0;
- a_pos = kh_get_oid_map(island_marks, *a);
- if (a_pos < kh_end(island_marks))
- a_bitmap = kh_value(island_marks, a_pos);
-
- b_pos = kh_get_oid_map(island_marks, *b);
- if (b_pos < kh_end(island_marks))
- b_bitmap = kh_value(island_marks, b_pos);
+ a_bitmap = oidtree_get(island_marks, a);
+ b_bitmap = oidtree_get(island_marks, b);
if (a_bitmap) {
if (!b_bitmap || !island_bitmap_is_subset(a_bitmap, b_bitmap))
@@ -149,30 +144,28 @@ int island_delta_cmp(const struct object_id *a, const struct object_id *b)
static struct island_bitmap *create_or_get_island_marks(struct object *obj)
{
- khiter_t pos;
- int hash_ret;
+ void **val;
+ size_t n = sizeof(struct island_bitmap *);
- pos = kh_put_oid_map(island_marks, obj->oid, &hash_ret);
- if (hash_ret)
- kh_value(island_marks, pos) = island_bitmap_new(NULL);
+ if (oidtree_put(island_marks, &obj->oid, &val, n))
+ *val = island_bitmap_new(NULL);
- return kh_value(island_marks, pos);
+ return *val;
}
static void set_island_marks(struct object *obj, struct island_bitmap *marks)
{
struct island_bitmap *b;
- khiter_t pos;
- int hash_ret;
+ void **val;
+ size_t n = sizeof(struct island_bitmap *);
- pos = kh_put_oid_map(island_marks, obj->oid, &hash_ret);
- if (hash_ret) {
+ if (oidtree_put(island_marks, &obj->oid, &val, n)) {
/*
* We don't have one yet; make a copy-on-write of the
* parent.
*/
marks->refcount++;
- kh_value(island_marks, pos) = marks;
+ *val = marks;
return;
}
@@ -180,10 +173,10 @@ static void set_island_marks(struct object *obj, struct island_bitmap *marks)
* We do have it. Make sure we split any copy-on-write before
* updating.
*/
- b = kh_value(island_marks, pos);
+ b = *val;
if (b->refcount > 1) {
b->refcount--;
- b = kh_value(island_marks, pos) = island_bitmap_new(b);
+ *val = b = island_bitmap_new(b);
}
island_bitmap_or(b, marks);
}
@@ -275,14 +268,11 @@ void resolve_tree_islands(struct repository *r,
struct tree *tree;
struct tree_desc desc;
struct name_entry entry;
- khiter_t pos;
- pos = kh_get_oid_map(island_marks, ent->idx.oid);
- if (pos >= kh_end(island_marks))
+ root_marks = oidtree_get(island_marks, &ent->idx.oid);
+ if (!root_marks)
continue;
- root_marks = kh_value(island_marks, pos);
-
tree = lookup_tree(r, &ent->idx.oid);
if (!tree || parse_tree(tree) < 0)
die(_("bad tree object %s"), oid_to_hex(&ent->idx.oid));
@@ -485,7 +475,8 @@ void load_delta_islands(struct repository *r, int progress)
{
struct island_load_data ild = { 0 };
- island_marks = kh_init_oid_map();
+ oidtree_init(&island_marks_storage);
+ island_marks = &island_marks_storage;
git_config(island_config_callback, &ild);
ild.remote_islands = kh_init_str();
@@ -500,11 +491,11 @@ void load_delta_islands(struct repository *r, int progress)
void propagate_island_marks(struct commit *commit)
{
- khiter_t pos = kh_get_oid_map(island_marks, commit->object.oid);
+ struct island_bitmap *root_marks;
- if (pos < kh_end(island_marks)) {
+ root_marks = oidtree_get(island_marks, &commit->object.oid);
+ if (root_marks) {
struct commit_list *p;
- struct island_bitmap *root_marks = kh_value(island_marks, pos);
parse_commit(commit);
set_island_marks(&get_commit_tree(commit)->object, root_marks);
@@ -522,16 +513,13 @@ int compute_pack_layers(struct packing_data *to_pack)
for (i = 0; i < to_pack->nr_objects; ++i) {
struct object_entry *entry = &to_pack->objects[i];
- khiter_t pos = kh_get_oid_map(island_marks, entry->idx.oid);
+ struct island_bitmap *bitmap;
oe_set_layer(to_pack, entry, 1);
- if (pos < kh_end(island_marks)) {
- struct island_bitmap *bitmap = kh_value(island_marks, pos);
-
- if (island_bitmap_get(bitmap, island_counter_core))
- oe_set_layer(to_pack, entry, 0);
- }
+ bitmap = oidtree_get(island_marks, &entry->idx.oid);
+ if (bitmap && island_bitmap_get(bitmap, island_counter_core))
+ oe_set_layer(to_pack, entry, 0);
}
return 2;
diff --git a/oidtree.c b/oidtree.c
index 0d39389bee..eb7e76335e 100644
--- a/oidtree.c
+++ b/oidtree.c
@@ -28,15 +28,16 @@ void oidtree_clear(struct oidtree *ot)
}
}
-void oidtree_insert(struct oidtree *ot, const struct object_id *oid)
+static void **oidtree_insert3(struct oidtree *ot, const struct object_id *oid,
+ size_t extra)
{
struct cb_node *on;
struct object_id k;
if (!oid->algo)
- BUG("oidtree_insert requires oid->algo");
+ BUG("oidtree insertion requires oid->algo");
- on = mem_pool_alloc(&ot->mem_pool, sizeof(*on) + sizeof(*oid));
+ on = mem_pool_alloc(&ot->mem_pool, sizeof(*on) + sizeof(*oid) + extra);
/*
* Clear the padding and copy the result in separate steps to
@@ -45,19 +46,22 @@ void oidtree_insert(struct oidtree *ot, const struct object_id *oid)
oidcpy_with_padding(&k, oid);
memcpy(on->k, &k, sizeof(k));
- /*
- * n.b. Current callers won't get us duplicates, here. If a
- * future caller causes duplicates, there'll be a a small leak
- * that won't be freed until oidtree_clear. Currently it's not
- * worth maintaining a free list
- */
- cb_insert(&ot->tree, on, sizeof(*oid));
+ if (!cb_insert(&ot->tree, on, sizeof(*oid)))
+ return (void **)(on->k + sizeof(k)); /* success */
+
+ warning("oidtree leak (check contains/get before insert/put)");
+ return NULL;
}
+void oidtree_insert(struct oidtree *ot, const struct object_id *oid)
+{
+ (void)oidtree_insert3(ot, oid, 0);
+}
-int oidtree_contains(struct oidtree *ot, const struct object_id *oid)
+static void **oidtree_find(struct oidtree *ot, const struct object_id *oid)
{
struct object_id k;
+ struct cb_node *on;
size_t klen = sizeof(k);
oidcpy_with_padding(&k, oid);
@@ -69,7 +73,31 @@ int oidtree_contains(struct oidtree *ot, const struct object_id *oid)
klen += BUILD_ASSERT_OR_ZERO(offsetof(struct object_id, hash) <
offsetof(struct object_id, algo));
- return cb_lookup(&ot->tree, (const uint8_t *)&k, klen) ? 1 : 0;
+ on = cb_lookup(&ot->tree, (const uint8_t *)&k, klen);
+ return on ? (void **)(on->k + sizeof(k)) : NULL;
+}
+
+int oidtree_put(struct oidtree *ot, const struct object_id *oid,
+ void ***p, size_t n)
+{
+ *p = oidtree_find(ot, oid);
+ if (*p)
+ return 0;
+
+ *p = oidtree_insert3(ot, oid, n);
+ assert(*p);
+ return 1;
+}
+
+void *oidtree_get(struct oidtree *ot, const struct object_id *oid)
+{
+ void **p = oidtree_find(ot, oid);
+ return p ? *p : NULL;
+}
+
+int oidtree_contains(struct oidtree *ot, const struct object_id *oid)
+{
+ return oidtree_find(ot, oid) ? 1 : 0;
}
static enum cb_next iter(struct cb_node *n, void *arg)
diff --git a/oidtree.h b/oidtree.h
index 77898f510a..2f6e6f1beb 100644
--- a/oidtree.h
+++ b/oidtree.h
@@ -12,6 +12,8 @@ struct oidtree {
void oidtree_init(struct oidtree *);
void oidtree_clear(struct oidtree *);
+
+/* oid_set-like API */
void oidtree_insert(struct oidtree *, const struct object_id *);
int oidtree_contains(struct oidtree *, const struct object_id *);
@@ -19,4 +21,17 @@ typedef enum cb_next (*oidtree_iter)(const struct object_id *, void *data);
void oidtree_each(struct oidtree *, const struct object_id *,
size_t oidhexsz, oidtree_iter, void *data);
+/* oid_map-like API */
+
+/* returns a pointer to the data payload associated with object_id */
+void *oidtree_get(struct oidtree *, const struct object_id *);
+
+/*
+ * points @p to the destination of the value
+ * @n must be consistent for the entire oidtree
+ * returns true if a new oidtree node was created,
+ * returns false if reusing an existing oidtree node
+ */
+int oidtree_put(struct oidtree *, const struct object_id *,
+ void ***p, size_t n);
#endif /* OIDTREE_H */
[2] https://80x24.org/mwrap-perl.git
# after install, run gc under mwrap-perl with backtrace 10
MWRAP=socket_dir:/tmp/mwrap,bt:10 mwrap-perl git gc
# recommended: use GNU addr2line 2.39+ (Aug 2022) for +OFFSET decoding
# start HTTP reverse proxy
ADDR2LINE='/path/to/addr2line -p -f -i' \
mwrap-rproxy --socket-dir=/tmp/mwrap
# the per-PID each/2000 URLs can get really expensive for browsers
# even w3m struggles:
w3m http://0:5000/ # follow per-PID links
^ permalink raw reply related
* Re: [PATCH] credential: new attribute password_expiry_utc
From: M Hickford @ 2023-02-01 8:29 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: M Hickford via GitGitGadget, git, M Hickford
In-Reply-To: <xmqqpmax5c4v.fsf@gitster.g>
On Sun, 29 Jan 2023 at 20:17, Junio C Hamano <gitster@pobox.com> wrote:
>
> "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > If password has expired, credential fill no longer returns early,
> > so later helpers can generate a fresh credential. This is backwards
> > compatible -- no change in behaviour with helpers that discard the
> > expiry attribute. The expiry logic is entirely in the git credential
> > layer; compatible helpers simply store and return the expiry
> > attribute verbatim.
> >
> > Store new attribute in cache.
>
> It is unclear what you are describing in the above. The current
> behaviour without the patch? The behaviour of the code if this
> patch gets applied? Write it in such a way that it is clear why
> the patch is a good idea, not just "this would not hurt because it
> is backwards compatible".
>
> The usual way to do so is to sell your change in this order:
>
> - Give background information to help readers understand what you
> are going to write in the following explanation.
>
> - Describe the current behaviour without any change to the code;
>
> - Present a situation where the current code results in an
> undesirable outcome. What exactly happens, what visible effect it
> has to the user, how the code could do better to help the user?
>
> - Propose an updated behaviour that would behave better in the
> above sample situation presented.
>
Thanks for the guidance. Writing a better commit message clarified my
own thoughts.
> Curiously, what you wrote below the "---" line, that will not be
> part of the log message, looks to be organized better than the
> above. The first paragraph (except for the "Add ...") prepares the
> readers, It is still unclear if the second paragraph "when expired"
> describes what happens with the current code (i.e. highlighting why
> a change is needed) or what you want to happen with the patch, but
> the paragraph should first explain the problem in the current
> behaviour to motivate readers to learn why the updated code would
> lead to a better world. And follow that with the behaviour of the
> updated code and its effect (e.g. "without first trying a credential
> that is stale and see it fail before asking to reauthenticate, such
> a known-to-be-stale credential gets discarded automatically").
>
>
> > +`password_expiry_utc`::
> > +
> > + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970.
> > +
>
> A overly long line. Please follow Documentation/CodingGuidelines
> and Documentation/SubmittingPatches
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index f3c89831d4a..5cb8a186b45 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
> > if (e) {
> > fprintf(out, "username=%s\n", e->item.username);
> > fprintf(out, "password=%s\n", e->item.password);
> > + if (e->item.password_expiry_utc != 0) {
> > + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc);
> > + }
>
> Style (multiple issues, check CodingGuidelines):
>
> if (e->item.password_expiry_utc)
> fprintf(out, "... overly long format template ...",
> e->item.password_expiry_utc);
>
> * Using integral value or pointer value as a truth value does not
> require an explicit comparison with 0;
>
> * A single-statement block does not need {} around it;
>
> * Overly long line should be folded, with properly indented.
>
> > diff --git a/credential.c b/credential.c
> > index f6389a50684..0a3a9cbf0a2 100644
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -7,6 +7,7 @@
> > #include "prompt.h"
> > #include "sigchain.h"
> > #include "urlmatch.h"
> > +#include <time.h>
>
> Don't include system headers directly; often git-compat-util.h
> already has it, and if not, we need to find the right place to have
> it in git-compat-util.h file, as there are platforms that are
> finicky in inclusion order of the header files and definition of
> feature macros.
>
> > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c)
> > free(c->path);
> > free(c->username);
> > free(c->password);
> > + c->password_expiry_utc = 0;
>
> Not a huge deal, but if the rule is "an credential with expiry
> timestamp that is too old behaves as if it no longer exists or is
> valid", then a large integer, not zero, may serve as a better
> sentinel value for "this entry never expires". Instead of having to
> do
>
> if (expiry && expiry < time()) {
> ... expired ...
> }
>
> you can just do
>
> if (expiry < time()) {
> ... expired ...
> }
>
> and that would be simpler to understand for human readers, too.
>
> > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)
> > } else if (!strcmp(key, "path")) {
> > free(c->path);
> > c->path = xstrdup(value);
> > + } else if (!strcmp(key, "password_expiry_utc")) {
> > + // TODO: ignore if can't parse integer
>
> Do not use // comment. /* Our single-liner comment reads like this */
>
> > + c->password_expiry_utc = atoi(value);
>
> Don't use atoi(); make sure value is not followed by a non-number,
> e.g.
>
> const char *value = "43q";
> printf("%d<%s>\n", atoi(value), value);
>
> would give you 43<43q>, but you want to reject and silently ignore
> such an expiry timestamp.
>
> > + // if expiry date has passed, ignore password and expiry fields
>
> Ditto, but if you used a large value as sentinel for "never expires"
> and wrote it like this
>
> if (c->password_expiry_utc < time(NULL)) {
>
> then it is clear enough that you do not even need such a comment.
> The expression itself makes it clear what is going on (i.e. the
> current time comes later than the expiry_utc value on the number
> line hence it appears on the right to it, clearly showing that it
> has passed the threshold).
>
> > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {
> > + trace_printf(_("Password has expired.\n"));
> > + FREE_AND_NULL(c->username);
> > + FREE_AND_NULL(c->password);
> > + c->password_expiry_utc = 0;
> > + }
> > +
^ permalink raw reply
* [PATCH] git-gui: fix inability to quit after closing another instance
From: Orgad Shaneh via GitGitGadget @ 2023-02-01 8:26 UTC (permalink / raw)
To: git; +Cc: Orgad Shaneh, Orgad Shaneh
From: Orgad Shaneh <orgads@gmail.com>
If you open 2 git gui instances in the same directory, then close one
of them and try to close the other, an error message pops up, saying:
'error renaming ".git/GITGUI_BCK": no such file or directory', and it
is no longer possible to close the window ever.
Fix by catching this error, and proceeding even if the file no longer
exists.
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
git-gui: fix inability to quit after closing another instance
If you open 2 git gui instances in the same directory, then close one of
them and try to close the other, an error message pops up, saying:
'error renaming ".git/GITGUI_BCK": no such file or directory', and it is
no longer possible to close the window ever.
Fix by catching this error, and proceeding even if the file no longer
exists.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1446%2Forgads%2Fgit-gui-no-quit-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1446/orgads/git-gui-no-quit-v1
Pull-Request: https://github.com/git/git/pull/1446
git-gui/git-gui.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 201524c34ed..b00ee691e3b 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2307,7 +2307,7 @@ proc do_quit {{rc {1}}} {
#
set save [gitdir GITGUI_MSG]
if {$GITGUI_BCK_exists && ![$ui_comm edit modified]} {
- file rename -force [gitdir GITGUI_BCK] $save
+ catch { file rename -force [gitdir GITGUI_BCK] $save }
set GITGUI_BCK_exists 0
} elseif {[$ui_comm edit modified]} {
set msg [string trim [$ui_comm get 0.0 end]]
base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'
From: Teng Long @ 2023-02-01 8:08 UTC (permalink / raw)
To: sunshine; +Cc: avarab, dyroneteng, git, tenglong.tl
In-Reply-To: <CAPig+cTVMGcRQqntPuwtV9x=WxUh-37Uy5V95A3F4ekeQgFq2g@mail.gmail.com>
On Mon, 30 Jan 2023 00:38:02 -0500 Eric Sunshine <sunshine@sunshineco.com>
wrote:
> It's unfortunate, perhaps, that the git-notes command-set is not
> orthogonal, but it's another case of historic accretion. According to
> the history, as originally implemented, git-notes only supported two
> commands, "edit" and "show", and "edit" was responsible for _all_
> mutation operations, including deletion. git-notes only grew a more
> complete set of commands a couple years later.
>
> At any rate, even though the original behaviors may not make as much
> sense these days now that git-notes has a more complete set of
> commands, we still need to be careful not to break existing workflows
> and scripts (tooling). That's why it would be nice to beef up the
> tests so that the existing behaviors don't get broken by changes such
> as this patch wants to make. But, as mentioned earlier, the additional
> tests probably shouldn't be part of this series, but rather can be
> done as a separate series (by someone; it doesn't have to be you).
Make sense by your detailed explanation.
Thanks.
^ permalink raw reply
* Re: [PATCH] docs: document zero bits in index "mode"
From: Glen Choo @ 2023-02-01 2:46 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20230201024041.29401-1-chooglen@google.com>
Glen Choo <chooglen@google.com> writes:
>> The existing explanation starts with "32-bit mode,
>> split into (high to low bits)", followed by "4-bit object type", as
>> if the "4-bit object type" occupies bits 29-32, which is not quite
>> what we want to say.
>
> If I am understanding you correctly (which I'm not sure, since I am
> honestly clueless about bit numbering and big endianness and whatnot),
> you're saying that highest 16 bits are zero, not the lowest? If so, then
> I genuinely made that mistake, hah.
>
> (We're storing 16 bits as bigendian 32 bits, so they would occupy the
> lower 16 bits, right..?)
>
> Perhaps something like this would be better. I took the "unused, must be
> zero" phrasing from elsewhere in the doc. And if I am completely
> off-base, feel free to patch it without waiting for my reroll if you
> think that's easier.
Whoops, this was meant to be a reply to
https://lore.kernel.org/git/xmqqmt5yy08d.fsf@gitster.g. That's what I
get for trying to write the In-Reply-To by hand.
^ permalink raw reply
* [PATCH] docs: document zero bits in index "mode"
From: Glen Choo @ 2023-02-01 2:40 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Junio C Hamano
Documentation/gitformat-index.txt describes the "mode" as 32 bits, but
only documents 16 bits. Document the missing 16 bits and specify that
'unused' bits must be zero.
Signed-off-by: Glen Choo <chooglen@google.com>
---
> The existing explanation starts with "32-bit mode,
> split into (high to low bits)", followed by "4-bit object type", as
> if the "4-bit object type" occupies bits 29-32, which is not quite
> what we want to say.
If I am understanding you correctly (which I'm not sure, since I am
honestly clueless about bit numbering and big endianness and whatnot),
you're saying that highest 16 bits are zero, not the lowest? If so, then
I genuinely made that mistake, hah.
(We're storing 16 bits as bigendian 32 bits, so they would occupy the
lower 16 bits, right..?)
Perhaps something like this would be better. I took the "unused, must be
zero" phrasing from elsewhere in the doc. And if I am completely
off-base, feel free to patch it without waiting for my reroll if you
think that's easier.
Documentation/gitformat-index.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/gitformat-index.txt b/Documentation/gitformat-index.txt
index 015cb21bdc..0773e5c380 100644
--- a/Documentation/gitformat-index.txt
+++ b/Documentation/gitformat-index.txt
@@ -83,11 +83,13 @@ Git index format
32-bit mode, split into (high to low bits)
+ 16-bit unused, must be zero
+
4-bit object type
valid values in binary are 1000 (regular file), 1010 (symbolic link)
and 1110 (gitlink)
- 3-bit unused
+ 3-bit unused, must be zero
9-bit unix permission. Only 0755 and 0644 are valid for regular files.
Symbolic links and gitlinks have value 0 in this field.
--
2.39.1.456.gfc5497dd1b-goog
^ permalink raw reply related
* Re: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script
From: Andrei Rybak @ 2023-02-01 2:21 UTC (permalink / raw)
To: Shuqi Liang, git
In-Reply-To: <20230131224929.2018546-1-cheskaqiqi@gmail.com>
Hi Shuqi Liang,
> Subject: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script
For patches that change a single test, the subject line can include just
the "t" and the number. The part after the colon should start with a
lowercase letter. Something like
t4113: modernize test style
On 31/01/2023 23:49, Shuqi Liang wrote:
>
> I cleaned up some old style in test script.
Commit message should start with description of the existing problem
in present tense, something like:
Test scripts in file t4113-apply-ending.sh are written in old style,
where the test_expect_success command and test title are written on
separate lines ...
Then changes should be described using imperative mood, as if you are
giving commands to the codebase. See section "[[describe-changes]]"
in "Documentation/SubmittingPatches" for details.
You can also find examples of existing commit messages for similar
changes:
$ git log --no-merges --grep='modernize' -- t
>
> for example :
>
> * old style:
>
> test_expect_success \
> 'title' \
> 'body line 1 &&
> body line 2'
>
> should become:
>
> test_expect_success 'title' '
> body line 1 &&
> body line 2
> '
>
>
>
>
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
> Hi,I'm Shuqi Liang.a junior student majors in Computer Science at
> University of Western Ontario.
Welcome!
> t/t4113-apply-ending.sh | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> index 66fa51591e..aa57895b22 100755
> --- a/t/t4113-apply-ending.sh
> +++ b/t/t4113-apply-ending.sh
> @@ -24,13 +24,14 @@ echo 'a' >file
> echo 'b' >>file
> echo 'c' >>file
A "modern" test could also do such preparation for test files as
part of its "setup" step. This could its own patch in the same
series, separate from style changes.
In case of t4113, files "test-patch" and "file" are created twice.
The second creation of the files could be either its own step
'setup for apply at the beginning', or incorporated into the step
'apply at the beginning'.
Section "Recommended style" in t/README also has some notes about
how heredocs should be indented.
>
> -test_expect_success setup \
> - 'git update-index --add file'
> -
> +test_expect_success setup '
> + git update-index --add file
> +'
While changing the quoting around test tiles and commands, the
indentation with spaces could also be changed to TABs.
> # test
If the setup code on top level of the file is moved into test
steps, this comment and the "# setup" comment at line 11 will
become unnecessary.
>
> -test_expect_success 'apply at the end' \
> - 'test_must_fail git apply --index test-patch'
> +test_expect_success 'apply at the end' '
> + test_must_fail git apply --index test-patch
> +'
>
> cat >test-patch <<\EOF
> diff a/file b/file
> @@ -47,7 +48,8 @@ b
> c'
> git update-index file
>
> -test_expect_success 'apply at the beginning' \
> - 'test_must_fail git apply --index test-patch'
> +test_expect_success 'apply at the beginning' '
> + test_must_fail git apply --index test-patch
> +'
>
> test_done
Thanks.
^ permalink raw reply
* Re: Inconsistency between git-log documentation and behavior regarding default date format.
From: Andrei Rybak @ 2023-02-01 1:33 UTC (permalink / raw)
To: Rafael Dulfer; +Cc: Jeff King, git
In-Reply-To: <9c3428f6-a254-13b4-046d-6e20ef602aef@dulfer.be>
On 24/01/2023 16:09, Rafael Dulfer wrote:
> The Git documentation
> <https://www.git-scm.com/docs/git-log#Documentation/git-log.txt---dateltformatgt> states that if no date format is given (or --date=default is given), the date format is similar to rfc2822 except of a few exceptions, of which those listed are:
>
> * There is no comma after the day-of-week
> * The time zone is omitted when the local time zone is used
>
> However, if we were to compare the two date formats, you can see another
> difference:
>
> git log --default -> Tue Jan 24 11:03:47 2023 +0100
> git log --rfc -> Tue, 24 Jan 2023 11:03:47 +0100
It seems that options in these samples were mistyped. They are missing
the "--date=" part:
$ git log -1 --date=default master | grep 'Date:'
Date: Sat Jan 21 16:35:14 2023 -0800
$ git log -1 --date=rfc2822 master | grep 'Date:'
Date: Sat, 21 Jan 2023 16:35:14 -0800
>
> With the default, the month and day-of-month are switched around. From
> my own quick investigation, this behavior occurs because of the
> statement found at date.c#L266
> <https://github.com/git/git/blob/56c8fb1e95377900ec9d53c07886022af0a5d3c2/date.c#L266> wherein the month is inserted before the day-of-month. I am unsure which behavior is exactly intended and whether this discrepancy was known, but it would probably be a good idea to have a note of it in the documentation.
Indeed, the description of the option (which comes from
Documentation/rev-list-options.txt) doesn't describe all differences
between --date=default and --date=rfc2822.
A fuller list could be:
* There is no comma after the day-of-week
* The time zone is omitted when the local time zone is used
* Day-of-month and month are switched around
* Time-of-day and the year are switched around
CC'ing Peff, who wrote the list of exceptions in add00ba2de (date: make
"local" orthogonal to date format, 2015-09-03).
^ permalink raw reply
* Re: Stability of git-archive, breaking (?) the Github universe, and a possible solution
From: brian m. carlson @ 2023-02-01 1:33 UTC (permalink / raw)
To: Eli Schwartz; +Cc: Git List
In-Reply-To: <6fc8e122-a190-c291-c347-258a5a2ad9c9@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 11930 bytes --]
On 2023-01-31 at 15:56:52, Eli Schwartz wrote:
> On 1/31/23 4:54 AM, brian m. carlson wrote:
> > Part of the reason I think this is valuable is that once SHA-1 and
> > SHA-256 interoperability is present, git archive will change the
> > contents of the archive format, since it will embed a SHA-256 hash into
> > the file instead of a SHA-1 hash, since that's what's in the repository.
> > Thus, we can't produce an archive that's deterministic in the face of
> > SHA-1/SHA-256 interoperability concerns, and we need to create a new
> > format that doesn't contain that data embedded in it.
>
>
> I assume that whatever the reason for originally embedding the OID into
> the file is still an applicable reason even if a new PAX format is
> established for the use of git-archive.
>
> It may not be a great reason -- I don't know. Perhaps there's an
> argument to remove it. But can't that be done irrespective of
> standardizing the PAX format?
>
> ...
>
> I'm not deeply knowledgeable about the SHA-256 transition work -- or
> knowledgeable at all about it, frankly. (Also my understanding was it
> seems to have stalled as discussed in https://lwn.net/Articles/898522/
> -- I understand that you're still enthusiastic about the work? But that
> doesn't really answer "is there a timeframe for that to ever happen".)
The timeframe is when my employer pays me to work on it. Right now,
I've implemented functional SHA-256 repositories but am currently a bit
on the way to burnout and am very selective about what things I'm doing
outside of work. My hope is that my employer will find time for me to
work on the interop stuff soon, but I'm not at liberty to discuss this
more in depth at the moment.
> But I sort of assumed that the transition work would already have to
> embed a fair bit of information into the repository about the whole
> process? Would it not be possible to determine whether a given tag
> started life as SHA-1 or SHA-256? Maybe even just a date when the
> repository was converted to work with both, and embed the OID based on
> whether the tag is tagging contents that were created after that conversion?
It's designed such that the two objects are completely interoperable and
can be accessed by either name, depending on how the repository is
configured locally. There may be a signature for one algorithm, both,
or neither, so it's hard to say definitively what version it's created
with. That is completely intentional since the goal is to transition
seamlessly from one to another at any point depending on the preferences
of the owner of the local repository.
> > Having said that, I don't think this should be based on the timestamp of
> > the file, since that means that two otherwise identical archives
> > differing in timestamp aren't ever going to be the same, and we do see
> > people who import or vendor other projects.
>
>
> The timestamp of the output file? Surely not. But I only suggested the
> timestamp of the commit/tag metadata that git-archive is asked to
> produce output for. And we would need that in order to solve the problem
> that reproducible github API archive endpoints poses.
I think it would simply be easier to say, "This is the command-line
option that implements canonical tar version 1." If you want a
reproducible archive, you use that command-line option, and your
uncompressed tar archive is reproducible. Otherwise, you get the same
guarantees on reproducibility that we've always provided, which is
absolutely none.
Using commit and tag metadata doesn't solve the problem of trees, which
would use the current timestamp. It's better to solve the problem in a
consistent way, which would mean embedding a fixed timestamp (probably
the Epoch) into those tree tarballs.
In my view, using the commit or tag timestamp is very risky, because it
changes the behaviour at some point in the future without notifying
people. If we produce a tar archive that isn't readable by FooZip, say,
then nobody will realize that until we actually start producing them,
several months after the release. And, I should point out, this still
poses problems for GitHub and other forges, because GitHub doesn't run
the latest release right away; we usually trail a version or two. So
using the commit or tag timestamp might mean that on an upgrade,
suddenly the behaviour changes because the new version has a change
(which was scheduled to have occurred in the past) but the old version
doesn't.
In addition, the one guarantee we've given with archives in the past is
that the same version of Git with the same input (flags, repository,
etc.) will produce deterministic results (that is, the same output), and
I think we're likely to run afoul of that with a timestamp-based
approach. I don't want the archive to suddenly be different because I
happened to do "git commit --amend" to update just a commit message and
we happened to cross that timestamp threshold.
> I'm not sure what the "import or vendor other projects" angle here
> means. Do you mean people who copy a directory of files into their
> project? Who expects this to be the same to begin with? And doesn't
> embedding the OID kill this idea, since the entire point of git commit
> sha's is that you shouldn't (it should be prohibitively unrealistic to)
> be able to produce the same one twice in different contexts?
We have people who import the entirety of Chromium into a project at
one time to work on a browser-based project.
> I have never said to myself "ah yes, I really would like to be able to
> download a git auto-generated tarball for project A, and compare its
> hash to the tarball for project B, and have them compare identical even
> though they are different projects with different commits". IMHO this
> isn't an interesting problem to solve -- the interesting problem to
> solve is that a single absolute URL to a downloadable file should be
> able to offer documented guarantees that it will always be the same
> file, even though it is generated on the fly.
I do think having identical output for identical contents is very
valuable. If our goal is reproducible output, we should endeavour to
produce identical output for identical input. What we're specifically
trying to move away from is varying output based on the same input.
> I do not think it is realistic or reasonable for people to implement
> compression using intentionally incompatible replacements for gzip and
> expect interoperability of any sort.
I disagree completely. The gzip and zlib formats are documented in RFCs
and have been since 1996. There are already at least a half-dozen
interoperable implementations, including zlib, gzip, pigz, Go's standard
library, miniz_oxide, and the Windows archiver. I'm sure if I searched
I could find at least half a dozen more.
> I also don't think people *have* to implement compression in rust using
> zlib, but if they are going to make a git-alike that produces archives,
> it would be worth it for them to write whatever memory-safe rust is
> necessary to memory-safely produce the same output stream of bytes. It's
> no less feasible than making sure that busybox gzip and GNU gzip produce
> the same output, surely.
I don't agree at all. The Go standard library couldn't achieve that,
because busybox and gzip are GPL and doing that would almost certainly
require looking at the code, which would require the Go standard library
to be GPL as well. The same thing goes for zlib, which is permissively
licensed, and which is clearly the obvious choice if we had to settle on
a standard, since it's a shared library.
That also ignores tools like pigz which provide parallel compression and
can provide an order of magnitude performance increase, but which won't
provide an identical byte stream. Why should we require people to use a
single core if they have a very large archive that could compress
several times as fast with a parallel operation?
My goal is to produce tar archives that are interoperable based on a
spec. That spec would be implementable by Git, GNU tar, libarchive, or
anyone else, by reading the spec and following it. That's very
different from saying, "Well, just make your program do exactly the same
thing as this other one without sharing any code." If you want to write
a spec for canonical gzip, I'm interested in reading it, but I think
it's practically going to be difficult to achieve.
> > That may mean that it's important for people to actually decompress the
> > archive before checking hashes if they want deterministic behaviour, and
> > I'm okay with that. You already have to do that if you're verifying the
> > signature on Git tarballs, since only the uncompressed tar archive is
> > signed, so I don't think this is out of the question.
>
>
> This is a very kernel.org-centric view of things, I think. I have rarely
> seen PGP signatures applied to the uncompressed tar except in that
> context. The vast majority of tarballs with signatures have signed a
> single compressed tarball and don't concern themselves with, say,
> providing a rotating backdated changeable list of compression formats
> with a single signature covering all of them.
Sure, and that's a valid approach if you have a consistent, persistent
tarball. However, Git does not persist data forever in tarballs, and
people want to use different versions to get the same data, which is a
new guarantee that we'd be providing. That is an easy guarantee to
provide with tar, but not an easy guarantee to provide with the gzip
format, as we've all just seen.
> >From experience, I can say that this needs to be selected on a
> per-tarball basis. Since signature files have filenames, we can match
> their stems and given foo.tar.asc and foo.tar.gz, check the signature of
> the output of gzip -dc < foo.tar.gz, but given foo.tar.gz.asc and
> foo.tar.gz, simply check the signature of the original foo.tar.gz.
>
> This doesn't really work for checksums, because you need to settle on
> one or the other everywhere or else embed decompression information into
> your checksum metadata field.
I don't think that's absolutely required. You need to know how to
decompress the archive, and you can have a hash for the tarball before
decompression or after decompression, as well as possibly needing to
deal with multiple different hash algorithms. I've implemented this
myself when I was a vendor of Git and lots of other software, and we
would take the hash of the compressed or decompressed archive as shipped
by the vendor and verify it, as long as the hash was sufficiently
strong.
> And for tarballs that are generated once and uploaded to ftp storage,
> not repeatedly generated on the fly, we know the checksum will never
> legitimately change, so we *want* to hash the compressed file.
> Decompressing kernel.org tarballs in order to run PGP on them is *slow*.
> Although at least one can verify the checksums first without
> decompression, which is virtually guaranteed to catch invalid source
> code releases, so if you ever progress to the PGP verification stage
> it's unlikely to be wasted effort -- that tarball is definitely getting
> used to build something.
Sure, and if you want to generate tarballs once and upload them to
storage, go ahead. That's always an option. Even GitHub provides you
the option to do that with release assets if you want.
My proposal is to provide deterministic archives in a functionally and
practically achievable way with nothing more than a version of Git,
which I think we can do with tar, but not gzip. I'm happy to be proven
wrong if you can develop a spec for canonical gzip compression.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: Bug: Cloning git repositories behind a proxy using the git:// protocol broken since 2.32
From: Florian Bezdeka @ 2023-01-31 23:19 UTC (permalink / raw)
To: Jacob Keller
Cc: brian m. carlson, git@vger.kernel.org, gitster@pobox.com,
greg.pflaum@pnp-hcl.com, peff@peff.net
In-Reply-To: <CA+P7+xpgJKojMmcN9TuGDw8oduQSQk-5nUtsWc+4Seqa+eVDJQ@mail.gmail.com>
On Tue, 2023-01-31 at 12:31 -0800, Jacob Keller wrote:
> On Tue, Jan 31, 2023 at 4:17 AM Florian Bezdeka
> <florian.bezdeka@siemens.com> wrote:
> > Thanks for the super fast response, highly appreciated!
> >
> > I was able to get it running by switching to ncat using the --no-
> > shutdown option, but I failed to bring back socat support so far.
> >
> > For me this is still a regression. We have to change our
> > infrastructure/environment because we have a new requirement
> > (independent handling of stdin/out) after updating git now. I would
> > expect some noise from the yocto/OE community in the future where oe-
> > git-proxy is heavily used.
> >
> > I guess proxy support was forgotten when the referenced change was
> > made. Any chance we can avoid closing stdout when running "in proxy
> > mode" to restore backward compatibility?
> >
> > Thanks a lot!
>
> I had this issue in the past and i got it working with socat using "-t
> 10". You need a timeout that is larger than the keep alive value of 5
> seconds which is sent by the git protocol.
>
> Junio pointed out the excellent analysis from Peff regarding the
> situation and the fact that socat is wrong here.
Thanks for pointing me to the old discussion. I was quite sure that I'm
not the first one facing this problem but couldn't find something.
It might be that socat is doing something wrong. But git is the
component that triggers the problem. Did someone talk to the socat
maintainers yet?
Peff also mentioned that the half-duplex shutdown of the socket is
inconsistent between proxy and raw TCP git://. It seems still a valid
option to skip the half-shutdown for the git:// proxy scenario.
>
> What value of -t did you try?
I was playing with -t 10 and -t 60 so far. Both does not work for
cloning a kernel stable tree. I guess it's hard to find a value that
works under all circumstances as timings might be different depending
on server/network speed.
^ permalink raw reply
* [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script
From: Shuqi Liang @ 2023-01-31 22:49 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang
I cleaned up some old style in test script.
for example :
* old style:
test_expect_success \
'title' \
'body line 1 &&
body line 2'
should become:
test_expect_success 'title' '
body line 1 &&
body line 2
'
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
Hi,I'm Shuqi Liang.a junior student majors in Computer Science at University of Western Ontario.
This patch is the microproject I try to getting involved with the Git project.
I have read 'MyFirstContribution.txt', 'Hacking Git' and the book 《pro git》 ,and I know more about objects, references, packfile format, etc.
Over the coming period, I will delve into the source code and gain a deeper understanding and try to contribute more meaningful patch to the community.
t/t4113-apply-ending.sh | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..aa57895b22 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -24,13 +24,14 @@ echo 'a' >file
echo 'b' >>file
echo 'c' >>file
-test_expect_success setup \
- 'git update-index --add file'
-
+test_expect_success setup '
+ git update-index --add file
+'
# test
-test_expect_success 'apply at the end' \
- 'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the end' '
+ test_must_fail git apply --index test-patch
+'
cat >test-patch <<\EOF
diff a/file b/file
@@ -47,7 +48,8 @@ b
c'
git update-index file
-test_expect_success 'apply at the beginning' \
- 'test_must_fail git apply --index test-patch'
+test_expect_success 'apply at the beginning' '
+ test_must_fail git apply --index test-patch
+'
test_done
--
2.39.0
^ permalink raw reply related
* Re: Stability of git-archive, breaking (?) the Github universe, and a possible solution
From: brian m. carlson @ 2023-01-31 22:32 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Eli Schwartz, Git List
In-Reply-To: <20230131150555.ewiwsbczwep6ltbi@meerkat.local>
[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]
On 2023-01-31 at 15:05:55, Konstantin Ryabitsev wrote:
> On Tue, Jan 31, 2023 at 09:54:58AM +0000, brian m. carlson wrote:
> > I'm one of the GitHub employees who chimed in there, and I'm also a Git
> > contributor in my own time (and I am speaking here only in my personal
> > capacity, since this is a personal address). I made a change some years
> > back to the archive format to fix the permissions on pax headers when
> > extracted as files, and kernel.org was relying on that and broke. Linus
> > yelled at me because of that.
> >
> > Since then, I've been very opposed to us guaranteeing output format
> > consistency without explicitly doing so. I had sent some patches before
> > that I don't think ever got picked up that documented this explicitly.
> > I very much don't want people to come to rely on our behaviour unless we
> > explicitly guarantee it.
>
> I understand your position, but I also think it's one of those things that
> happen despite your best efforts to prevent it. :)
>
> May I suggest adding a "git-archive --stable" that offers this guarantee,
> simply as a matter of codifying the fact that the world has built
> infrastructure around git's repeatable output. Maybe just for .tar (and
> .tar.gz).
It is my intention to implement just .tar. That's my proposal: simply a
pax-based format that serializes in a consistent way according to a
predefined spec.
As far as whether other people want to implement consistent compression,
they are welcome to also write a spec and implement it. I personally
feel that's too hard to get right and am not planning on working on it.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH v3 00/11] Bundle URIs V: creationToken heuristic for incremental fetches
From: Junio C Hamano @ 2023-01-31 22:01 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, me, vdye, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <pull.1454.v3.git.1675171759.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This fifth part to the bundle URIs feature follows part IV (advertising via
> protocol v2) which recently merged to 'master', so this series is based on
> 'master'.
Thanks for a pleasant read. Will queue.
^ permalink raw reply
* Re: [PATCH v3 05/11] bundle-uri: parse bundle.<id>.creationToken values
From: Junio C Hamano @ 2023-01-31 21:22 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, me, vdye, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <ff629bc119b466dcd827f758b3d892fefd6a9703.1675171760.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> + if (info->creationToken)
> + fprintf(fp, "\tcreationToken = %"PRIu64"\n", info->creationToken);
So, a bundle info with .creationToken set to 0 signals that the
token is missing for that bundle. REMOTE_BUNDLE_INFO_INIT clears
all members, so unless we find a concrete value and assign to the
member, we'll have 0 in the member (and never a random value).
Very good.
We used to avoid camelCased variable names and member names and
instead prefer snake_case; I wonder if it is still the case.
Looking at bundle.h it still seems to be the case, and we may want
to match that.
> @@ -203,6 +206,13 @@ static int bundle_list_update(const char *key, const char *value,
> return 0;
> }
>
> + if (!strcmp(subkey, "creationtoken")) {
> + if (sscanf(value, "%"PRIu64, &bundle->creationToken) != 1)
> + warning(_("could not parse bundle list key %s with value '%s'"),
> + "creationToken", value);
> + return 0;
> + }
Should this be a warning, or a hard error? I _think_ it depends on
how much we trust creationToken supplied by the bundle providers for
correctness. If we only use the values as hint to gain performance,
and downloading the bundles in a wrong order (due to bogus
creationToken values assigned to them) does not lead to correctness
error, then warning is fine.
Looking good.
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 95
From: Christian Couder @ 2023-01-31 21:19 UTC (permalink / raw)
To: git
Cc: lwn, Junio C Hamano, Jakub Narebski, Markus Jansen,
Kaartic Sivaraam, Taylor Blau, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Teng Long, ZheNing Hu,
Michal Suchánek
Hi everyone,
The 95th edition of Git Rev News is now published:
https://git.github.io/rev_news/2023/01/31/edition-95/
Thanks a lot to Teng Long who helped this month!
Enjoy,
Christian, Jakub, Markus and Kaartic.
PS: An issue for the next edition is already opened and contributions
are welcome:
https://github.com/git/git.github.io/issues/625
^ permalink raw reply
* Re: [PATCH v3] grep: fall back to interpreter if JIT memory allocation fails
From: Junio C Hamano @ 2023-01-31 21:05 UTC (permalink / raw)
To: Mathias Krause
Cc: git, Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón
In-Reply-To: <20230131185611.520311-1-minipli@grsecurity.net>
Mathias Krause <minipli@grsecurity.net> writes:
> Mention the possibility to prefix a failing pattern with '(*NO_JIT)' in
> case we run into the JIT's limitations, as per Junio. Also clip the
> printed pattern to ensure the hint actually gets printed.
Looking good. Will queue. Thanks.
^ permalink raw reply
* Re: Stability of git-archive, breaking (?) the Github universe, and a possible solution
From: Michal Suchánek @ 2023-01-31 20:45 UTC (permalink / raw)
To: Eli Schwartz; +Cc: Konstantin Ryabitsev, brian m. carlson, Git List
In-Reply-To: <df7b0b43-efa2-ea04-dc5b-9515e7f1d86f@gmail.com>
On Tue, Jan 31, 2023 at 11:34:59AM -0500, Eli Schwartz wrote:
> On 1/31/23 11:20 AM, Konstantin Ryabitsev wrote:
> > On Tue, Jan 31, 2023 at 10:56:52AM -0500, Eli Schwartz wrote:
> >> And for tarballs that are generated once and uploaded to ftp storage,
> >> not repeatedly generated on the fly, we know the checksum will never
> >> legitimately change, so we *want* to hash the compressed file.
> >> Decompressing kernel.org tarballs in order to run PGP on them is *slow*.
> >
> > FWIW, the most correct way is:
> >
> > * download sha256sums.asc and verify its signature (auto-signed by infra)
> > * download the tarball you want and verify that the checksum matches
> > * uncompress and verify the PGP signature (signed by developer)
> >
> > This script implements this workflow:
> > https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/get-verified-tarball
>
>
> This is just what I said, but with an additional first step for when you
> are updating to a new tarball and don't have your own checksums
> integrated into your own ecosystem tracking.
>
> In most contexts, it's utterly unacceptable to not remember the checksum
> of the file you used last time and instead simply trust PGP identity
> verification. This permits upstream the technical means to be malicious,
> and re-upload a totally different tarball with the same name, different
> contents, and different PGP signature, and you will never notice because
> the PGP signature is still okay.
But where is the hash remembered?
The signature is a hash+signature, it you can replace that, you can also
repolace a hash without a signature.
You can store hashesd of anything you want locally, and indeed such
stored hashes in some build systemns did detect some code hosting
corruption but that's not for upstream to do, that's something that only
unrelated third party can do.
Thanks
Michal
^ 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