* Re: [PATCH v3 6/6] git-std-lib: add test file to call git-std-lib.a functions
From: Junio C Hamano @ 2023-09-15 20:22 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Calvin Wan, git, nasamuffin, linusa, phillip.wood123, vdye
In-Reply-To: <20230915184321.1598611-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Calvin Wan <calvinwan@google.com> writes:
>> Add test file that directly or indirectly calls all functions defined in
>> git-std-lib.a object files to showcase that they do not reference
>> missing objects and that git-std-lib.a can stand on its own.
>>
>> Certain functions that cause the program to exit or are already called
>> by other functions are commented out.
>>
>> TODO: replace with unit tests
>> Signed-off-by: Calvin Wan <calvinwan@google.com>
>
> I think the TODO should go into the code, so that when we add a unit
> test that also deletes stdlib-test.c, we can see what's happening just
> from the diff. The TODO should also explain what stdlib-test.c is hoping
> to do, and why replacing it is OK. (Also, do we need to invoke all the
> functions? I thought that missing functions are checked at link time, or
> at the very latest, when the executable is run. No need to change this,
> though - invoking all the functions we can is fine.)
>
Thanks for excellent reviews (not just against this 6/6 but others,
too).
^ permalink raw reply
* Re: [PATCH v3] revision: add `--ignore-missing-links` user option
From: Junio C Hamano @ 2023-09-15 18:54 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, me
In-Reply-To: <20230915083415.263187-1-knayak@gitlab.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> The revision backend is used by multiple porcelain commands such as
> git-rev-list(1) and git-log(1). The backend currently supports ignoring
> missing links by setting the `ignore_missing_links` bit. This allows the
> revision walk to skip any objects links which are missing. Expose this
> bit via an `--ignore-missing-links` user option.
Given the above "we merely surface a feature that already exists and
supported to be used by the end users from the command line" claim ...
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index ff715d6918..5239d83c76 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,
> {
> struct rev_list_info *info = cb_data;
> if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {
> - finish_object__ma(obj);
> + if (!info->revs->ignore_missing_links)
> + finish_object__ma(obj);
> return 1;
> }
... this hunk is a bit unexpected. As a low-level plumbing command,
shouldn't it be left to the user who gives --ignore-missing-links
from their command line to specify how the missing "obj" here should
be dealt with by giving the "--missing=<foo>" option? While giving
"allow-promisor" may not make much sense, "--missing=allow-any" may
of course make sense (it is the same as hardcoding the decision not
to call finish_object__ma() at all), and so may "--missing=print".
Stepping back a bit, with "--missing=print", is this change still
needed? The missing objects discovered will be shown at the end,
with the setting, no?
Thanks.
^ permalink raw reply
* Re: [PATCH v3 6/6] git-std-lib: add test file to call git-std-lib.a functions
From: Jonathan Tan @ 2023-09-15 18:43 UTC (permalink / raw)
To: Calvin Wan; +Cc: Jonathan Tan, git, nasamuffin, linusa, phillip.wood123, vdye
In-Reply-To: <20230908174443.1027716-6-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> Add test file that directly or indirectly calls all functions defined in
> git-std-lib.a object files to showcase that they do not reference
> missing objects and that git-std-lib.a can stand on its own.
>
> Certain functions that cause the program to exit or are already called
> by other functions are commented out.
>
> TODO: replace with unit tests
> Signed-off-by: Calvin Wan <calvinwan@google.com>
I think the TODO should go into the code, so that when we add a unit
test that also deletes stdlib-test.c, we can see what's happening just
from the diff. The TODO should also explain what stdlib-test.c is hoping
to do, and why replacing it is OK. (Also, do we need to invoke all the
functions? I thought that missing functions are checked at link time, or
at the very latest, when the executable is run. No need to change this,
though - invoking all the functions we can is fine.)
^ permalink raw reply
* Re: [PATCH v3 5/6] git-std-lib: introduce git standard library
From: Jonathan Tan @ 2023-09-15 18:39 UTC (permalink / raw)
To: Calvin Wan; +Cc: Jonathan Tan, git, nasamuffin, linusa, phillip.wood123, vdye
In-Reply-To: <20230908174443.1027716-5-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> diff --git a/Makefile b/Makefile
> index 9226c719a0..0a2d1ae3cc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -669,6 +669,7 @@ FUZZ_PROGRAMS =
> GIT_OBJS =
> LIB_OBJS =
> SCALAR_OBJS =
> +STUB_OBJS =
> OBJECTS =
> OTHER_PROGRAMS =
> PROGRAM_OBJS =
I don't think stubs should be compiled into git-std-lib.a - I would
expect a consumer of this library to be able to specify their own
implementations if needed (e.g. their own trace2).
> @@ -956,6 +957,7 @@ COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
>
> LIB_H = $(FOUND_H_SOURCES)
>
> +ifndef GIT_STD_LIB
> LIB_OBJS += abspath.o
> LIB_OBJS += add-interactive.o
> LIB_OBJS += add-patch.o
> @@ -1196,6 +1198,27 @@ LIB_OBJS += write-or-die.o
> LIB_OBJS += ws.o
> LIB_OBJS += wt-status.o
> LIB_OBJS += xdiff-interface.o
> +else ifdef GIT_STD_LIB
> +LIB_OBJS += abspath.o
> +LIB_OBJS += ctype.o
> +LIB_OBJS += date.o
> +LIB_OBJS += hex-ll.o
> +LIB_OBJS += parse.o
> +LIB_OBJS += strbuf.o
> +LIB_OBJS += usage.o
> +LIB_OBJS += utf8.o
> +LIB_OBJS += wrapper.o
This means that LIB_OBJS (in this patch, used both by git-std-lib and
as part of compiling the regular Git binary) can differ based on the
GIT_STD_LIB variable. It does seem that we cannot avoid GIT_STD_LIB
for now, because the git-std-lib can only be compiled without GETTEXT
(so we need a variable to make sure that none of these .o files are
compiled with GETTEXT), but we should still minimize the changes between
compiling with GIT_STD_LIB and without it, at least to minimize future
work. Could we have two separate lists? So, leave LIB_OBJS alone and
make a new STD_LIB_OBJS.
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3e7a59b5ff..14bf71c530 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -455,8 +455,8 @@ static inline int noop_core_config(const char *var UNUSED,
> #define platform_core_config noop_core_config
> #endif
>
> +#if !defined(__MINGW32__) && !defined(_MSC_VER) && !defined(GIT_STD_LIB)
> int lstat_cache_aware_rmdir(const char *path);
> -#if !defined(__MINGW32__) && !defined(_MSC_VER)
> #define rmdir lstat_cache_aware_rmdir
> #endif
I think we still want to keep the idea of "the code should still be good
even if we have no use for git-std-lib" as much as possible, so could we
stub lstat_cache_aware_rmdir() instead? We could have a new git-compat-
util-stub.c (or whatever we want to call it).
> @@ -966,9 +966,11 @@ const char *inet_ntop(int af, const void *src, char *dst, size_t size);
> #endif
>
> #ifdef NO_PTHREADS
> +#ifdef GIT_STD_LIB
> #define atexit git_atexit
> int git_atexit(void (*handler)(void));
> #endif
> +#endif
>
> static inline size_t st_add(size_t a, size_t b)
> {
Same for git_atexit().
> @@ -1462,14 +1464,17 @@ static inline int is_missing_file_error(int errno_)
> return (errno_ == ENOENT || errno_ == ENOTDIR);
> }
>
> +#ifndef GIT_STD_LIB
> int cmd_main(int, const char **);
>
> /*
> * Intercept all calls to exit() and route them to trace2 to
> * optionally emit a message before calling the real exit().
> */
> +
> int common_exit(const char *file, int line, int code);
> #define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
> +#endif
>
> /*
> * You can mark a stack variable with UNLEAK(var) to avoid it being
And for common_exit().
As for cmd_main(), that seems to be a convenience so that we can link
common_main.o with various other files (e.g. http-backend.c). I think
the right thing to do is to define a new cmd-main.h that declares only
cmd_main(), and then have only the files that need it (common_main.c and
all the files that define cmd_main()) include it. This cleanup patch can
be done before this patch. I think this is a good change that we would
want even without libification.
^ permalink raw reply
* Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0
From: Taylor Blau @ 2023-09-15 18:38 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230915113443.GB3531587@coredump.intra.peff.net>
On Fri, Sep 15, 2023 at 07:34:43AM -0400, Jeff King wrote:
> @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)
> skip_iprefix(line, "h2 [", out))
> return 1;
>
> + /*
> + * curl 8.3.0 uses:
> + * [HTTP/2] [<stream-id>] [<header-name>: <header-val>]
> + * where <stream-id> is numeric.
> + */
> + if (skip_iprefix(line, "[HTTP/2] [", &p)) {
> + while (isdigit(*p))
> + p++;
> + if (skip_prefix(p, "] [", out))
> + return 1;
> + }
> +
This looks good, too, though I do have one question. The HTTP/2
specification in 5.1 says (among other things):
Streams are identified with an unsigned 31-bit integer. Streams
initiated by a client MUST use odd-numbered stream identifiers; those
initiated by the server MUST use even-numbered stream identifiers. A
stream identifier of zero (0x0) is used for connection control messages;
the stream identifier of zero cannot be used to establish a new stream.
So the parsing you wrote here makes sense in that we consume digits
between the pair of square brackets enclosing the stream identifier.
But I think we would happily eat a line like:
[HTTP/2] [] [Secret: xyz]
even lacking a stream identifier. I think that's reasonably OK in
practice, because we're being over-eager in redacting instead of the
other way around. And we're unlikely to see such a line from curl
anyway, so I don't think that it matters.
If you feel otherwise, though, I think something as simple as:
if (skip_iprefix(line, "[HTTP/2] [", &p)) {
if (!*p)
return 0;
while (isdigit(*p))
p++;
if (skip_prefix(p, "] [", out))
return 1;
}
would do the trick. I *think* that this would also work:
if (skip_iprefix(line, "[HTTP/2] [", &p)) {
do {
p++;
} while (isdigit(*p))
if (skip_prefix(p, "] [", out))
return 1;
}
since we know that p is non-NULL, and if it's the end of the line, *p
will be NUL and isdigit(*p) will return 0. But it's arguably less
direct, and requires some extra reasoning, so I have a vague preference
for the former.
But this may all be moot anyway, I don't feel strongly one way or the
other.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/2] http: factor out matching of curl http/2 trace lines
From: Taylor Blau @ 2023-09-15 18:29 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230915113316.GA3531587@coredump.intra.peff.net>
On Fri, Sep 15, 2023 at 07:33:16AM -0400, Jeff King wrote:
> We have to parse out curl's http/2 trace lines so we can redact their
> headers. We already match two different types of lines from various
> vintages of curl. In preparation for adding another (which will be
> slightly more complex), let's pull the matching into its own function,
> rather than doing it in the middle of a conditional.
>
> While we're doing so, let's expand the comment a bit to describe the two
> matches. That probably should have been part of db30130165 (http: handle
> both "h2" and "h2h3" in curl info lines, 2023-06-17), but will become
> even more important as we add new types.
>
> Signed-off-by: Jeff King <peff@peff.net>
Makes sense, and this sets us up well for the next step in fixing the
test fallout from newer versions of curl.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 0/2] updating curl http/2 header matching (again)
From: Taylor Blau @ 2023-09-15 18:28 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230915113237.GA3531328@coredump.intra.peff.net>
On Fri, Sep 15, 2023 at 07:32:37AM -0400, Jeff King wrote:
> (If anyone wants to confirm the bug or test, the new version of curl
> just hit debian unstable).
These two patches look sensible, and I can confirm that they resolve the
issue with the newest version of curl. With the latest 'master' (which
is bda494f404 (The ninth batch, 2023-09-14), at the time of writing) and
an old version of curl, I get:
$ ./t5559-http-fetch-smart-http2.sh -vdi
[...]
expecting success of 5559.17 'GIT_TRACE_CURL redacts auth details':
rm -rf redact-auth trace &&
set_askpass user@host pass@host &&
GIT_TRACE_CURL="$(pwd)/trace" git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth &&
expect_askpass both user@host &&
# Ensure that there is no "Basic" followed by a base64 string, but that
# the auth details are redacted
! grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace &&
grep -i "Authorization: Basic <redacted>" trace
Cloning into bare repository 'redact-auth'...
remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (6/6), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 6 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (6/6), done.
== Info: [HTTP/2] [3] [authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==]
== Info: [HTTP/2] [5] [authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==]
== Info: [HTTP/2] [7] [authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==]
not ok 17 - GIT_TRACE_CURL redacts auth details
#
# rm -rf redact-auth trace &&
# set_askpass user@host pass@host &&
# GIT_TRACE_CURL="$(pwd)/trace" git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth &&
# expect_askpass both user@host &&
#
# # Ensure that there is no "Basic" followed by a base64 string, but that
# # the auth details are redacted
# ! grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace &&
# grep -i "Authorization: Basic <redacted>" trace
#
and after applying these patches, those tests pass.
$ ./t5559-http-fetch-smart-http2.sh -di
[...]
# passed all 54 test(s)
1..54
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH] Not computing changed path filter for root commits
From: Junio C Hamano @ 2023-09-15 18:25 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, szeder.dev, me, derrickstolee
In-Reply-To: <20230911223157.446269-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> This is following the discussion about adding a new changed path filter
> version due to the current implementation of murmur3 not matching the
> algorithm. [1]
>
> SZEDER Gábor suggested [2] that we change the revision walk to read
> changed path filters also for root commits, but I don't think that's
> possible - we have to tie reading changed path filters to when we read
> trees, and right now, we don't seem to read trees when evaluating root
> commits (rev_compare_tree() in revision.c is in the only code path that
> uses changed path filters, and it itself is only called per-parent and
> thus not called for root commits). The alternative is to not generate
> changed path filters for root commits (or what I did in this patch,
> which is to generate an all-1 filter), which seems reasonable to me.
I know this is a very silly question, but if the filter is not read
for root commits at runtime, does it matter if a filter is created
for them beforehand (or not)? They will not be read whether if they
exist or not, no? One observation in the thread [2] appears in was:
In several of the above test cases test_bloom_filters_used is invoked
in a repository with only a root commit, so they don't check that
the output is the same with and without Bloom filters.
i.e. the check would be ineffective with the current system that we
know does not use the filter for a root commit even if it existed.
But would it be an improvement to add a filter to a root commit and
test with the filter enabled and disabled to compare the results, if
we know the filter is not used anyway?
^ permalink raw reply
* Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0
From: Junio C Hamano @ 2023-09-15 18:21 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230915113443.GB3531587@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)
> skip_iprefix(line, "h2 [", out))
> return 1;
>
> + /*
> + * curl 8.3.0 uses:
> + * [HTTP/2] [<stream-id>] [<header-name>: <header-val>]
> + * where <stream-id> is numeric.
> + */
> + if (skip_iprefix(line, "[HTTP/2] [", &p)) {
> + while (isdigit(*p))
> + p++;
> + if (skip_prefix(p, "] [", out))
> + return 1;
> + }
Looking good assuming that <stream-id> part will never be updated to
allow spaces around the ID, or allow non-digits in the ID, in the
future. Is there much harm if this code allowed false positives and
sent something that is *not* a curl trace, like "foo]" parsed out of
"[HTTP/2] [PATCH] [foo]", to redact_sensitive_header() function?
By the way, would this patch make sense? Everybody in the function
that try to notice a sensitive header seems to check the sentting
independently, which seems error prone for those who want to add a
new header to redact.
http.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git c/http.c w/http.c
index 8f71bf00d8..3dfa34fe65 100644
--- c/http.c
+++ w/http.c
@@ -684,8 +684,10 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)
int ret = 0;
const char *sensitive_header;
- if (trace_curl_redact &&
- (skip_iprefix(header->buf + offset, "Authorization:", &sensitive_header) ||
+ if (!trace_curl_redact)
+ return ret;
+
+ if ((skip_iprefix(header->buf + offset, "Authorization:", &sensitive_header) ||
skip_iprefix(header->buf + offset, "Proxy-Authorization:", &sensitive_header))) {
/* The first token is the type, which is OK to log */
while (isspace(*sensitive_header))
@@ -696,8 +698,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)
strbuf_setlen(header, sensitive_header - header->buf);
strbuf_addstr(header, " <redacted>");
ret = 1;
- } else if (trace_curl_redact &&
- skip_iprefix(header->buf + offset, "Cookie:", &sensitive_header)) {
+ } else if (skip_iprefix(header->buf + offset, "Cookie:", &sensitive_header)) {
struct strbuf redacted_header = STRBUF_INIT;
const char *cookie;
^ permalink raw reply related
* Re: [PATCH] doc: pull: improve rebase=false documentation
From: Junio C Hamano @ 2023-09-15 18:00 UTC (permalink / raw)
To: Linus Arver; +Cc: Dragan Simic, git
In-Reply-To: <xmqqh6nvfi2p.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Supplying "--rebase" defaults to "--rebase=true". Running git-pull
>> without arguments implies "--rebase=false", unless relevant
>> configuration variables have been set otherwise.
>
> Phrase nit.
>
> $ git pull origin
>
> does run the command with arguments. What you mean is "running
> git-pull without any --rebase arguments implies --no-rebase", but
A nit of my own: "--rebase arguments" -> "--rebase option"
Sorry for the noise ;-)
^ permalink raw reply
* Re: [PATCH v3 2/6] wrapper: remove dependency to Git-specific internal file
From: Jonathan Tan @ 2023-09-15 17:54 UTC (permalink / raw)
To: Calvin Wan; +Cc: Jonathan Tan, git, nasamuffin, linusa, phillip.wood123, vdye
In-Reply-To: <20230908174443.1027716-2-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> In order for wrapper.c to be built independently as part of a smaller
> library, it cannot have dependencies to other Git specific
> internals. remove_or_warn() creates an unnecessary dependency to
> object.h in wrapper.c. Therefore move the function to entry.[ch] which
> performs changes on the worktree based on the Git-specific file modes in
> the index.
Looking at remove_or_warn(), it's only used from entry.c and apply.c
(which already includes entry.h for another reason) so moving it to
entry.c looks fine.
^ permalink raw reply
* [PATCH] docs: fix "without do cleaning" typo
From: Caleb Hill via GitGitGadget @ 2023-09-15 17:53 UTC (permalink / raw)
To: git; +Cc: Caleb Hill, Caleb Hill
From: Caleb Hill <chill389cc@gmail.com>
This pr fixes a typo I noticed today while reading documentation here:
https://git-scm.com/docs/git-clean#Documentation/git-clean.txt-quit
Signed-off-by: Caleb Hill <chill389cc@gmail.com>
---
docs: fix "without do cleaning" typo
This pr fixes a simple typo I noticed today while reading documentation
here:
https://git-scm.com/docs/git-clean#Documentation/git-clean.txt-quit
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1572%2Fchill389cc%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1572/chill389cc/master-v1
Pull-Request: https://github.com/git/git/pull/1572
Documentation/git-clean.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 160d08b86bb..5e1a3d5148c 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -127,7 +127,7 @@ ask each::
quit::
- This lets you quit without do cleaning.
+ This lets you quit without doing any cleaning.
help::
base-commit: 94e83dcf5b5faaa22e32729305f8fd7090bfdfed
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Junio C Hamano @ 2023-09-15 17:51 UTC (permalink / raw)
To: Jeff King
Cc: Rubén Justo, Git List,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20230915112906.GA3125427@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But having done so, the main value in re-rolling would be preventing
> somebody else from reading the code and having the same question.
Indeed. It would be valuable to help future developers not to waste
time on wondering what we already know they may do so on.
^ permalink raw reply
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Junio C Hamano @ 2023-09-15 17:49 UTC (permalink / raw)
To: Rubén Justo
Cc: Jeff King, Git List, Ævar Arnfjörð Bjarmason
In-Reply-To: <2890b210-c42f-41cf-e676-0b1c56310f73@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
>> And the problem is in (3). You switch it to trigger only if we have no
>> failures (fixing the inversion). But should we have the same a/b split
>> for this case? I.e.:
>>
>> 3a. if we saw no test failures, invert to cause a failure
>> 3b. we saw other failures; do not invert, but _do_ mention that the
>> log found extra leaks
>>
>> In 3b we are explaining to the user what happened. Though maybe it is
>> not super important, because I think we'd have dumped the log contents
>> anyway?
>
> I think so too. At that point we've already dumped the contents of the
> $TEST_RESULTS_SAN_FILE file.
> ...
> However, if you or anyone else thinks it adds value, I have no objection
> to re-roll with it.
I do not know offhand if we need the code update to implement what
Peff called "maybe it is not super important", but if we decide not
to, at least it would help future developers to document the fact
that we were aware of the issue when the code was developed, and why
we decided not to address it (in other words, describe why we
decided it is not super important).
Thanks both for polishing the series and making it better.
^ permalink raw reply
* Re: [PATCH] doc: pull: improve rebase=false documentation
From: Junio C Hamano @ 2023-09-15 17:43 UTC (permalink / raw)
To: Linus Arver; +Cc: Dragan Simic, git
In-Reply-To: <owlyedj0jok7.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Aside: interestingly, there appears to be a "--no-rebase" option that
> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about --rebase,
> 2007-11-28)):
>
> --no-rebase
> This is shorthand for --rebase=false.
> ...
> How about adding something like this instead as the very first paragraph
> for this flag?
>
> Supplying "--rebase" defaults to "--rebase=true". Running git-pull
> without arguments implies "--rebase=false", unless relevant
> configuration variables have been set otherwise.
Phrase nit.
$ git pull origin
does run the command with arguments. What you mean is "running
git-pull without any --rebase arguments implies --no-rebase", but
that is saying "not giving --rebase=<any> and not giving --rebase
means not rebasing", which makes my head spin.
"--no-rebase" as a command line option does have use to defeat
configured pull.rebase that is not set to "false", and allowing
"pull.rebase" to be set to "false" does have use to defeat settings
for the same variable made by lower-precedence configuration file.
"--rebase=false" does not have any reason to exist, except for
making the repertoire of "--rebase=<kind>" to be complete.
So, I am still not sure if saying "'git pull' (no other arguments
and no configuration) is equivalent to 'git pull --rebase=false'"
adds much value.
If --no-rebase and --rebase=false are explained in terms of why
these options that specify such an unnatural action (after all, you
say "do this" or "do it this way", but do not usually have to say
"do not do it that way") need to exist.
If I were writing this patch, I would rearrange the existing text
like so:
* Update the description of "--no-rebase" *NOT* to depend on
--rebase=false. Instead move it higher and say
- The default for "git pull" is to "merge" the other history into
your history, but optionally you can "rebase" your history on
top of the other history.
- There are configuration variables (pull.rebase and
branch.<name>.rebase) that trigger the optional behaviour, and
when you set it, your "git pull" would "rebase".
- The "--no-rebase" option is to defeat such configuration to
tell the command to "merge" for this particular invocation.
* Update the description of "--rebase=<kind>" and move the
paragraph that begins with "When false" to the end, something
like:
- `--rebase` alone is equivalent to `--rebase=true`.
- When set to 'merges'...
- When set to 'interactive'...
- See `pull.rebase`, ..., if you want to make `git pull` always
rebase your history on top of theirs, instead of merging their
history to yours.
- `--rebase=false` is synonym to `--no-rebase`.
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Junio C Hamano @ 2023-09-15 17:24 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87y1h8wbpo.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> I don't see why desire to look at diff-to-first-parent on "side"
> branches is any different from desire to look at them on "primary"
> branch
Yeah, but that is not what I meant. The above argues for why
"--diff-merges=first-parent" should exist independently from the
"--first-parent" traversal *and* display option. I am not saying
it should not exist.
But I view that the desire to look at any commits and its changes on
the "side" branch at all *is* at odds with the wish to look at
first-parent change for merge commits. Once you decide to look at
first-parent change for a merge commit, then every change you see
for each commit on the "side" branch, whether it is shown as
first-parent diff or N pairwise diffs, is what you have already seen
in the change in the merge commit, because "git log" goes newer to
older, and the commits on the side branches appear after the merge
that brings them to the mainline.
Making "log -d" mean "log --diff-merges=first-parent --patch" lets
that less useful combination ("show first-parent patches but
traverse side branches as well") squat on the short and sweet "-d"
that could be used for more useful "log --first-parent --patch",
which would also be more common and intuitive to users, and that is
what I suspect will become problematic in the longer run.
Thanks.
^ permalink raw reply
* Re: BUG: git-gui no longer executes hook scripts
From: Junio C Hamano @ 2023-09-15 17:15 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git, johannes.schindelin, me
In-Reply-To: <xmqqa5tngynh.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Shouldn't this "is it absolute" check with "$cmd" also check if $cmd
> has either forward or backward slash in it?
>
> Checking the use of _which with fixed arguments, it is used to spawn
> git, gitk, nice, sh; and _which finding where they appear on the
> search path does sound sane. But _which does not seem to have the "if
> given a command with directory separator, the search path does not
> matter. The caller means it is relative to the $cwd" logic at all,
> so it seems it is the callers responsibility to make sure it does
> not pass things like ".git/hooks/pre-commit" to it.
In other words, something along this line may go in the right
direction (I no longer speak Tcl, and this is done with manual in
one hand, while typing with the other hand).
git-gui.sh | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git c/git-gui.sh w/git-gui.sh
index 8bc8892c40..45d8f48b39 100755
--- c/git-gui/git-gui.sh
+++ w/git-gui/git-gui.sh
@@ -119,11 +119,15 @@ proc sanitize_command_line {command_line from_index} {
while {$i < [llength $command_line]} {
set cmd [lindex $command_line $i]
if {[file pathtype $cmd] ne "absolute"} {
- set fullpath [_which $cmd]
- if {$fullpath eq ""} {
- throw {NOT-FOUND} "$cmd not found in PATH"
+ if {1 < [llength [file split $cmd]]]} {
+ set cmdpath [_which $cmd]
+ if {$cmdpath eq ""} {
+ throw {NOT-FOUND} "$cmd not found in PATH"
+ }
+ } else {
+ set cmdpath $cmd
}
- lset command_line $i $fullpath
+ lset command_line $i $cmdpath
}
# handle piped commands, e.g. `exec A | B`
^ permalink raw reply related
* Re: BUG: git-gui no longer executes hook scripts
From: Junio C Hamano @ 2023-09-15 17:00 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git, johannes.schindelin, me
In-Reply-To: <bd510f6d-6613-413b-6d64-c3d2fd01d8a9@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> The commit titled "Work around Tcl's default |PATH| lookup",|aae9560,
> adds checking on all commands to be executed to assure these are on
> the PATH.
commit aae9560a355d4ab91385e49eae62fade2ddd27ef
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: Wed Nov 23 09:31:06 2022 +0100
Work around Tcl's default `PATH` lookup
As per https://www.tcl.tk/man/tcl8.6/TclCmd/exec.html#M23, Tcl's `exec`
function goes out of its way to imitate the highly dangerous path lookup
of `cmd.exe`, but _of course_ only on Windows:
If a directory name was not specified as part of the application
name, the following directories are automatically searched in
order when attempting to locate the application:
In other words, if somebody tries to run ".git/hooks/pre-commit",
because a directory name _is_ given (i.e. ".git/hooks/" in this case),
the path lookup is *not* done. Which is what I would expect, and then
"oh, only on Windows to match what cmd.exe does, the current directory
is early in the search order" should not be a problem.
To avoid that, Git GUI already has the `_which` function that does not
imitate that dangerous practice when looking up executables in the
search path.
Sounds good, but ...
diff --git a/git-gui.sh b/git-gui.sh
index b0eb5a6ae4..cb92bba1c4 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -121,6 +121,62 @@ proc _which {what args} {
return {}
}
+proc sanitize_command_line {command_line from_index} {
+ set i $from_index
+ while {$i < [llength $command_line]} {
+ set cmd [lindex $command_line $i]
+ if {[file pathtype $cmd] ne "absolute"} {
+ set fullpath [_which $cmd]
+ if {$fullpath eq ""} {
+ throw {NOT-FOUND} "$cmd not found in PATH"
+ }
+ lset command_line $i $fullpath
Shouldn't this "is it absolute" check with "$cmd" also check if $cmd
has either forward or backward slash in it? I do not know about the
Windows cmd.exe convention, but with Unix background, I would be
surprised if dir/cmd gave by end users ran "C:\program
files\dir\cmd" (unless I happened to be in the "C:\program files\"
folder, that is).
Checking the use of _which with fixed arguments, it is used to spawn
git, gitk, nice, sh; and _which finding where they appear on the
search path does sound sane. But _which does not seem to have the "if
given a command with directory separator, the search path does not
matter. The caller means it is relative to the $cwd" logic at all,
so it seems it is the callers responsibility to make sure it does
not pass things like ".git/hooks/pre-commit" to it.
+ }
+
+ # handle piped commands, e.g. `exec A | B`
+ for {incr i} {$i < [llength $command_line]} {incr i} {
+ if {[lindex $command_line $i] eq "|"} {
+ incr i
+ break
+ }
+ }
+ }
+ return $command_line
+}
+
+# Override `exec` to avoid unsafe PATH lookup
+
+rename exec real_exec
+
+proc exec {args} {
+ # skip options
+ for {set i 0} {$i < [llength $args]} {incr i} {
+ set arg [lindex $args $i]
+ if {$arg eq "--"} {
+ incr i
+ break
+ }
+ if {[string range $arg 0 0] ne "-"} {
+ break
+ }
+ }
+ set args [sanitize_command_line $args $i]
+ uplevel 1 real_exec $args
+}
+
+# Override `open` to avoid unsafe PATH lookup
+
+rename open real_open
+
+proc open {args} {
+ set arg0 [lindex $args 0]
+ if {[string range $arg0 0 0] eq "|"} {
+ set command_line [string trim [string range $arg0 1 end]]
+ lset args 0 "| [sanitize_command_line $command_line 0]"
+ }
+ uplevel 1 real_open $args
+}
+
######################################################################
##
## locate our library
^ permalink raw reply related
* BUG: git-gui no longer executes hook scripts
From: Mark Levedahl @ 2023-09-15 16:45 UTC (permalink / raw)
To: git, johannes.schindelin, me
The commit titled "Work around Tcl's default |PATH| lookup",|aae9560,
adds checking on all commands to be executed to assure these are on the
PATH. Any script in .git/hooks is rejected as .git/hooks is not (in
general) on the PATH, even if the entry in .git/hooks is a symlink to a
file on the PATH. Instead, git-gui throws and error without completing
the operation. This is easily demonstrated by say, enabling the
commit-msg script (hooks-commit-msg.sample templates) and attempting a
commit.
|
|I don't have a suggested solution to this: reverting the above commit
will fix this problem, but that commit was made to mitigate a security
issue. Perhaps anything in .git/hooks should be accepted without
further checks?
|
|
|
|Mark
|
^ permalink raw reply
* Re: Please explain avoiding history simplifications when diffing merges
From: Magnus Holmgren @ 2023-09-15 15:10 UTC (permalink / raw)
To: git
In-Reply-To: <3337579.YDm8p7mPUg@utklippan>
Friday, 8 September 2023 11:09:20 CEST, I wrote
> QGit was bitten by
> https://github.com/git/git/commit/0dec322d31db3920872f43bdd2a7ddd282a5be67
Maybe I should link to the QGit issue:
https://github.com/tibirna/qgit/issues/129
> It looks like passing --simplify-merges to override the default solves the
> problem, but I still want to ask here because I'm not sure I fully
> understand
> the reasoning:
> > the default history simplification would remove merge commits from
> > consideration if the file "path" matched the second parent.
As I wrote at the above URL, I realized that the old git log output without --
simplify-merges and the output with --simplify-merges aren't quite the same.
The old output indeed omits some interesting merge commits, which may explain
why the change was made, but git log --simplify-merges does include them, so
it seems a reasonable default to me.
However, QGit has a problem: git log --diff-merges=separate includes a
separate diff for each parent, but only for each parent with differences
compared to the merge commit, *and* there's no custom format placeholder for
the current parent, only for the list of parents (%P/%p). How should one go
about adding that? I figure the format_commit_context struct in pretty.c needs
another field.
--
Magnus Holmgren
./¯\_/¯\. Milient
^ permalink raw reply
* [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0
From: Jeff King @ 2023-09-15 11:34 UTC (permalink / raw)
To: git
In-Reply-To: <20230915113237.GA3531328@coredump.intra.peff.net>
To redact header lines in http/2 curl traces, we have to parse past some
prefix bytes that curl sticks in the info lines it passes to us. That
changed once already, and we adapted in db30130165 (http: handle both
"h2" and "h2h3" in curl info lines, 2023-06-17).
Now it has changed again, in curl's fbacb14c4 (http2: cleanup trace
messages, 2023-08-04), which was released in curl 8.3.0. Running a build
of git linked against that version will fail to redact the trace (and as
before, t5559 notices and complains).
The format here is a little more complicated than the other ones, as it
now includes a "stream id". This is not constant but is always numeric,
so we can easily parse past it.
We'll continue to match the old versions, of course, since we want to
work with many different versions of curl. We can't even select one
format at compile time, because the behavior depends on the runtime
version of curl we use, not the version we build against.
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/http.c b/http.c
index c93e0a7ea6..8f71bf00d8 100644
--- a/http.c
+++ b/http.c
@@ -740,6 +740,8 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)
static int match_curl_h2_trace(const char *line, const char **out)
{
+ const char *p;
+
/*
* curl prior to 8.1.0 gives us:
*
@@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)
skip_iprefix(line, "h2 [", out))
return 1;
+ /*
+ * curl 8.3.0 uses:
+ * [HTTP/2] [<stream-id>] [<header-name>: <header-val>]
+ * where <stream-id> is numeric.
+ */
+ if (skip_iprefix(line, "[HTTP/2] [", &p)) {
+ while (isdigit(*p))
+ p++;
+ if (skip_prefix(p, "] [", out))
+ return 1;
+ }
+
return 0;
}
--
2.42.0.661.g2507eb519e
^ permalink raw reply related
* [PATCH 1/2] http: factor out matching of curl http/2 trace lines
From: Jeff King @ 2023-09-15 11:33 UTC (permalink / raw)
To: git
In-Reply-To: <20230915113237.GA3531328@coredump.intra.peff.net>
We have to parse out curl's http/2 trace lines so we can redact their
headers. We already match two different types of lines from various
vintages of curl. In preparation for adding another (which will be
slightly more complex), let's pull the matching into its own function,
rather than doing it in the middle of a conditional.
While we're doing so, let's expand the comment a bit to describe the two
matches. That probably should have been part of db30130165 (http: handle
both "h2" and "h2h3" in curl info lines, 2023-06-17), but will become
even more important as we add new types.
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/http.c b/http.c
index e138b4b96f..c93e0a7ea6 100644
--- a/http.c
+++ b/http.c
@@ -738,18 +738,29 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)
return ret;
}
+static int match_curl_h2_trace(const char *line, const char **out)
+{
+ /*
+ * curl prior to 8.1.0 gives us:
+ *
+ * h2h3 [<header-name>: <header-val>]
+ *
+ * Starting in 8.1.0, the first token became just "h2".
+ */
+ if (skip_iprefix(line, "h2h3 [", out) ||
+ skip_iprefix(line, "h2 [", out))
+ return 1;
+
+ return 0;
+}
+
/* Redact headers in info */
static void redact_sensitive_info_header(struct strbuf *header)
{
const char *sensitive_header;
- /*
- * curl's h2h3 prints headers in info, e.g.:
- * h2h3 [<header-name>: <header-val>]
- */
if (trace_curl_redact &&
- (skip_iprefix(header->buf, "h2h3 [", &sensitive_header) ||
- skip_iprefix(header->buf, "h2 [", &sensitive_header))) {
+ match_curl_h2_trace(header->buf, &sensitive_header)) {
if (redact_sensitive_header(header, sensitive_header - header->buf)) {
/* redaction ate our closing bracket */
strbuf_addch(header, ']');
--
2.42.0.661.g2507eb519e
^ permalink raw reply related
* [PATCH 0/2] updating curl http/2 header matching (again)
From: Jeff King @ 2023-09-15 11:32 UTC (permalink / raw)
To: git
This is similar to the issue discussed/fixed a few months ago in:
https://lore.kernel.org/git/20230617051559.GD562686@coredump.intra.peff.net/
but it looks like curl has updated the trace line format again. This
updates our matching code to handle it. It would obviously be nice to
avoid the dependency altogether, but I don't think there is another
option here. I expected the previous update to last longer than it did,
but hopefully things will settle a bit more now. :)
(If anyone wants to confirm the bug or test, the new version of curl
just hit debian unstable).
[1/2]: http: factor out matching of curl http/2 trace lines
[2/2]: http: update curl http/2 info matching for curl 8.3.0
http.c | 37 +++++++++++++++++++++++++++++++------
1 file changed, 31 insertions(+), 6 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Jeff King @ 2023-09-15 11:29 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Ævar Arnfjörð Bjarmason
In-Reply-To: <2890b210-c42f-41cf-e676-0b1c56310f73@gmail.com>
On Fri, Sep 15, 2023 at 02:28:15AM +0200, Rubén Justo wrote:
> > And the problem is in (3). You switch it to trigger only if we have no
> > failures (fixing the inversion). But should we have the same a/b split
> > for this case? I.e.:
> >
> > 3a. if we saw no test failures, invert to cause a failure
> > 3b. we saw other failures; do not invert, but _do_ mention that the
> > log found extra leaks
> >
> > In 3b we are explaining to the user what happened. Though maybe it is
> > not super important, because I think we'd have dumped the log contents
> > anyway?
>
> I think so too. At that point we've already dumped the contents of the
> $TEST_RESULTS_SAN_FILE file.
>
> IMO, when $test_failure is zero (the "if" I'm touching), the message
> makes sense not so much to say that a leak has been found, but rather
> because we're forcing the non-zero exit.
>
> But when $test_failure is not zero, after we've already dumped the
> log, maybe this is somewhat redundant:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 87cfea9e9a..b160ae3f7a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {
> then
> say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
> invert_exit_code=t
> + else
> + say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak"
> fi
> }
>
> However, if you or anyone else thinks it adds value, I have no objection
> to re-roll with it.
I'm on the fence. It is probably not a big deal, and my biggest issue is
just that I had to walk through the explanation in my previous mail to
convince myself the change was not missing an important case.
But having done so, the main value in re-rolling would be preventing
somebody else from reading the code and having the same question. But
this discussion in the archive is probably sufficient.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Christian Couder @ 2023-09-15 10:09 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
On Wed, Sep 13, 2023 at 9:17 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> Here is a small reroll of my series to clean up some of the internals of
> 'git repack' used to track the set of existing packs.
>
> Much is unchanged from the last round, save for some additional clean-up
> on how we handle the '->util' field for each pack's string_list_item in
> response to very helpful review from those CC'd.
>
> As usual, a range-diff is available below for convenience. Thanks in
> advance for your review!
>
> Taylor Blau (8):
> builtin/repack.c: extract structure to store existing packs
> builtin/repack.c: extract marking packs for deletion
> builtin/repack.c: extract redundant pack cleanup for --geometric
> builtin/repack.c: extract redundant pack cleanup for existing packs
> builtin/repack.c: extract `has_existing_non_kept_packs()`
> builtin/repack.c: store existing cruft packs separately
> builtin/repack.c: avoid directly inspecting "util"
> builtin/repack.c: extract common cruft pack loop
I think it would be a bit nicer with s/builtin\/repack.c/repack/ in
all the above commit subjects, but I don't think it's worth a reroll.
Except for another very small nit in a commit message also not worth a
reroll, this LGTM.
Thanks,
Christian.
^ 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