* Re: [PATCH v2 1/4] doc: switch links to https
From: Josh Soref @ 2023-11-24 3:12 UTC (permalink / raw)
To: Elijah Newren; +Cc: Josh Soref via GitGitGadget, git
In-Reply-To: <CABPp-BEbfsss39-cENw2BwnQPp4edp9_JSN_O1e7vcci9XE+cQ@mail.gmail.com>
Elijah Newren wrote:
> > -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.
I claim that it refers to the file that is presented to the user,
which is current as of the time it was delivered by the specific
version of the git package.
> > -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.
The `[]` is part of AsciiDoc's [1] notation. I tripped on this when I
first looked into
this series (as I'm much more familiar w/ Markdown and Restructured Text).
[1] https://docs.asciidoctor.org/asciidoc/latest/syntax-quick-reference/
^ permalink raw reply
* [PATCH 2/2] orphan/unborn: fix use of 'orphan' in end-user facing messages
From: Junio C Hamano @ 2023-11-24 3:10 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqbkbj97a9.fsf@gitster.g>
"orphan branch" is not even grammatical ("orphaned branch" is), and
we have been using "unborn branch" to mean the state where the HEAD
points at a branch that does not yet exist.
Update end-user facing messages to correct them. There are cases
other random words are used (e.g., "unparented branch") but now we
have a glossary entry, use the term "unborn branch" consistently.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/checkout.c | 2 +-
builtin/worktree.c | 6 +++---
t/t2400-worktree-add.sh | 6 +++---
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f53612f468..9d250587df 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1620,7 +1620,7 @@ static struct option *add_common_switch_branch_options(
parse_opt_tracking_mode),
OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"),
PARSE_OPT_NOCOMPLETE),
- OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")),
+ OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unborn branch")),
OPT_BOOL_F(0, "overwrite-ignore", &opts->overwrite_ignore,
N_("update ignored files (default)"),
PARSE_OPT_NOCOMPLETE),
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 10db70b7ec..f0853a9927 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -49,14 +49,14 @@
_("No possible source branch, inferring '--orphan'")
#define WORKTREE_ADD_ORPHAN_WITH_DASH_B_HINT_TEXT \
- _("If you meant to create a worktree containing a new orphan branch\n" \
+ _("If you meant to create a worktree containing a new unborn branch\n" \
"(branch with no commits) for this repository, you can do so\n" \
"using the --orphan flag:\n" \
"\n" \
" git worktree add --orphan -b %s %s\n")
#define WORKTREE_ADD_ORPHAN_NO_DASH_B_HINT_TEXT \
- _("If you meant to create a worktree containing a new orphan branch\n" \
+ _("If you meant to create a worktree containing a new unborn branch\n" \
"(branch with no commits) for this repository, you can do so\n" \
"using the --orphan flag:\n" \
"\n" \
@@ -784,7 +784,7 @@ static int add(int ac, const char **av, const char *prefix)
N_("create a new branch")),
OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
N_("create or reset a branch")),
- OPT_BOOL(0, "orphan", &opts.orphan, N_("create unborn/orphaned branch")),
+ OPT_BOOL(0, "orphan", &opts.orphan, N_("create unborn branch")),
OPT_BOOL('d', "detach", &opts.detach, N_("detach HEAD at named commit")),
OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
OPT_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")),
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 051363acbb..3c12c3932b 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -414,7 +414,7 @@ test_wt_add_orphan_hint () {
git -C repo switch --orphan noref &&
test_must_fail git -C repo worktree add $opts foobar/ 2>actual &&
! grep "error: unknown switch" actual &&
- grep "hint: If you meant to create a worktree containing a new orphan branch" actual &&
+ grep "hint: If you meant to create a worktree containing a new unborn branch" actual &&
if [ $use_branch -eq 1 ]
then
grep -E "^hint: +git worktree add --orphan -b [^ ]+ [^ ]+$" actual
@@ -435,7 +435,7 @@ test_expect_success "'worktree add' doesn't show orphan hint in bad/orphan HEAD
(cd repo && test_commit commit) &&
test_must_fail git -C repo worktree add --quiet foobar_branch foobar/ 2>actual &&
! grep "error: unknown switch" actual &&
- ! grep "hint: If you meant to create a worktree containing a new orphan branch" actual
+ ! grep "hint: If you meant to create a worktree containing a new unborn branch" actual
'
test_expect_success 'local clone from linked checkout' '
@@ -708,7 +708,7 @@ test_expect_success 'git worktree --no-guess-remote option overrides config' '
test_dwim_orphan () {
local info_text="No possible source branch, inferring '--orphan'" &&
local fetch_error_text="fatal: No local or remote refs exist despite at least one remote" &&
- local orphan_hint="hint: If you meant to create a worktree containing a new orphan branch" &&
+ local orphan_hint="hint: If you meant to create a worktree containing a new unborn branch" &&
local invalid_ref_regex="^fatal: invalid reference: " &&
local bad_combo_regex="^fatal: '[-a-z]*' and '[-a-z]*' cannot be used together" &&
--
2.43.0
^ permalink raw reply related
* [PATCH 1/2] orphan/unborn: add to the glossary and use them consistently
From: Junio C Hamano @ 2023-11-24 3:09 UTC (permalink / raw)
To: git
To orphan is a verb that denotes the act of getting on an unborn
branch, and a few references to "orphan branch" in our documentation
are misuses of the word. They caused end-user confusion, which was
made even worse because we did not have the term defined in the
glossary document. Add entries for "unborn" branch and "orphan"
operation to the glossary, and adjust existing documentation
accordingly.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/config/advice.txt | 2 +-
Documentation/git-checkout.txt | 2 +-
Documentation/git-switch.txt | 2 +-
Documentation/git-worktree.txt | 4 ++--
Documentation/glossary-content.txt | 18 ++++++++++++++++++
5 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c548a91e67..6aaee01d7f 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -140,6 +140,6 @@ advice.*::
Advice shown when a fast-forward is not possible.
worktreeAddOrphan::
Advice shown when a user tries to create a worktree from an
- invalid reference, to instruct how to create a new orphan
+ invalid reference, to instruct how to create a new unborn
branch instead.
--
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 4af0904f47..3d526613d5 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -213,7 +213,7 @@ variable.
below for details.
--orphan <new-branch>::
- Create a new 'orphan' branch, named `<new-branch>`, started from
+ Create a new unborn branch, named `<new-branch>`, started from
`<start-point>` and switch to it. The first commit made on this
new branch will have no parents and it will be the root of a new
history totally disconnected from all the other branches and
diff --git a/Documentation/git-switch.txt b/Documentation/git-switch.txt
index c60fc9c138..3e23a82cf2 100644
--- a/Documentation/git-switch.txt
+++ b/Documentation/git-switch.txt
@@ -171,7 +171,7 @@ name, the guessing is aborted. You can explicitly give a name with
`branch.autoSetupMerge` configuration variable is true.
--orphan <new-branch>::
- Create a new 'orphan' branch, named `<new-branch>`. All
+ Create a new unborn branch, named `<new-branch>`. All
tracked files are removed.
--ignore-other-worktrees::
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 93d76f5d66..2a240f53ba 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -99,7 +99,7 @@ command will refuse to create the worktree (unless `--force` is used).
If `<commit-ish>` is omitted, neither `--detach`, or `--orphan` is
used, and there are no valid local branches (or remote branches if
`--guess-remote` is specified) then, as a convenience, the new worktree is
-associated with a new orphan branch named `<branch>` (after
+associated with a new unborn branch named `<branch>` (after
`$(basename <path>)` if neither `-b` or `-B` is used) as if `--orphan` was
passed to the command. In the event the repository has a remote and
`--guess-remote` is used, but no remote or local branches exist, then the
@@ -234,7 +234,7 @@ This can also be set up as the default behaviour by using the
--orphan::
With `add`, make the new worktree and index empty, associating
- the worktree with a new orphan/unborn branch named `<new-branch>`.
+ the worktree with a new unborn branch named `<new-branch>`.
--porcelain::
With `list`, output in an easy-to-parse format for scripts.
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 5a537268e2..c67b7f3ec1 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -294,6 +294,12 @@ This commit is referred to as a "merge commit", or sometimes just a
[[def_octopus]]octopus::
To <<def_merge,merge>> more than two <<def_branch,branches>>.
+[[def_orphan]]orphan::
+ The act of getting on a <<def_branch,branch>> that does not
+ exist yet (i.e., an <<def_unborn,unborn>> branch). After
+ such an operation, the commit first created becomes a commit
+ without a parent, starting a new history.
+
[[def_origin]]origin::
The default upstream <<def_repository,repository>>. Most projects have
at least one upstream project which they track. By default
@@ -674,6 +680,18 @@ The most notable example is `HEAD`.
object,
etc.
+[[def_unborn]]unborn::
+ The <<def_HEAD,HEAD>> can point at a <<def_branch,branch>>
+ that does not yet exist and that does not have any commit on
+ it yet, and such a branch is called an unborn branch. The
+ most typical way users encounter an unborn branch is by
+ creating a repository anew without cloning from elsewhere.
+ The HEAD would point at the 'main' (or 'master', depending
+ on your configuration) branch that is yet to be born. Also
+ some operations can get you on an unborn branch with their
+ <<def_orphan,orphan>> option.
+
+
[[def_unmerged_index]]unmerged index::
An <<def_index,index>> which contains unmerged
<<def_index_entry,index entries>>.
--
2.43.0
^ permalink raw reply related
* Re: Orphan branch not well-defined?
From: Junio C Hamano @ 2023-11-24 2:31 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Chris Torek, Craig H Maynard, Git Community
In-Reply-To: <CAPig+cS6dAANqm7AcrjU9nhBezXRvB0Y-zPOzdar7s_8E-c28Q@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Thu, Nov 23, 2023 at 9:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>> +[[def_orphan]]orphan::
>> + The act of becoming on an <<def_unborn,unborn>> branch.
>
> s/on an/an/
I actually did mean it. It is not a verb whose subject is a branch.
The user (or you can call a repository) gets on a branch that
happens not to exist yet.
^ permalink raw reply
* Re: Orphan branch not well-defined?
From: Eric Sunshine @ 2023-11-24 2:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Chris Torek, Craig H Maynard, Git Community
In-Reply-To: <xmqqwmu79ac4.fsf@gitster.g>
On Thu, Nov 23, 2023 at 9:03 PM Junio C Hamano <gitster@pobox.com> wrote:
> +[[def_orphan]]orphan::
> + The act of becoming on an <<def_unborn,unborn>> branch.
s/on an/an/
> + After such an operation, the <<def_HEAD,HEAD>> points at a
> + <<def_branch,branch>> that does not yet exist, and the
> + commit first created from such a state becomes a root
> + commit, starting a new history.
^ permalink raw reply
* Re: Orphan branch not well-defined?
From: Junio C Hamano @ 2023-11-24 2:12 UTC (permalink / raw)
To: Chris Torek; +Cc: Craig H Maynard, Git Community
In-Reply-To: <xmqqwmu79ac4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Chris Torek <chris.torek@gmail.com> writes:
>
>> ** Unborn Branch is the better term **
>
> Yes, To orphan is a verb that denotes the act of becoming on an
> unborn branch, and a few references to "orphan branch" in our
> documentation are misuses of the word, I would have to say.
To be fair, the use of verb "orphan" by the folks first designed the
"checkout --orphan" does make quite a lot of sense and it is very
much consistent with the fact that the operation leaves the index
and the working tree intact (unlike "switch --orphan" that empties
the contents, which came much later).
The intended use case was that the user had the current set of
contents that is desirable with history that is undesirable behind
it, and wanted to part with the baggage^Whistory while keeping the
end state. The operation was meant to be the first step to create a
"parent-less" (aka "orphaned" from the parents in the original
history) commit that records the desired state. It is the reason
why "checkout --orphan" keeps the contents intact and moves the HEAD
to be on an unborn branch.
^ permalink raw reply
* Re: Orphan branch not well-defined?
From: Junio C Hamano @ 2023-11-24 2:03 UTC (permalink / raw)
To: Chris Torek; +Cc: Craig H Maynard, Git Community
In-Reply-To: <CAPx1Gvf_meaEBq9XfS9aPg0yLja-2sAW5SUg0jx6f1jNyfmWHw@mail.gmail.com>
Chris Torek <chris.torek@gmail.com> writes:
> ** Unborn Branch is the better term **
Yes, To orphan is a verb that denotes the act of becoming on an
unborn branch, and a few references to "orphan branch" in our
documentation are misuses of the word, I would have to say.
I suspect that there are other mentions of "orphan branch" in the
code comments, variable names, and even end-user facing messages,
some of which may need to be corrected, but the first place to start
is in the glossary. How about this?
Documentation/glossary-content.txt | 19 +++++++++++++++++++
Documentation/config/advice.txt | 2 +-
Documentation/git-checkout.txt | 2 +-
Documentation/git-switch.txt | 2 +-
Documentation/git-worktree.txt | 4 ++--
5 files changed, 24 insertions(+), 5 deletions(-)
diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
index 59d8ab8572..bbf7b84ab7 100644
--- c/Documentation/glossary-content.txt
+++ w/Documentation/glossary-content.txt
@@ -312,6 +312,13 @@ This commit is referred to as a "merge commit", or sometimes just a
[[def_octopus]]octopus::
To <<def_merge,merge>> more than two <<def_branch,branches>>.
+[[def_orphan]]orphan::
+ The act of becoming on an <<def_unborn,unborn>> branch.
+ After such an operation, the <<def_HEAD,HEAD>> points at a
+ <<def_branch,branch>> that does not yet exist, and the
+ commit first created from such a state becomes a root
+ commit, starting a new history.
+
[[def_origin]]origin::
The default upstream <<def_repository,repository>>. Most projects have
at least one upstream project which they track. By default
@@ -695,6 +702,18 @@ The most notable example is `HEAD`.
object,
etc.
+[[def_unborn]]unborn::
+ The <<def_HEAD,HEAD>> can point at a <<def_branch,branch>>
+ that does not yet have any <<def_commit,commit>> on it, and
+ such a branch is called an unborn branch. The most typical
+ way users encounter an unborn branch is by creating a
+ repository anew without cloning from elsewhere. The HEAD
+ would point at the 'main' (or 'master', depending on your
+ configuration) branch that is yet to be born. Also some
+ operations can get you on an unborn branch with their
+ <<def_orphan,orphan>> option.
+
+
[[def_unmerged_index]]unmerged index::
An <<def_index,index>> which contains unmerged
<<def_index_entry,index entries>>.
diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
index 2737381a11..4d7e5d8759 100644
--- c/Documentation/config/advice.txt
+++ w/Documentation/config/advice.txt
@@ -140,6 +140,6 @@ advice.*::
Advice shown when a fast-forward is not possible.
worktreeAddOrphan::
Advice shown when a user tries to create a worktree from an
- invalid reference, to instruct how to create a new orphan
+ invalid reference, to instruct how to create a new unborn
branch instead.
--
diff --git c/Documentation/git-checkout.txt w/Documentation/git-checkout.txt
index 240c54639e..26ad1a5e27 100644
--- c/Documentation/git-checkout.txt
+++ w/Documentation/git-checkout.txt
@@ -215,7 +215,7 @@ variable.
below for details.
--orphan <new-branch>::
- Create a new 'orphan' branch, named `<new-branch>`, started from
+ Create a new unborn branch, named `<new-branch>`, started from
`<start-point>` and switch to it. The first commit made on this
new branch will have no parents and it will be the root of a new
history totally disconnected from all the other branches and
diff --git c/Documentation/git-switch.txt w/Documentation/git-switch.txt
index c60fc9c138..3e23a82cf2 100644
--- c/Documentation/git-switch.txt
+++ w/Documentation/git-switch.txt
@@ -171,7 +171,7 @@ name, the guessing is aborted. You can explicitly give a name with
`branch.autoSetupMerge` configuration variable is true.
--orphan <new-branch>::
- Create a new 'orphan' branch, named `<new-branch>`. All
+ Create a new unborn branch, named `<new-branch>`. All
tracked files are removed.
--ignore-other-worktrees::
diff --git c/Documentation/git-worktree.txt w/Documentation/git-worktree.txt
index 93d76f5d66..2a240f53ba 100644
--- c/Documentation/git-worktree.txt
+++ w/Documentation/git-worktree.txt
@@ -99,7 +99,7 @@ command will refuse to create the worktree (unless `--force` is used).
If `<commit-ish>` is omitted, neither `--detach`, or `--orphan` is
used, and there are no valid local branches (or remote branches if
`--guess-remote` is specified) then, as a convenience, the new worktree is
-associated with a new orphan branch named `<branch>` (after
+associated with a new unborn branch named `<branch>` (after
`$(basename <path>)` if neither `-b` or `-B` is used) as if `--orphan` was
passed to the command. In the event the repository has a remote and
`--guess-remote` is used, but no remote or local branches exist, then the
@@ -234,7 +234,7 @@ This can also be set up as the default behaviour by using the
--orphan::
With `add`, make the new worktree and index empty, associating
- the worktree with a new orphan/unborn branch named `<new-branch>`.
+ the worktree with a new unborn branch named `<new-branch>`.
--porcelain::
With `list`, output in an easy-to-parse format for scripts.
^ permalink raw reply related
* Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere
From: Junio C Hamano @ 2023-11-24 1:19 UTC (permalink / raw)
To: Eric Sunshine; +Cc: phillip.wood, Willem Verstraeten, git
In-Reply-To: <CAPig+cRdQW-DG8PFb-P0U_44pFWxskVoOtjbGfD_OiHHDk8DdA@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> 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 am surprised that I did not remember that old discussion at all,
and I am doubly surprised that I still do not, even though I clearly
recognise my writing in the thread.
> I agree, also, that this two-patch series is simple to digest.
OK.
^ permalink raw reply
* Re: [PATCH 4/4] completion: avoid user confusion in non-cone mode
From: Junio C Hamano @ 2023-11-24 1:19 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <fe8669a3f4f05c186e497f870c7e7ba9a94ac63f.1700761448.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> if [[ "$using_cone" == "true" ]]; then
> __gitcomp_directories
Hmph, doesn't "Providing the files and directories currently present
is thus always wrong." apply equally to cone mode?
> + 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.
This makes a lot of sense. May make even more sense with
s/HEAD/index/, though.
> + # 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.)
Hmph, would an obvious alternative to (1) check against the HEAD (or
the index) to see if the prefix string matches an entity at the
current directory level, and then (2) to prefix the result of the
previous step with "/$(git rev-parse --show-prefix)" work? That is
something like this:
$ cd t
$ git sparse-checkout add help<TAB>
->
$ git sparse-checkout add /t/helper/
and when the user gave the full path from the root level, do the
obvious:
$ cd t
$ git sparse-checkout add /t/help<TAB>
->
$ git sparse-checkout add /t/helper/
Another more fundamental approach to avoid "confusion" this bullet
item tries to side step might be to *fix* the command that gets
completed. As "git sparse-checkout --help" is marked as
EXPERIMENTAL in capital letters, we should be able to say "what was
traditionally known as 'add' is from now on called 'add-pattern' and
command line completion would not get in the way; the 'add'
subcommand now takes only literal paths, not patterns, that are
relative to the current directory" if we wanted to.
> + # 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
> }
^ permalink raw reply
* Re: [PATCH v7 00/14] Introduce new `git replay` command
From: Junio C Hamano @ 2023-11-24 0:28 UTC (permalink / raw)
To: Elijah Newren
Cc: Johannes Schindelin, Christian Couder, git, Patrick Steinhardt,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CABPp-BFgJFeL4cUJ8+JZ2ZuM58F3rSYGYu_5w_mDLYWtzO4raw@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> 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!
Thanks, both, for reviews. I guess the only remaining issue now is
the "(EXPERIMENTAL)" label and we are ready to declare a victory?
Thanks.
^ permalink raw reply
* 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
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