* Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox
From: Andy Koppe @ 2023-11-23 22:03 UTC (permalink / raw)
To: Junio C Hamano, Willem Verstraeten; +Cc: git
In-Reply-To: <xmqqv89tau3r.fsf@gitster.g>
On 23/11/2023 05:58, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I guess we could change the behaviour so that
>>
>> git checkout -B <branch> [<start-point>]
>>
>> fails when <branch> is an existing branch that is in use in another
>> worktree, and allow "-f" to be used to override the safety, i.e.,
>>
>> git checkout -f -B <branch> [<start-point>]
>>
>> would allow the <branch> to be repointed to <start-point> (or HEAD)
>> even when it is used elsewhere.
>
> It turns out that for some reason "-f" is not how we decided to
> override this one---there is "--ignore-other-worktrees" option.
Presumably that's because -f means throwing away any local changes that
are in the way of checking out the new HEAD, which you wouldn't
necessarily want when trying to replace an existing branch.
Andy
^ permalink raw reply
* Re: [PATCH v2 4/4] doc: refer to internet archive
From: Elijah Newren @ 2023-11-23 21:33 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <9f0bba69492b345fe7b0c7f9529b025ed98c7e29.1695553043.git.gitgitgadget@gmail.com>
On Sun, Sep 24, 2023 at 2:47 PM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Josh Soref <jsoref@gmail.com>
>
> These pages are no longer reachable from their original locations,
> which makes things difficult for readers. Instead, switch to linking to
> the Internet Archive for the content.
Thanks, these all look good, except on of the old links works for me.
Maybe it was just down the day you checked? More comments on that
below...
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> gitweb/gitweb.perl | 4 ++--
> sha1dc/sha1.c | 2 +-
> t/lib-gpg.sh | 2 +-
> t/t9816-git-p4-locked.sh | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index b6659410ef1..f12bed87db9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -8192,8 +8192,8 @@ sub git_feed {
> my $format = shift || 'atom';
> my $have_blame = gitweb_check_feature('blame');
>
> - # Atom: http://www.atomenabled.org/developers/syndication/
> - # RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
> + # Atom: https://web.archive.org/web/20230815171113/https://www.atomenabled.org/developers/syndication/
> + # RSS: https://web.archive.org/web/20030729001534/http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
The original www.atomenabled.org link works for me.
^ permalink raw reply
* Re: [PATCH v2 2/4] doc: update links to current pages
From: Elijah Newren @ 2023-11-23 21:27 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <80eb5da8ed45af4306e7bb28403e31e285efb3a9.1695553042.git.gitgitgadget@gmail.com>
On Sun, Sep 24, 2023 at 8:06 AM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Josh Soref <jsoref@gmail.com>
>
> It's somewhat traditional to respect sites' self-identification.
I don't understand this comment; was it meant for patch 1?
[...]
> --- a/json-writer.h
> +++ b/json-writer.h
> @@ -4,7 +4,7 @@
> /*
> * JSON data structures are defined at:
> * [1] https://www.ietf.org/rfc/rfc7159.txt
> - * [2] http://json.org/
> + * [2] https://www.json.org/
> *
> * The JSON-writer API allows one to build JSON data structures using a
> * simple wrapper around a "struct strbuf" buffer. It is intended as a
Ah, you did fix the http/https thing for json, you just moved it to
patch 2 because you also added the 'www.'. Got it.
^ permalink raw reply
* Re: [PATCH v2 1/4] doc: switch links to https
From: Elijah Newren @ 2023-11-23 21:19 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <71ed1286d7f38ecc901b40a542508fba9777f02d.1695553042.git.gitgitgadget@gmail.com>
On Sun, Sep 24, 2023 at 5:53 AM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
[...]
> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> index 34b1d6e2243..1edabdfef36 100644
> --- a/Documentation/gitweb.conf.txt
> +++ b/Documentation/gitweb.conf.txt
> @@ -820,7 +820,7 @@ filesystem (i.e. "$projectroot/$project"), `%h` to the current hash
> (\'h' gitweb parameter) and `%b` to the current hash base
> (\'hb' gitweb parameter); `%%` expands to \'%'.
> +
> -For example, at the time this page was written, the http://repo.or.cz[]
> +For example, at the time this page was written, the https://repo.or.cz[]
Given the "at the time this page was written" comment, I'm not sure we
should switch to https here.
> Git hosting site set it to the following to enable graphical log
> (using the third party tool *git-browser*):
> +
> diff --git a/Documentation/gitweb.txt b/Documentation/gitweb.txt
> index af6bf3c45ec..434893595a4 100644
> --- a/Documentation/gitweb.txt
> +++ b/Documentation/gitweb.txt
> @@ -28,7 +28,7 @@ Gitweb provides a web interface to Git repositories. Its features include:
> revisions one at a time, viewing the history of the repository.
> * Finding commits which commit messages matches given search term.
>
> -See http://repo.or.cz/w/git.git/tree/HEAD:/gitweb/[] for gitweb source code,
> +See https://repo.or.cz/w/git.git/tree/HEAD:/gitweb/[] for gitweb source code,
> browsed using gitweb itself.
The suggested link gives a "404 - No such tree". Granted, the http:
link also does that, but it'd be nicer to provide a non-broken link,
which you can do by stripping the '/[]' from the end of the URL.
> diff --git a/json-writer.h b/json-writer.h
> index 209355e0f12..de140e54c98 100644
> --- a/json-writer.h
> +++ b/json-writer.h
> @@ -3,7 +3,7 @@
>
> /*
> * JSON data structures are defined at:
> - * [1] http://www.ietf.org/rfc/rfc7159.txt
> + * [1] https://www.ietf.org/rfc/rfc7159.txt
> * [2] http://json.org/
As Eric commented on v1, the json.org link should also be converted to https.
The rest of the patch that I didn't comment on looks fine.
^ permalink raw reply
* Re: [PATCH v7 00/14] Introduce new `git replay` command
From: Elijah Newren @ 2023-11-23 19:32 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Christian Couder, git, Junio C Hamano, Patrick Steinhardt,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <fb6eb685-0af1-082a-b20c-028b06b6914e@gmx.de>
Hi Christian,
On Thu, Nov 16, 2023 at 12:53 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 15 Nov 2023, Christian Couder wrote:
>
> > # Range-diff between v6 and v7
> >
[...]
> Apart from the one little outstanding nit where I would love to see
> `(EXPERIMENTAL!)` as the first word of the synopsis both in the manual
> page and in the output of `git replay -h`, you have addressed all of my
> concerns.
>
> Thank you!
> Johannes
Looks good to me too. Thanks!
^ permalink raw reply
* Re: [PATCH 0/4] Redact unsafe URLs in the Trace2 output
From: Elijah Newren @ 2023-11-23 19:08 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1616.git.1700680717.gitgitgadget@gmail.com>
On Wed, Nov 22, 2023 at 11:18 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> The Trace2 output can contain secrets when a user issues a Git command with
> sensitive information in the command-line. A typical (if highly discouraged)
> example is: git clone https://user:password@host.com/.
>
> With this PR, the Trace2 output redacts passwords in such URLs by default.
>
> This series also includes a commit to temporarily disable leak checking on
> t0210,t0211 because the tests uncover other unrelated bugs in Git.
>
> These patches were integrated into Microsoft's fork of Git, as
> https://github.com/microsoft/git/pull/616, and have been cooking there ever
> since.
Thanks for making these changes. Makes me wonder, back when we were
logging trace2 data, if we had some of these leaks. Eek.
As I commented in patch 2, I think this is a good start, but I'm
curious if others would be willing to turn clone/fetch of such bad
URLs into warnings for now and errors later. The prevalence of
AI-assist add-ons for various IDEs and the number of developers opting
to use those IDEs and add-ons, and the fact that these tools sometimes
include repository URLs in what they send off to third parties, makes
me wonder if our recent infosec fire drill is soon going to be a
widely shared experience by lots of other companies and individuals.
Training users to not do bad things is hard, and it might be worth
saving them from themselves. Thoughts?
^ permalink raw reply
* Re: [PATCH 2/4] trace2: redact passwords from https:// URLs by default
From: Elijah Newren @ 2023-11-23 18:59 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <a1686ab52f1bec4bddeaab973c9b77e55e8b539b.1700680717.git.gitgitgadget@gmail.com>
On Wed, Nov 22, 2023 at 11:19 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is an unsafe practice to call something like
>
> git clone https://user:password@example.com/
>
> This not only risks leaking the password "over the shoulder" or into the
> readline history of the current Unix shell, it also gets logged via
> Trace2 if enabled.
Indeed. Clone urls _also_ seem to be slurped up by other tools, such
as IDEs, and possibly sent off to various third-party cloud services
when users have various AI-assist plugins installed in their IDEs,
resulting in some infosec incidents and fire drills. (Not a
theoretical scenario, and not fun.)
> Let's at least avoid logging such secrets via Trace2, much like we avoid
> logging secrets in `http.c`. Much like the code in `http.c` is guarded
> via `GIT_TRACE_REDACT` (defaulting to `true`), we guard the new code via
> `GIT_TRACE2_REDACT` (also defaulting to `true`).
Training users is hard. I appreciate the changes here to make trace2
not be a leak vector, but is it time to perhaps consider bigger safety
measures: At the clone/fetch level, automatically warn loudly whenever
such a URL is provided, accompanied with a note that in the future it
will be turned into a hard error?
Either way, I agree with your "at least" comment here and the changes
you are making.
> The new tests added in this commit uncover leaks in `builtin/clone.c`
> and `remote.c`. Therefore we need to turn off
> `TEST_PASSES_SANITIZE_LEAK`. The reasons:
>
> - We observed that `the_repository->remote_status` is not released
> properly.
>
> - We are using `url...insteadOf` and that runs into a code path where an
> allocated URL is replaced with another URL, and the original URL is
> never released.
>
> - `remote_states` contains plenty of `struct remote`s whose refspecs
> seem to be usually allocated by never released.
>
> More investigation is needed here to identify the exact cause and
> proper fixes for these leaks/bugs.
Thanks for carefully documenting and explaining.
> Co-authored-by: Jeff Hostetler <jeffhostetler@github.com>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> t/t0210-trace2-normal.sh | 20 ++++++-
> trace2.c | 120 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 136 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
> index 80e76a4695e..c312657a12c 100755
> --- a/t/t0210-trace2-normal.sh
> +++ b/t/t0210-trace2-normal.sh
> @@ -2,7 +2,7 @@
>
> test_description='test trace2 facility (normal target)'
>
> -TEST_PASSES_SANITIZE_LEAK=true
> +TEST_PASSES_SANITIZE_LEAK=false
> . ./test-lib.sh
>
> # Turn off any inherited trace2 settings for this test.
> @@ -283,4 +283,22 @@ test_expect_success 'using global config with include' '
> test_cmp expect actual
> '
>
> +test_expect_success 'unsafe URLs are redacted by default' '
> + test_when_finished \
> + "rm -r trace.normal unredacted.normal clone clone2" &&
> +
> + test_config_global \
> + "url.$(pwd).insteadOf" https://user:pwd@example.com/ &&
> + test_config_global trace2.configParams "core.*,remote.*.url" &&
> +
> + GIT_TRACE2="$(pwd)/trace.normal" \
> + git clone https://user:pwd@example.com/ clone &&
> + ! grep user:pwd trace.normal &&
> +
> + GIT_TRACE2_REDACT=0 GIT_TRACE2="$(pwd)/unredacted.normal" \
> + git clone https://user:pwd@example.com/ clone2 &&
> + grep "start .* clone https://user:pwd@example.com" unredacted.normal &&
> + grep "remote.origin.url=https://user:pwd@example.com" unredacted.normal
> +'
> +
> test_done
> diff --git a/trace2.c b/trace2.c
> index 6dc74dff4c7..87d9a3a0361 100644
> --- a/trace2.c
> +++ b/trace2.c
> @@ -20,6 +20,7 @@
> #include "trace2/tr2_tmr.h"
>
> static int trace2_enabled;
> +static int trace2_redact = 1;
>
> static int tr2_next_child_id; /* modify under lock */
> static int tr2_next_exec_id; /* modify under lock */
> @@ -227,6 +228,8 @@ void trace2_initialize_fl(const char *file, int line)
> if (!tr2_tgt_want_builtins())
> return;
> trace2_enabled = 1;
> + if (!git_env_bool("GIT_TRACE2_REDACT", 1))
> + trace2_redact = 0;
>
> tr2_sid_get();
>
> @@ -247,12 +250,93 @@ int trace2_is_enabled(void)
> return trace2_enabled;
> }
>
> +/*
> + * Redacts an argument, i.e. ensures that no password in
> + * https://user:password@host/-style URLs is logged.
> + *
> + * Returns the original if nothing needed to be redacted.
> + * Returns a pointer that needs to be `free()`d otherwise.
> + */
> +static const char *redact_arg(const char *arg)
> +{
> + const char *p, *colon;
> + size_t at;
> +
> + if (!trace2_redact ||
> + (!skip_prefix(arg, "https://", &p) &&
> + !skip_prefix(arg, "http://", &p)))
> + return arg;
> +
> + at = strcspn(p, "@/");
> + if (p[at] != '@')
> + return arg;
> +
> + colon = memchr(p, ':', at);
> + if (!colon)
> + return arg;
> +
> + return xstrfmt("%.*s:<REDACTED>%s", (int)(colon - arg), arg, p + at);
> +}
> +
> +/*
> + * Redacts arguments in an argument list.
> + *
> + * Returns the original if nothing needed to be redacted.
> + * Otherwise, returns a new array that needs to be released
> + * via `free_redacted_argv()`.
> + */
> +static const char **redact_argv(const char **argv)
> +{
> + int i, j;
> + const char *redacted = NULL;
> + const char **ret;
> +
> + if (!trace2_redact)
> + return argv;
> +
> + for (i = 0; argv[i]; i++)
> + if ((redacted = redact_arg(argv[i])) != argv[i])
> + break;
> +
> + if (!argv[i])
> + return argv;
> +
> + for (j = 0; argv[j]; j++)
> + ; /* keep counting */
> +
> + ALLOC_ARRAY(ret, j + 1);
> + ret[j] = NULL;
> +
> + for (j = 0; j < i; j++)
> + ret[j] = argv[j];
> + ret[i] = redacted;
> + for (++i; argv[i]; i++) {
> + redacted = redact_arg(argv[i]);
> + ret[i] = redacted ? redacted : argv[i];
> + }
> +
> + return ret;
> +}
> +
> +static void free_redacted_argv(const char **redacted, const char **argv)
> +{
> + int i;
> +
> + if (redacted != argv) {
> + for (i = 0; argv[i]; i++)
> + if (redacted[i] != argv[i])
> + free((void *)redacted[i]);
> + free((void *)redacted);
> + }
> +}
> +
> void trace2_cmd_start_fl(const char *file, int line, const char **argv)
> {
> struct tr2_tgt *tgt_j;
> int j;
> uint64_t us_now;
> uint64_t us_elapsed_absolute;
> + const char **redacted;
>
> if (!trace2_enabled)
> return;
> @@ -260,10 +344,14 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)
> us_now = getnanotime() / 1000;
> us_elapsed_absolute = tr2tls_absolute_elapsed(us_now);
>
> + redacted = redact_argv(argv);
> +
> for_each_wanted_builtin (j, tgt_j)
> if (tgt_j->pfn_start_fl)
> tgt_j->pfn_start_fl(file, line, us_elapsed_absolute,
> - argv);
> + redacted);
> +
> + free_redacted_argv(redacted, argv);
> }
>
> void trace2_cmd_exit_fl(const char *file, int line, int code)
> @@ -409,6 +497,7 @@ void trace2_child_start_fl(const char *file, int line,
> int j;
> uint64_t us_now;
> uint64_t us_elapsed_absolute;
> + const char **orig_argv = cmd->args.v;
>
> if (!trace2_enabled)
> return;
> @@ -419,10 +508,24 @@ void trace2_child_start_fl(const char *file, int line,
> cmd->trace2_child_id = tr2tls_locked_increment(&tr2_next_child_id);
> cmd->trace2_child_us_start = us_now;
>
> + /*
> + * The `pfn_child_start_fl` API takes a `struct child_process`
> + * rather than a simple `argv` for the child because some
> + * targets make use of the additional context bits/values. So
> + * temporarily replace the original argv (inside the `strvec`)
> + * with a possibly redacted version.
> + */
> + cmd->args.v = redact_argv(orig_argv);
> +
> for_each_wanted_builtin (j, tgt_j)
> if (tgt_j->pfn_child_start_fl)
> tgt_j->pfn_child_start_fl(file, line,
> us_elapsed_absolute, cmd);
> +
> + if (cmd->args.v != orig_argv) {
> + free_redacted_argv(cmd->args.v, orig_argv);
> + cmd->args.v = orig_argv;
> + }
> }
>
> void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,
> @@ -493,6 +596,7 @@ int trace2_exec_fl(const char *file, int line, const char *exe,
> int exec_id;
> uint64_t us_now;
> uint64_t us_elapsed_absolute;
> + const char **redacted;
>
> if (!trace2_enabled)
> return -1;
> @@ -502,10 +606,14 @@ int trace2_exec_fl(const char *file, int line, const char *exe,
>
> exec_id = tr2tls_locked_increment(&tr2_next_exec_id);
>
> + redacted = redact_argv(argv);
> +
> for_each_wanted_builtin (j, tgt_j)
> if (tgt_j->pfn_exec_fl)
> tgt_j->pfn_exec_fl(file, line, us_elapsed_absolute,
> - exec_id, exe, argv);
> + exec_id, exe, redacted);
> +
> + free_redacted_argv(redacted, argv);
>
> return exec_id;
> }
> @@ -637,13 +745,19 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
> {
> struct tr2_tgt *tgt_j;
> int j;
> + const char *redacted;
>
> if (!trace2_enabled)
> return;
>
> + redacted = redact_arg(value);
> +
> for_each_wanted_builtin (j, tgt_j)
> if (tgt_j->pfn_param_fl)
> - tgt_j->pfn_param_fl(file, line, param, value, kvi);
> + tgt_j->pfn_param_fl(file, line, param, redacted, kvi);
> +
> + if (redacted != value)
> + free((void *)redacted);
> }
>
> void trace2_def_repo_fl(const char *file, int line, struct repository *repo)
> --
> gitgitgadget
^ permalink raw reply
* Re: Migration of git-scm.com to a static web site: ready for review/testing
From: Kaartic Sivaraam @ 2023-11-23 18:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Matt Burke, Victoria Dye, Matthias Aßhauer, git
In-Reply-To: <6f7d20b4-a725-0ef9-f6d3-ff2810da9e7a@gmx.de>
Hi Johannes,
On 17/11/23 18:55, Johannes Schindelin wrote:
>
> To that end, I deployed this branch to GitHub Pages so that anyone
> interested (hopefully many!) can have a look at
> https://git.github.io/git-scm.com/ and compare to the existing
> https://git-scm.com/.
>
Thanks for hosting it to easily check things!
I gave a quick try at the search and it seems to be behaving a bit
strangely.
For instance, I searched for 'commit' and 'log'. I was hoping to see the
corresponding reference page show up in the results but they don't seem
to show up. At least they don't show up in the first few results. They
show up in the first few results in the existing site.
Here are some screenshots:
Existing site: https://ibb.co/pZHx9TM
New site:
https://ibb.co/dMpNth3
https://ibb.co/h26J5Rx
This not always the case, though. Some terms like 'checkout' seem to
bring the relevant results properly in the top results.
--
Sivaraam
^ permalink raw reply
* [PATCH 4/4] completion: avoid user confusion in non-cone mode
From: Elijah Newren via GitGitGadget @ 2023-11-23 17:44 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
In-Reply-To: <pull.1349.git.1700761448.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
It is tempting to think of "files and directories" of the current
directory as valid inputs to the add and set subcommands of git
sparse-checkout. However, in non-cone mode, they often aren't and using
them as potential completions leads to *many* forms of confusion:
Issue #1. It provides the *wrong* files and directories.
For
git sparse-checkout add
we always want to add files and directories not currently in our sparse
checkout, which means we want file and directories not currently present
in the current working tree. Providing the files and directories
currently present is thus always wrong.
For
git sparse-checkout set
we have a similar problem except in the subset of cases where we are
trying to narrow our checkout to a strict subset of what we already
have. That is not a very common scenario, especially since it often
does not even happen to be true for the first use of the command; for
years we required users to create a sparse-checkout via
git sparse-checkout init
git sparse-checkout set <args...>
(or use a clone option that did the init step for you at clone time).
The init command creates a minimal sparse-checkout with just the
top-level directory present, meaning the set command has to be used to
expand the checkout. Thus, only in a special and perhaps unusual cases
would any of the suggestions from normal file and directory completion
be appropriate.
Issue #2: Suggesting patterns that lead to warnings is unfriendly.
If the user specifies any regular file and omits the leading '/', then
the sparse-checkout command will warn the user that their command is
problematic and suggest they use a leading slash instead.
Issue #3: Completion gets confused by leading '/', and provides wrong paths.
Users often want to anchor their patterns to the toplevel of the
repository, especially when listing individual files. There are a
number of reasons for this, but notably even sparse-checkout encourages
them to do so (as noted above). However, if users do so (via adding a
leading '/' to their pattern), then bash completion will interpret the
leading slash not as a request for a path at the toplevel of the
repository, but as a request for a path at the root of the filesytem.
That means at best that completion cannot help with such paths, and if
it does find any completions, they are almost guaranteed to be wrong.
Issue #4: Suggesting invalid patterns from subdirectories is unfriendly.
There is no per-directory equivalent to .gitignore with
sparse-checkouts. There is only a single worktree-global
$GIT_DIR/info/sparse-checkout file. As such, paths to files must be
specified relative to the toplevel of a repository. Providing
suggestions of paths that are relative to the current working directory,
as bash completion defaults to, is wrong when the current working
directory is not the worktree toplevel directory.
Issue #5: Paths with special characters will be interpreted incorrectly
The entries in the sparse-checkout file are patterns, not paths. While
most paths also qualify as patterns (though even in such cases it would
be better for users to not use them directly but prefix them with a
leading '/'), there are a variety of special characters that would need
special escaping beyond the normal shell escaping: '*', '?', '\', '[',
']', and any leading '#' or '!'. If completion suggests any such paths,
users will likely expect them to be treated as an exact path rather than
as a pattern that might match some number of files other than 1.
Because of the combination of the above issues, turn completion off for
the `set` and `add` subcommands of `sparse-checkout` when in non-cone
mode, but leave a NEEDSWORK comment specifying what could theoretically
be done if someone wanted to provide completion rules that were more
helpful than harmful.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
contrib/completion/git-completion.bash | 61 ++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 136faeca1e9..7d460da2fab 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3068,6 +3068,67 @@ _git_sparse_checkout ()
fi
if [[ "$using_cone" == "true" ]]; then
__gitcomp_directories
+ else
+ # NEEDSWORK: It might be useful to provide a
+ # completion function which:
+ #
+ # 1. Provides completions based on
+ # files/directories that exist in HEAD, not
+ # just those currently present in the working
+ # tree. Bash's default file and directory
+ # completion is totally useless for "git
+ # sparse-checkout add" because of this. It is
+ # likewise problematic for "git
+ # sparse-checkout set" except in those subset
+ # of cases when trying to narrow scope to a
+ # strict subset of what you already have
+ # checked out.
+ #
+ # 2. Always provides file/directory completions
+ # with a prepended leading '/', so that
+ # files/directories are only searched at the
+ # relevant level rather than throughout all
+ # trees in the hierarchy. Doing this also
+ # avoids suggesting the user run a
+ # sparse-checkout command that will result in
+ # a warning be thrown at the user.
+ #
+ # 3. Does not accidentally search the root of
+ # the filesystem when a path with a leading
+ # slash is specified. ("git sparse-checkout
+ # add /ho<TAB>" should not complete to
+ # "/home" but to e.g. "/hooks" if there is a
+ # "hooks" in the top of the repository.)
+ #
+ # 4. Provides no completions when run from a
+ # subdirectory of the repository root. (If we
+ # did provide file/directory completions, the
+ # user would just get a "please run from the
+ # toplevel directory" error message when they
+ # ran it. *Further*, if the user did rerun
+ # the command from the toplevel, the
+ # completions we previously provided would
+ # likely be wrong as they'd be relative to the
+ # subdirectory rather than the repository
+ # root. That could lead to users getting a
+ # nasty surprise based on trying to use a
+ # command we helped them create.)
+ #
+ # 5. Provides escaped completions for any paths
+ # containing a '*', '?', '\', '[', ']', or
+ # leading '#' or '!'. (These characters might
+ # already be escaped to protect from the
+ # shell, but they need an *extra* layer of
+ # escaping to prevent the pattern parsing in
+ # Git from seeing them as special characters.)
+ #
+ # Of course, this would be a lot of work, so for now,
+ # just avoid the many forms of user confusion that
+ # could be caused by providing bad completions by
+ # providing a fake completion to avoid falling back to
+ # bash's normal file and directory completion.
+
+ COMPREPLY=( "" )
fi
esac
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/4] completion: avoid misleading completions in cone mode
From: Elijah Newren via GitGitGadget @ 2023-11-23 17:44 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
In-Reply-To: <pull.1349.git.1700761448.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
The "set" and "add" subcommands of "sparse-checkout", when in cone mode,
should only complete on directories. For bash_completion in general,
when no completions are returned for any subcommands, it will often fall
back to standard completion of files and directories as a substitute.
That is not helpful here. Since we have already looked for all valid
completions, if none are found then falling back to standard bash file
and directory completion is at best actively misleading. In fact, there
are three different ways it can be actively misleading. Add a long
comment in the code about how that fallback behavior can deceive, and
disable the fallback by returning a fake result as the sole completion.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 42e9e0cba8f..136faeca1e9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3020,6 +3020,26 @@ __gitcomp_directories ()
# No possible further completions any deeper, so assume we're at
# a leaf directory and just consider it complete
__gitcomp_direct_append "$cur "
+ elif [[ $_found == 0 ]]; then
+ # No possible completions found. Avoid falling back to
+ # bash's default file and directory completion, because all
+ # valid completions have already been searched and the
+ # fallbacks can do nothing but mislead. In fact, they can
+ # mislead in three different ways:
+ # 1) Fallback file completion makes no sense when asking
+ # for directory completions, as this function does.
+ # 2) Fallback directory completion is bad because
+ # e.g. "/pro" is invalid and should NOT complete to
+ # "/proc".
+ # 3) Fallback file/directory completion only completes
+ # on paths that exist in the current working tree,
+ # i.e. which are *already* part of their
+ # sparse-checkout. Thus, normal file and directory
+ # completion is always useless for "git
+ # sparse-checkout add" and is also probelmatic for
+ # "git sparse-checkout set" unless using it to
+ # strictly narrow the checkout.
+ COMPREPLY=( "" )
fi
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/4] completion: fix logic for determining whether cone mode is active
From: Elijah Newren via GitGitGadget @ 2023-11-23 17:44 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
In-Reply-To: <pull.1349.git.1700761448.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
_git_sparse_checkout() was checking whether we were in cone mode by
checking whether either:
A) core.sparseCheckoutCone was "true"
B) "--cone" was specified on the command line
This code has 2 bugs I didn't catch in my review at the time
1) core.sparseCheckout must be "true" for core.sparseCheckoutCone to
be relevant (which matters since "git sparse-checkout disable"
only unsets core.sparseCheckout, not core.sparseCheckoutCone)
2) The presence of "--no-cone" should override any config setting
Further, I forgot to update this logic as part of 2d95707a02
("sparse-checkout: make --cone the default", 2022-04-22) for the new
default.
Update the code for the new default and make it be more careful in
determining whether to complete based on cone mode or non-cone mode.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
contrib/completion/git-completion.bash | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6fced40d04c..42e9e0cba8f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3027,6 +3027,7 @@ _git_sparse_checkout ()
{
local subcommands="list init set disable add reapply"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ local using_cone=true
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
return
@@ -3037,8 +3038,15 @@ _git_sparse_checkout ()
__gitcomp_builtin sparse-checkout_$subcommand "" "--"
;;
set,*|add,*)
- if [ "$(__git config core.sparseCheckoutCone)" == "true" ] ||
- [ -n "$(__git_find_on_cmdline --cone)" ]; then
+ if [[ "$(__git config core.sparseCheckout)" == "true" &&
+ "$(__git config core.sparseCheckoutCone)" == "false" &&
+ -z "$(__git_find_on_cmdline --cone)" ]]; then
+ using_cone=false
+ fi
+ if [[ -n "$(__git_find_on_cmdline --no-cone)" ]]; then
+ using_cone=false
+ fi
+ if [[ "$using_cone" == "true" ]]; then
__gitcomp_directories
fi
esac
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/4] completion: squelch stray errors in sparse-checkout completion
From: Elijah Newren via GitGitGadget @ 2023-11-23 17:44 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
In-Reply-To: <pull.1349.git.1700761448.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
If, in the root of a project, one types
git sparse-checkout set --cone ../<TAB>
then an error message of the form
fatal: ../: '../' is outside repository at '/home/newren/floss/git'
is written to stderr, which munges the users view of their own command.
Squelch such messages.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ba5c395d2d8..6fced40d04c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3014,7 +3014,7 @@ __gitcomp_directories ()
COMPREPLY+=("$c/")
_found=1
fi
- done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir)
+ done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir 2>/dev/null)
if [[ $_found == 0 ]] && [[ "$cur" =~ /$ ]]; then
# No possible further completions any deeper, so assume we're at
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/4] Sparse checkout completion fixes
From: Elijah Newren via GitGitGadget @ 2023-11-23 17:44 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
This fixes a few issues with tab completion for the sparse-checkout command,
specifically with the "add" and "set" subcommands.
The 4th patch should probably be considered RFC; it is notable in that we
previously discussed a much different proposal and the general problem
area[1], though our discussion was from a limited vantage point and none of
us (myself included) were aware of the full context at the time. In that
thread, Junio gave some helpful general high-level guidelines for
completion. I believe the existing completion rules fail Junio's guidelines
pretty badly and that we thus need to do something else. See the lengthy
commit message. I implement a simple though somewhat suboptimal choice for
that something else (while arguing that it's at least much better than our
current solution), while also documenting with a code comment a much more
involved alternative solution that we could consider in the future. Comments
on this choice welcome.
[1] https://lore.kernel.org/git/xmqqv8yjz5us.fsf@gitster.g/
Elijah Newren (4):
completion: squelch stray errors in sparse-checkout completion
completion: fix logic for determining whether cone mode is active
completion: avoid misleading completions in cone mode
completion: avoid user confusion in non-cone mode
contrib/completion/git-completion.bash | 95 +++++++++++++++++++++++++-
1 file changed, 92 insertions(+), 3 deletions(-)
base-commit: 79f2338b3746d23454308648b2491e5beba4beff
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1349%2Fnewren%2Fsparse-checkout-completion-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1349/newren/sparse-checkout-completion-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1349
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere
From: Eric Sunshine @ 2023-11-23 17:09 UTC (permalink / raw)
To: phillip.wood; +Cc: Junio C Hamano, Willem Verstraeten, git
In-Reply-To: <bf848477-b4dd-49d3-8e4b-de0fc3948570@gmail.com>
On Thu, Nov 23, 2023 at 11:33 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 23/11/2023 06:00, Junio C Hamano wrote:
> > "git checkout -B <branch> [<start-point>]", being a "forced" version
> > of "-b", switches to the <branch>, after optionally resetting its
> > tip to the <start-point>, even if the <branch> is in use in another
> > worktree, which is somewhat unexpected.
> >
> > Protect the <branch> using the same logic that forbids "git checkout
> > <branch>" from touching a branch that is in use elsewhere.
> >
> > This is a breaking change that may deserve backward compatibliity
> > warning in the Release Notes. The "--ignore-other-worktrees" option
> > can be used as an escape hatch if the finger memory of existing
> > users depend on the current behaviour of "-B".
>
> I think this change makes sense and I found the implementation here much
> easier to understand than a previous attempt at
> https://lore.kernel.org/git/20230120113553.24655-1-carenas@gmail.com/
Thanks for digging up this link. Upon reading the problem report, I
felt certain that we had seen this issue reported previously and that
patches had been proposed, but I was unable to find the conversation
(despite having taken part in it).
I agree, also, that this two-patch series is simple to digest.
^ permalink raw reply
* Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere
From: Phillip Wood @ 2023-11-23 16:33 UTC (permalink / raw)
To: Junio C Hamano, Willem Verstraeten; +Cc: git
In-Reply-To: <xmqqpm01au0w.fsf_-_@gitster.g>
Hi Junio
On 23/11/2023 06:00, Junio C Hamano wrote:
> "git checkout -B <branch> [<start-point>]", being a "forced" version
> of "-b", switches to the <branch>, after optionally resetting its
> tip to the <start-point>, even if the <branch> is in use in another
> worktree, which is somewhat unexpected.
>
> Protect the <branch> using the same logic that forbids "git checkout
> <branch>" from touching a branch that is in use elsewhere.
>
> This is a breaking change that may deserve backward compatibliity
> warning in the Release Notes. The "--ignore-other-worktrees" option
> can be used as an escape hatch if the finger memory of existing
> users depend on the current behaviour of "-B".
I think this change makes sense and I found the implementation here much
easier to understand than a previous attempt at
https://lore.kernel.org/git/20230120113553.24655-1-carenas@gmail.com/
> Reported-by: Willem Verstraeten <willem.verstraeten@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * The documentation might also need updates, but I didn't look at.
This option is documented as an atomic version of
git branch -f <branch> [<start-point>]
git checkout <branch>
However "git branch -f <branch>" will fail if the branch is checked out
in the current worktree whereas "git checkout -B" succeeds. I think
allowing the checkout in that case makes sense for "git checkout -B" but
it does mean that description is not strictly accurate. I'm not sure it
matters that much though.
The documentation for "switch -C" is a bit lacking compared to "checkout
-B" but that is a separate problem.
>
> builtin/checkout.c | 7 +++++++
> t/t2060-switch.sh | 2 ++
> t/t2400-worktree-add.sh | 8 ++++++++
> 3 files changed, 17 insertions(+)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b4ab972c5a..8a8ad23e98 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1600,6 +1600,13 @@ static int checkout_branch(struct checkout_opts *opts,
> if (new_branch_info->path && !opts->force_detach && !opts->new_branch)
> die_if_switching_to_a_branch_in_use(opts, new_branch_info->path);
>
> + /* "git checkout -B <branch>" */
> + if (opts->new_branch_force) {
> + char *full_ref = xstrfmt("refs/heads/%s", opts->new_branch);
> + die_if_switching_to_a_branch_in_use(opts, full_ref);
> + free(full_ref);
At the moment this is academic as neither of the test scripts changed by
this patch are leak free and so I don't think we need to worry about it
but it raises an interesting question about how we should handle memory
leaks when dying. Leaving the leak when dying means that a test script
that tests an expected failure will never be leak free but using
UNLEAK() would mean we miss a leak being introduced in the successful
case should the call to "free()" ever be removed. We could of course
rename die_if_checked_out() to error_if_checked_out() and return an
error instead of dying but that seems like a lot of churn just to keep
the leak checker happy.
Best Wishes
Phillip
> + }
> +
> if (!new_branch_info->commit && opts->new_branch) {
> struct object_id rev;
> int flag;
> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
> index e247a4735b..c91c4db936 100755
> --- a/t/t2060-switch.sh
> +++ b/t/t2060-switch.sh
> @@ -170,8 +170,10 @@ test_expect_success 'switch back when temporarily detached and checked out elsew
> # we test in both worktrees to ensure that works
> # as expected with "first" and "next" worktrees
> test_must_fail git -C wt1 switch shared &&
> + test_must_fail git -C wt1 switch -C shared &&
> git -C wt1 switch --ignore-other-worktrees shared &&
> test_must_fail git -C wt2 switch shared &&
> + test_must_fail git -C wt2 switch -C shared &&
> git -C wt2 switch --ignore-other-worktrees shared
> '
>
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index df4aff7825..bbcb2d3419 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -126,6 +126,14 @@ test_expect_success 'die the same branch is already checked out' '
> )
> '
>
> +test_expect_success 'refuse to reset a branch in use elsewhere' '
> + (
> + cd here &&
> + test_must_fail git checkout -B newmain 2>actual &&
> + grep "already used by worktree at" actual
> + )
> +'
> +
> test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
> head=$(git -C there rev-parse --git-path HEAD) &&
> ref=$(git -C there symbolic-ref HEAD) &&
^ permalink raw reply
* Re: [PATCH] object-name: reject too-deep recursive ancestor queries
From: Patrick Steinhardt @ 2023-11-23 13:53 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jeff King, Junio C Hamano,
Carlos Andrés Ramírez Cataño
In-Reply-To: <57c0b30ddfe7c0ae78069682ff8454791e54469f.1700496801.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 9200 bytes --]
On Mon, Nov 20, 2023 at 11:13:45AM -0500, Taylor Blau wrote:
> When trying to resolve a revision query like "HEAD~~~~~", our call
> pattern looks something like:
>
> - object-name.c::get_oid_with_context()
> - object-name.c::get_oid_1()
> - object-name.c::get_nth_ancestor()
> - object-name.c::get_oid_1()
> - ...
>
> With `get_nth_ancestor()` and `get_oid_1()` mutually recurring, popping
> one '~' off of the revision query for each round of the recursion.
One thing I noticed just now is that we have exactly the same problem
with `^`, just with a different callstack. This problem isn't yet
addressed by your patch.
I have to wonder whether we should tighten restrictions even further:
instead of manually keeping track of how deep in the stack we are, we
limit the length of revisions to at most 1MB. I would claim that this
limit is sufficiently large to never be a problem in practice. Revisions
are limited to 4kB on most platforms anyway due to the maximum path
length. And if somebody really wants to request the (1024 * 1024) + 1th
parent, they shouldn't do that by appending this many "~" or "^" chars,
but instead they should ask for "~1048577" or "^1048577".
I realize that this is much more restrictive than the current patch. But
it would be a good defensive mechanism against all kinds of weird revs,
and I am very certain that there are other ways to blow the stack or
cause out-of-bounds reads or writes here.
Patrick
> Since this recursive behavior is unbounded, having too many "~"'s
> contained in a revision query will cause us to blow the stack.
> Generating a message like this when compiled under SANITIZE=address:
>
> $ valgrind git rev-parse "HEAD$(perl -e "print \"~\" x 1000000000000")"
> ==597453== Memcheck, a memory error detector
> ==597453== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
> ==597453== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
> ==597453== Command: /home/ttaylorr/local/bin/git.compile diff HEAD~~~~~~~~~~~~[...]
> ==597453==
> AddressSanitizer:DEADLYSIGNAL
> =================================================================
> ==597453==ERROR: AddressSanitizer: stack-overflow on address 0x7fffdd838ff8 (pc 0x7f2726082748 bp 0x7fffdd839110 sp 0x7fffdd839000 T0)
> #0 0x7f2726082748 in __asan::GetTLSFakeStack() ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:176
> #1 0x7f2726082748 in GetFakeStackFast ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:193
> #2 0x7f27260833de in OnMalloc ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:207
> #3 0x7f27260833de in __asan_stack_malloc_1 ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:256
> #4 0x563f9077d9d8 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1087
> #5 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295
> #6 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092
> #7 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295
> #8 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092
> [...]
> #247 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295
> #248 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092
>
> SUMMARY: AddressSanitizer: stack-overflow ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:176 in __asan::GetTLSFakeStack()
> ==597453==ABORTING
>
> (Note that the actual stack is much deeper. GDB reports that the bottom
> of the stack looks something like the following):
>
> #54866 0x0000555555c6d3bf in get_oid_with_context_1 (repo=0x5555563849a0 <the_repo>, name=0x7fffffff4be5 "HEAD", '~' <repeats 196 times>..., flags=128, prefix=0x0, oid=0x7ffff5713d40, oc=0x7ffff5713d90) at object-name.c:1947
> #54867 0x0000555555c6e2fa in get_oid_with_context (repo=0x5555563849a0 <the_repo>, str=0x7fffffff4be5 "HEAD", '~' <repeats 196 times>..., flags=128, oid=0x7ffff5713d40, oc=0x7ffff5713d90) at object-name.c:2096
> #54868 0x0000555555d8eed8 in handle_revision_arg_1 (arg_=0x7fffffff4be5 "HEAD", '~' <repeats 196 times>..., revs=0x7ffff5b000d0, flags=0, revarg_opt=0) at revision.c:2174
> #54869 0x0000555555d8f1a9 in handle_revision_arg (arg=0x7fffffff4be5 "HEAD", '~' <repeats 196 times>..., revs=0x7ffff5b000d0, flags=0, revarg_opt=0) at revision.c:2189
> #54870 0x0000555555d97ca9 in setup_revisions (argc=2, argv=0x7fffffff4970, revs=0x7ffff5b000d0, opt=0x0) at revision.c:2932
> #54871 0x00005555557d6a63 in cmd_diff (argc=2, argv=0x7fffffff4970, prefix=0x0) at builtin/diff.c:502
> #54872 0x00005555557367bf in run_builtin (p=0x5555561c4c30 <commands+816>, argc=2, argv=0x7fffffff4970) at git.c:469
> #54873 0x000055555573716b in handle_builtin (argc=2, argv=0x7fffffff4970) at git.c:723
> #54874 0x000055555573785a in run_argv (argcp=0x7ffff56028b0, argv=0x7ffff56028e0) at git.c:787
> #54875 0x0000555555738626 in cmd_main (argc=2, argv=0x7fffffff4970) at git.c:922
> #54876 0x00005555559d3fdd in main (argc=3, argv=0x7fffffff4968) at common-main.c:62
>
> Fortunately, we can impose a limit on the maximum recursion depth we're
> willing to accept when resolving queries like the above without
> significantly impeding users. This patch sets the limit at 4096, though
> we could probably increase that limit depending on the size of each
> frame.
>
> The limit introduced here is large enough that any reasonable query
> should still run to completion, but small enough that if the frame size
> were to significantly increase, our protection would still be effective.
>
> The change here is straightforward: each call to get_nth_ancestor()
> increases a counter, and then decrements that counter before returning.
>
> The diff is a little noisy since there are a handful of return paths
> from `get_nth_ancestor()`, all of which need to decrement the depth
> variable.
>
> Since this is a local-only exploit, a user would have to be tricked into
> running such a query by an adversary. Even if they were successfully
> tricked into running the malicious query, the blast radius is limited to
> a local stack overflow, which does not have meaningful paths to remote
> code execution, arbitrary memory reads, or any more grave security
> concerns.
>
> Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> object-name.c | 26 ++++++++++++++++++++------
> t/t1506-rev-parse-diagnosis.sh | 5 +++++
> 2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/object-name.c b/object-name.c
> index 0bfa29dbbf..675e0a759e 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -1080,6 +1080,9 @@ static enum get_oid_result get_parent(struct repository *r,
> return MISSING_OBJECT;
> }
>
> +static int get_nth_ancestor_max_depth = 4096;
> +static int get_nth_ancestor_curr_depth;
> +
> static enum get_oid_result get_nth_ancestor(struct repository *r,
> const char *name, int len,
> struct object_id *result,
> @@ -1089,20 +1092,31 @@ static enum get_oid_result get_nth_ancestor(struct repository *r,
> struct commit *commit;
> int ret;
>
> + if (++get_nth_ancestor_curr_depth > get_nth_ancestor_max_depth)
> + return error(_("exceeded maximum ancestor depth"));
> +
> ret = get_oid_1(r, name, len, &oid, GET_OID_COMMITTISH);
> if (ret)
> - return ret;
> + goto done;
> commit = lookup_commit_reference(r, &oid);
> - if (!commit)
> - return MISSING_OBJECT;
> + if (!commit) {
> + ret = MISSING_OBJECT;
> + goto done;
> + }
>
> while (generation--) {
> - if (repo_parse_commit(r, commit) || !commit->parents)
> - return MISSING_OBJECT;
> + if (repo_parse_commit(r, commit) || !commit->parents) {
> + ret = MISSING_OBJECT;
> + goto done;
> + }
> commit = commit->parents->item;
> }
> oidcpy(result, &commit->object.oid);
> - return FOUND;
> +
> + ret = FOUND;
> +done:
> + get_nth_ancestor_curr_depth--;
> + return ret;
> }
>
> struct object *repo_peel_to_type(struct repository *r, const char *name, int namelen,
> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> index ef40511d89..b3b9f6c8c5 100755
> --- a/t/t1506-rev-parse-diagnosis.sh
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -244,6 +244,11 @@ test_expect_success 'reject Nth ancestor if N is too high' '
> test_must_fail git rev-parse HEAD~100000000000000000000000000000000
> '
>
> +test_expect_success 'reject too-deep recursive ancestor queries' '
> + test_must_fail git rev-parse "HEAD$(perl -e "print \"~\" x 4097")" 2>err &&
> + grep "error: exceeded maximum ancestor depth" err
> +'
> +
> test_expect_success 'pathspecs with wildcards are not ambiguous' '
> echo "*.c" >expect &&
> git rev-parse "*.c" >actual &&
>
> base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169
> --
> 2.43.0.rc2.19.geadd45bf00
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [BUG?] Semantics of HEAD when bare cloning non-bare repositories
From: guillaume.yziquel @ 2023-11-23 12:13 UTC (permalink / raw)
To: git
Hi.
Observation: When git clone --bare is used on a non-bare repo, the HEAD is put in ./refs/HEAD, and conflicts with ./HEAD in the sense that they may end up decorelated over time.
Suggestion: When bare cloning on a non-bare repo, do not put the HEAD of the non-bare repo in refs/HEAD like every branch, but treat it separately: put it in ./HEAD, not ./refs/HEAD.
Context: I was setting up an alternate repository for cargo crates, which requires a git repo to hold the index. So I wrote my config.json in a non-bare git repo, and "deployed" as a bare repo by git clone --bare. This created a ./refs/HEAD. However, the cargo crates repository I deployed (ktra) also updated the HEAD of that repo whenever I published private crates on it. So HEAD and refs/HEAD were not in sync. And, in the end, these got unsynchronised and I could not perform cargo fetch or cargo build with this alternate repo.
Ratonale: HEAD is just not a normal branch, semantically. It is what you have at the tip of your fingers in the shell with non-bare git repos. Treating as "just as all other branches" may be programatically convenient, but, in the end, it bypasses the fact that it just is not quite a normal branch. As the problem I experienced highlights. Which is why I would argue in favour of a distinguished treatment of HEADs when bare cloning a non-bare repo.
Best regards,
Guillaume Yziquel.
^ permalink raw reply
* Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox
From: Willem Verstraeten @ 2023-11-23 12:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzq9cl70.fsf@gitster.g>
> > git checkout -f main origin/main #also reports a fatal error, as expected
>
> This is expected because origin/main is taken as pathspec, and it is
> a request to checkout the paths that match the pathspec out of the
> named tree-ish (i.e., "main"), even when these paths have local
> changes, but you do not have paths that match "origin/main". The
> failure is not because "main" is checked out elsewhere.
>
My mistake: I meant to do `git branch -f main origin/main`, as
documented for `git checkout -B main origin/main`
So, for completeness' sake, this is my revised reproduction scenario
that actually demonstrates the problem I have:
~/temp> git clone https://github.com/servo/pathfinder.git primary
Cloning into 'primary'...
....
~/temp> cd primary
~/temp/primary> git worktree add -b metal ../secondary origin/metal
Preparing worktree (new branch 'metal')
branch 'metal' set up to track 'origin/metal'.
HEAD is now at 1cdcc209 wip
~/temp/primary> cd ..\secondary\
~/temp/secondary> git checkout main
fatal: 'main' is already used by worktree at ../primary'
~/temp/secondary> git branch -f main origin/main
fatal: cannot force update the branch 'main' used by worktree at
'../primary'
~/temp/secondary> git checkout -B main origin/main
Switched to and reset branch 'main'
branch 'main' set up to track 'origin/main'.
Your branch is up to date with 'origin/main'.
I would expect that last `git checkout -B ...` to fail with a similar
error as the `git branch -f ...` command right before that, since the
documentation for `git checkout -B <branch> <start-point>` states that
it is the atomic equivalent of `git branch -f <branch> <start-point> ;
git checkout <branch>`
> I guess we could change the behaviour so that
>
> git checkout -B <branch> [<start-point>]
>
> fails when <branch> is an existing branch that is in use in another
> worktree, and allow "-f" to be used to override the safety, i.e.,
>
> git checkout -f -B <branch> [<start-point>]
I would be very much in favor of that, indeed.
However, as you noted in your follow-up mail, the
--ignore-other-worktrees option would be better suited than the -f
flag.
> It turns out that for some reason "-f" is not how we decided to
> override this one---there is "--ignore-other-worktrees" option.
This means it would look like this then, if you decide to tackle this?
~/temp/secondary> git checkout -B metal origin/metal
fatal: cannot force update the branch 'main' used by worktree at
'../primary'
~/temp/secondary> git checkout --ignore-other-worktrees -B metal
origin/metal
Switched to and reset branch 'metal'
branch 'metal' set up to track 'origin/metal'.
Your branch is up to date with 'origin/metal'.
My thoughts as an experienced user, though not an experienced
contributor, admittedly :)
Kind regards,
Willem Verstraeten
On Thu, 23 Nov 2023 at 02:28, Junio C Hamano <gitster@pobox.com> wrote:
>
> Willem Verstraeten <willem.verstraeten@gmail.com> writes:
>
> > git checkout -b main #reports a fatal error, as expected
>
> This is expected because "main" already exists, not because "main"
> is checked out elsewhere.
>
> > git checkout -f main origin/main #also reports a fatal error, as expected
>
> This is expected because origin/main is taken as pathspec, and it is
> a request to checkout the paths that match the pathspec out of the
> named tree-ish (i.e., "main"), even when these paths have local
> changes, but you do not have paths that match "origin/main". The
> failure is not because "main" is checked out elsewhere.
>
> A slight variant of the command
>
> git checkout -f -b main origin/main
>
> still fails for the same reason as the first of your examples above.
>
> It is a tangent, but I suspect this failure may be a bit unexpected.
> In this example, "-f"orce could be overriding the usual failure from
> "-b" to switch to a branch that already exists, but that is what
> "-B" does, and "-f -b" does not work as a synonym for "-B".
>
> In any case, these example you marked "fail as expected" do fail as
> expected, but they fail for reasons that have nothing to do with the
> protection of branches that are used in other worktrees.
>
> > git checkout -B main origin/main # ----> this succeeds, which is
> > unexpected <----
>
> I agree this may be undesirable.
>
> But it makes sort of sense, because "-B" is a forced form of "-b"
> (i.e., it tells git: even when "-b" would fail, take necessary
> measures to make it work), and we can view that it is part of
> "forcing" to override the protection over branches that are used
> elsewhere.
>
> I guess we could change the behaviour so that
>
> git checkout -B <branch> [<start-point>]
>
> fails when <branch> is an existing branch that is in use in another
> worktree, and allow "-f" to be used to override the safety, i.e.,
>
> git checkout -f -B <branch> [<start-point>]
>
> would allow the <branch> to be repointed to <start-point> (or HEAD)
> even when it is used elsewhere.
>
> Thoughts, whether they agree or disagree with what I just said, by
> other experienced contributors are very much welcome, before I can
> say "patches welcome" ;-).
>
> Willem, thanks for raising the issue.
>
>
>
^ permalink raw reply
* Re: [PATCH v2] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Junio C Hamano @ 2023-11-23 11:44 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Jeff King
In-Reply-To: <f1235741cea5866e67c83aca83a760e0cdde8730.1700478031.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Note that this requires us to amend some tests to manually turn on the
> paranoid checks again. This is because we cause repository corruption by
> manually deleting objects which are part of the commit graph already.
> These circumstances shouldn't usually happen in repositories.
> ...
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 40265a4f66..1ca4eb5a36 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ b/t/t6022-rev-list-missing.sh
> @@ -13,6 +13,11 @@ test_expect_success 'create repository and alternate directory' '
> test_commit 3
> '
>
> +# We manually corrupt the repository, which means that the commit-graph may
> +# contain references to already-deleted objects. We thus need to enable
> +# commit-graph paranoia to not returned these deleted commits from the graph.
> +export GIT_COMMIT_GRAPH_PARANOIA=true
test-lint-shell-syntax is a bit overly strict and complains against
this line, so until it is loosened, I'd suggest to do
GIT_COMMIT_GRAPH_PARANOIA=true
export GIT_COMMIT_GRAPH_PARANOIA
instead here.
^ permalink raw reply
* Re: [PATCH 1/4] trace2: fix signature of trace2_def_param() macro
From: Junio C Hamano @ 2023-11-23 6:10 UTC (permalink / raw)
To: Jeff Hostetler via GitGitGadget; +Cc: git, Johannes Schindelin, Jeff Hostetler
In-Reply-To: <97d17c22ff310c26c3ec391c7bf870e7e5bab4f8.1700680717.git.gitgitgadget@gmail.com>
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Add `struct key_value_info` argument to `trace2_def_param()`.
>
> In dc90208497 (trace2: plumb config kvi, 2023-06-28) a `kvi`
> argument was added to `trace2_def_param_fl()` but the macro
> was not up updated. Let's fix that.
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
> trace2.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/trace2.h b/trace2.h
> index 40d8c2e02a5..1f0669bbd2d 100644
> --- a/trace2.h
> +++ b/trace2.h
> @@ -337,8 +337,8 @@ struct key_value_info;
> void trace2_def_param_fl(const char *file, int line, const char *param,
> const char *value, const struct key_value_info *kvi);
>
> -#define trace2_def_param(param, value) \
> - trace2_def_param_fl(__FILE__, __LINE__, (param), (value))
> +#define trace2_def_param(param, value, kvi) \
> + trace2_def_param_fl(__FILE__, __LINE__, (param), (value), (kvi))
IOW, this macro was not used back when it was updated, and nobody
used it since then?
I briefly wondered if we are better off removing it but that does
not make sense because you are adding a new (and only) user to it.
Will queue. Thanks.
>
> /*
> * Tell trace2 about a newly instantiated repo object and assign
^ permalink raw reply
* [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere
From: Junio C Hamano @ 2023-11-23 6:00 UTC (permalink / raw)
To: Willem Verstraeten; +Cc: git
In-Reply-To: <xmqqv89tau3r.fsf@gitster.g>
"git checkout -B <branch> [<start-point>]", being a "forced" version
of "-b", switches to the <branch>, after optionally resetting its
tip to the <start-point>, even if the <branch> is in use in another
worktree, which is somewhat unexpected.
Protect the <branch> using the same logic that forbids "git checkout
<branch>" from touching a branch that is in use elsewhere.
This is a breaking change that may deserve backward compatibliity
warning in the Release Notes. The "--ignore-other-worktrees" option
can be used as an escape hatch if the finger memory of existing
users depend on the current behaviour of "-B".
Reported-by: Willem Verstraeten <willem.verstraeten@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The documentation might also need updates, but I didn't look at.
builtin/checkout.c | 7 +++++++
t/t2060-switch.sh | 2 ++
t/t2400-worktree-add.sh | 8 ++++++++
3 files changed, 17 insertions(+)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b4ab972c5a..8a8ad23e98 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1600,6 +1600,13 @@ static int checkout_branch(struct checkout_opts *opts,
if (new_branch_info->path && !opts->force_detach && !opts->new_branch)
die_if_switching_to_a_branch_in_use(opts, new_branch_info->path);
+ /* "git checkout -B <branch>" */
+ if (opts->new_branch_force) {
+ char *full_ref = xstrfmt("refs/heads/%s", opts->new_branch);
+ die_if_switching_to_a_branch_in_use(opts, full_ref);
+ free(full_ref);
+ }
+
if (!new_branch_info->commit && opts->new_branch) {
struct object_id rev;
int flag;
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index e247a4735b..c91c4db936 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -170,8 +170,10 @@ test_expect_success 'switch back when temporarily detached and checked out elsew
# we test in both worktrees to ensure that works
# as expected with "first" and "next" worktrees
test_must_fail git -C wt1 switch shared &&
+ test_must_fail git -C wt1 switch -C shared &&
git -C wt1 switch --ignore-other-worktrees shared &&
test_must_fail git -C wt2 switch shared &&
+ test_must_fail git -C wt2 switch -C shared &&
git -C wt2 switch --ignore-other-worktrees shared
'
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index df4aff7825..bbcb2d3419 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -126,6 +126,14 @@ test_expect_success 'die the same branch is already checked out' '
)
'
+test_expect_success 'refuse to reset a branch in use elsewhere' '
+ (
+ cd here &&
+ test_must_fail git checkout -B newmain 2>actual &&
+ grep "already used by worktree at" actual
+ )
+'
+
test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
head=$(git -C there rev-parse --git-path HEAD) &&
ref=$(git -C there symbolic-ref HEAD) &&
--
2.43.0
^ permalink raw reply related
* Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox
From: Junio C Hamano @ 2023-11-23 5:58 UTC (permalink / raw)
To: Willem Verstraeten; +Cc: git
In-Reply-To: <xmqqjzq9cl70.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I guess we could change the behaviour so that
>
> git checkout -B <branch> [<start-point>]
>
> fails when <branch> is an existing branch that is in use in another
> worktree, and allow "-f" to be used to override the safety, i.e.,
>
> git checkout -f -B <branch> [<start-point>]
>
> would allow the <branch> to be repointed to <start-point> (or HEAD)
> even when it is used elsewhere.
It turns out that for some reason "-f" is not how we decided to
override this one---there is "--ignore-other-worktrees" option.
I'll attach the first step (preparatory refactoring) to this message
below, and follow it up with the second step to implement and test
the change.
--- >8 ---
From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 23 Nov 2023 14:11:41 +0900
Subject: [PATCH 1/2] checkout: refactor die_if_checked_out() caller
There is a bit dense logic to make a call to "die_if_checked_out()"
while trying to check out a branch. Extract it into a helper
function and give it a bit of comment to describe what is going on.
The most important part of the refactoring is the separation of the
guarding logic before making the call to die_if_checked_out() into
the caller specific part (e.g., the logic that decides that the
caller is trying to check out an existing branch) and the bypass due
to the "--ignore-other-worktrees" option. The latter will be common
no matter how the current or future callers decides they need this
protection.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/checkout.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc15..b4ab972c5a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1516,6 +1516,26 @@ static void die_if_some_operation_in_progress(void)
wt_status_state_free_buffers(&state);
}
+/*
+ * die if attempting to checkout an existing branch that is in use
+ * in another worktree, unless ignore-other-wortrees option is given.
+ * The check is bypassed when the branch is already the current one,
+ * as it will not make things any worse.
+ */
+static void die_if_switching_to_a_branch_in_use(struct checkout_opts *opts,
+ const char *full_ref)
+{
+ int flags;
+ char *head_ref;
+
+ if (opts->ignore_other_worktrees)
+ return;
+ head_ref = resolve_refdup("HEAD", 0, NULL, &flags);
+ if (head_ref && (!(flags & REF_ISSYMREF) || strcmp(head_ref, full_ref)))
+ die_if_checked_out(full_ref, 1);
+ free(head_ref);
+}
+
static int checkout_branch(struct checkout_opts *opts,
struct branch_info *new_branch_info)
{
@@ -1576,15 +1596,9 @@ static int checkout_branch(struct checkout_opts *opts,
if (!opts->can_switch_when_in_progress)
die_if_some_operation_in_progress();
- if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
- !opts->ignore_other_worktrees) {
- int flag;
- char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
- if (head_ref &&
- (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
- die_if_checked_out(new_branch_info->path, 1);
- free(head_ref);
- }
+ /* "git checkout <branch>" */
+ if (new_branch_info->path && !opts->force_detach && !opts->new_branch)
+ die_if_switching_to_a_branch_in_use(opts, new_branch_info->path);
if (!new_branch_info->commit && opts->new_branch) {
struct object_id rev;
--
2.43.0
^ permalink raw reply related
* Re: [commit-graph] v2.43.0 segfault with fetch.writeCommitGraph enabled when fetch
From: Taylor Blau @ 2023-11-23 3:56 UTC (permalink / raw)
To: ryenus; +Cc: Git mailing list
In-Reply-To: <CAKkAvayLrYS1GQ_-Z7kWM=k4pCnNv1Q=NvYcvT8+wqYPkePVcw@mail.gmail.com>
On Wed, Nov 22, 2023 at 12:05:02PM +0800, ryenus wrote:
> The issue appeared after updating to git v2.43.0, now git fetch would cause
> segmentation fault when commit graph is enabled. Though I only observed this
> issue in a repo with two submodules, regardless of whether the submodules are
> checked out or not. Meanwhile most other repos without submodules worked fine.
>
> > 15:57:10.660377 run-command.c:726 child_start[2] git maintenance run --auto --no-quiet
> > 15:57:10.665870 common-main.c:55 version 2.43.0
> > 15:57:10.666265 common-main.c:56 start /opt/homebrew/opt/git/libexec/git-core/git maintenance run --auto --no-quiet
> > 15:57:10.666469 repository.c:143 worktree /path/to/repo/sub/module2
> > 15:57:10.666649 git.c:464 cmd_name maintenance (_run_dashed_/_run_git_alias_/pull/fetch/fetch/maintenance)
> > 15:57:10.668232 git.c:723 exit elapsed:0.003405 code:0
> > 15:57:10.668241 trace2/tr2_tgt_normal.c:127 atexit elapsed:0.003415 code:0
> > 15:57:10.668611 run-command.c:979 child_exit[2] pid:46018 code:0 elapsed:0.008227
> > 15:57:10.668635 git.c:723 exit elapsed:1.837179 code:0
> > 15:57:10.668639 trace2/tr2_tgt_normal.c:127 atexit elapsed:1.837182 code:0
> > 15:57:10.669007 run-command.c:979 child_exit[3] pid:46006 code:0 elapsed:1.843739
> > 15:57:10.671522 usage.c:80 error fetch died of signal 11
> > error: fetch died of signal 11
> > 15:57:10.671645 run-command.c:979 child_exit[1] pid:45980 code:139 elapsed:5.292927
> > 15:57:10.671658 git.c:723 exit elapsed:5.337363 code:1
> > 15:57:10.671663 trace2/tr2_tgt_normal.c:127 atexit elapsed:5.337368 code:1
> > 15:57:10.672048 run-command.c:979 child_exit[1] pid:45978 code:1 elapsed:5.345050
> > 15:57:10.672099 git.c:819 exit elapsed:5.355644 code:1
> > 15:57:10.672105 trace2/tr2_tgt_normal.c:127 atexit elapsed:5.355649 code:1
I couldn't seem to find an easy reproduction for this bug. Would you
mind sharing a copy of your repository and/or a script that reproduces
this issue? Thanks.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] git-p4: fix fast import when empty commit is first
From: Junio C Hamano @ 2023-11-23 1:57 UTC (permalink / raw)
To: Alisha Kim via GitGitGadget; +Cc: git, Alisha Kim
In-Reply-To: <pull.1609.git.git.1700639764041.gitgitgadget@gmail.com>
"Alisha Kim via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Alisha Kim <pril@pril.cc>
>
> When executing p4 sync by specifying an excluded path, an empty commit
> will be created if there is only a change in the excluded path in
> revision.
> If git-p4.keepEmptyCommits is turned off and an empty commit is the
> first, fast-import will fail.
The above describe under what condition a failure gets triggered,
but there is no description on what approach the proposed solution
takes. You could teach "fast-import" to deal with an empty commit
properly, you could ignore empty commits and not produce input for
the fast-import command, you could probably turn these initial empty
commits into non-empty commits by adding dummy contents, etc. We
want to see in our proposed log messages what solution was taken and
how the solution was designed to satisfy what requirements. This is
to help future developers who will have to change the code that is
given by this patch, so that their updates can still adhere to what
ever design criteria you had in working on this change [*].
Side note: Your solution might be to ignore empty commits
despite keepEmptyCommits option is set (as I said, you did not
describe it at all in the above, so this is a hypothetical
example). If the reason behind choosing that design were "I
just do not want it to fail---I do not care if the resulting
history coming out of fast-import is crappy (I lose the p4 CL
descriptions for these commits, even though the user wants to
keep them)", then future developers can safely "fix" your fix
here by turning the initial empty commits into non-empty ones by
adding fake contents.
> @@ -3876,10 +3878,12 @@ class P4Sync(Command, P4UserMap):
> self.commit(description, filesForCommit, branch, parent)
> else:
> files = self.extractFilesFromCommit(description)
> - self.commit(description, files, self.branch,
> + isCommitted = self.commit(description, files, self.branch,
> self.initialParent)
> # only needed once, to connect to the previous commit
> - self.initialParent = ""
> + if isCommitted:
> + self.initialParent = ""
"is" does not sound grammatically correct. "didCommit" (i.e., "we
made a commit"), "haveCommitted" (i.e., "we have made a commit")
might be more understandable.
> except IOError:
> print(self.gitError.read())
> sys.exit(1)
>
> base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169
^ permalink raw reply
* Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox
From: Junio C Hamano @ 2023-11-23 1:28 UTC (permalink / raw)
To: Willem Verstraeten; +Cc: git
In-Reply-To: <CAGX9RpFMCVLQV7RbK2u9AabusvkZD+RZNv_UD=R00cSUrjutBg@mail.gmail.com>
Willem Verstraeten <willem.verstraeten@gmail.com> writes:
> git checkout -b main #reports a fatal error, as expected
This is expected because "main" already exists, not because "main"
is checked out elsewhere.
> git checkout -f main origin/main #also reports a fatal error, as expected
This is expected because origin/main is taken as pathspec, and it is
a request to checkout the paths that match the pathspec out of the
named tree-ish (i.e., "main"), even when these paths have local
changes, but you do not have paths that match "origin/main". The
failure is not because "main" is checked out elsewhere.
A slight variant of the command
git checkout -f -b main origin/main
still fails for the same reason as the first of your examples above.
It is a tangent, but I suspect this failure may be a bit unexpected.
In this example, "-f"orce could be overriding the usual failure from
"-b" to switch to a branch that already exists, but that is what
"-B" does, and "-f -b" does not work as a synonym for "-B".
In any case, these example you marked "fail as expected" do fail as
expected, but they fail for reasons that have nothing to do with the
protection of branches that are used in other worktrees.
> git checkout -B main origin/main # ----> this succeeds, which is
> unexpected <----
I agree this may be undesirable.
But it makes sort of sense, because "-B" is a forced form of "-b"
(i.e., it tells git: even when "-b" would fail, take necessary
measures to make it work), and we can view that it is part of
"forcing" to override the protection over branches that are used
elsewhere.
I guess we could change the behaviour so that
git checkout -B <branch> [<start-point>]
fails when <branch> is an existing branch that is in use in another
worktree, and allow "-f" to be used to override the safety, i.e.,
git checkout -f -B <branch> [<start-point>]
would allow the <branch> to be repointed to <start-point> (or HEAD)
even when it is used elsewhere.
Thoughts, whether they agree or disagree with what I just said, by
other experienced contributors are very much welcome, before I can
say "patches welcome" ;-).
Willem, thanks for raising the issue.
^ 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