* [PATCH v5 4/5] doc: pretty-formats describe use of ellipsis in truncation
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20230119181827.1319-1-philipoakley@iee.email>
Commit a7f01c6b4d (pretty: support truncating in %>, %< and %><,
2013-04-19) added the use of ellipsis when truncating placeholder
values.
Show our 'two dot' ellipsis, and examples for the left, middle and
right truncation to avoid any confusion as to which end of the string
is adjusted. (cf justification and sub-string).
Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
Documentation/pretty-formats.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index cbca60a196..e51f1e54e1 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -149,9 +149,9 @@ The placeholders are:
'%<( <N> [,trunc|ltrunc|mtrunc])':: make the next placeholder take at
least N column widths, padding spaces on
the right if necessary. Optionally
- truncate at the beginning (ltrunc),
- the middle (mtrunc) or the end
- (trunc) if the output is longer than
+ truncate (with ellipsis '..') at the left (ltrunc) `..ft`,
+ the middle (mtrunc) `mi..le`, or the end
+ (trunc) `rig..`, if the output is longer than
N columns.
Note 1: that truncating
only works correctly with N >= 2.
--
2.39.1.windows.1
^ permalink raw reply related
* [PATCH v5 2/5] doc: pretty-formats: delineate `%<|(` parameter values
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20230119181827.1319-1-philipoakley@iee.email>
Commit a57523428b4 (pretty: support padding placeholders, %< %> and %><,
2013-04-19) introduced column width place holders. It also added
separate column position `%<|(` placeholders for display screen based
placement.
Change the display screen parameter reference from 'N' to 'M' and
corresponding descriptives to make the distinction clearer.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
Documentation/pretty-formats.txt | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 02bec23509..8cc1072196 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -147,7 +147,7 @@ The placeholders are:
'%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of
linkgit:git-shortlog[1].
'%<( <N> [,trunc|ltrunc|mtrunc])':: make the next placeholder take at
- least N columns, padding spaces on
+ least N column widths, padding spaces on
the right if necessary. Optionally
truncate at the beginning (ltrunc),
the middle (mtrunc) or the end
@@ -155,18 +155,18 @@ The placeholders are:
N columns.
Note 1: that truncating
only works correctly with N >= 2.
- Note 2: spaces around the N
+ Note 2: spaces around the N and M (see below)
values are optional.
-'%<|( <N> )':: make the next placeholder take at least until Nth
- columns, padding spaces on the right if necessary
-'%>( <N> )', '%>|( <N> )':: similar to '%<( <N> )', '%<|( <N> )' respectively,
+'%<|( <M> )':: make the next placeholder take at least until Mth
+ display column, padding spaces on the right if necessary
+'%>( <N> )', '%>|( <M> )':: similar to '%<( <N> )', '%<|( <M> )' respectively,
but padding spaces on the left
-'%>>( <N> )', '%>>|( <N> )':: similar to '%>( <N> )', '%>|( <N> )'
+'%>>( <N> )', '%>>|( <M> )':: similar to '%>( <N> )', '%>|( <M> )'
respectively, except that if the next
placeholder takes more spaces than given and
there are spaces on its left, use those
spaces
-'%><( <N> )', '%><|( <N> )':: similar to '%<( <N> )', '%<|( <N> )'
+'%><( <N> )', '%><|( <M> )':: similar to '%<( <N> )', '%<|( <M> )'
respectively, but padding both sides
(i.e. the text is centered)
--
2.39.1.windows.1
^ permalink raw reply related
* [PATCH v5 1/5] doc: pretty-formats: separate parameters from placeholders
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20230119181827.1319-1-philipoakley@iee.email>
Commit a57523428b4 (pretty: support padding placeholders, %< %> and %><,
2013-04-19) introduced columnated place holders. These placeholders
can be confusing as they contain `<` and `>` characters as part
of their placeholders adjacent to the `<N>` parameters.
Add spaces either side of the `<N>` parameters in the title line.
The code (strtol) will consume any spaces around the number values
(assuming they are passed as a quoted string with spaces).
Note that the spaces are optional.
Subsequent commits will clarify other confusions.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
Documentation/pretty-formats.txt | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 0b4c1c8d98..02bec23509 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -146,24 +146,27 @@ The placeholders are:
'%m':: left (`<`), right (`>`) or boundary (`-`) mark
'%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of
linkgit:git-shortlog[1].
-'%<(<N>[,trunc|ltrunc|mtrunc])':: make the next placeholder take at
+'%<( <N> [,trunc|ltrunc|mtrunc])':: make the next placeholder take at
least N columns, padding spaces on
the right if necessary. Optionally
truncate at the beginning (ltrunc),
the middle (mtrunc) or the end
(trunc) if the output is longer than
- N columns. Note that truncating
+ N columns.
+ Note 1: that truncating
only works correctly with N >= 2.
-'%<|(<N>)':: make the next placeholder take at least until Nth
+ Note 2: spaces around the N
+ values are optional.
+'%<|( <N> )':: make the next placeholder take at least until Nth
columns, padding spaces on the right if necessary
-'%>(<N>)', '%>|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' respectively,
+'%>( <N> )', '%>|( <N> )':: similar to '%<( <N> )', '%<|( <N> )' respectively,
but padding spaces on the left
-'%>>(<N>)', '%>>|(<N>)':: similar to '%>(<N>)', '%>|(<N>)'
+'%>>( <N> )', '%>>|( <N> )':: similar to '%>( <N> )', '%>|( <N> )'
respectively, except that if the next
placeholder takes more spaces than given and
there are spaces on its left, use those
spaces
-'%><(<N>)', '%><|(<N>)':: similar to '%<(<N>)', '%<|(<N>)'
+'%><( <N> )', '%><|( <N> )':: similar to '%<( <N> )', '%<|( <N> )'
respectively, but padding both sides
(i.e. the text is centered)
--
2.39.1.windows.1
^ permalink raw reply related
* [PATCH v5 0/5] Pretty formats: Clarify column alignment
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20221112143616.1429-1-philipoakley@iee.email>
This is a follow up to earlier versions of the series [1,2,3,4] that looked
to add left and right hard truncation of the column limited placeholders.
Taylor had provided an early review at [5].
Junio identified [6] that one problem would be that wide characters may
need to be cut in half and this wasn't properly covered.
I noted that there were other existing complications, and the wide
character problem could already be seen with particular placeholders.
I said I would at least update the docs [7] to cover features that weren't
fully documented and add a few simple tests to show the wide character
and other discovered problems.
The series now has 5 patches. The first four add documentation to match
their original code changes.
The final patch adds the tests for the special character cases, and adds a cautionary note to the docs.
The added tests make assumptions as to the desired solution in the 'expect'
result, and alternatives are discussed as to how to 'split' a wide
character to fit a single column when required to fit to column limits.
This series is unaffected by the security fix (CVE-2022-41903 size_t/int)
in Git 2.39.1 [8] within this formatting code.
Philip
[1] https://lore.kernel.org/git/20221030185614.3842-1-philipoakley@iee.email/ [PATCH 0/1] extend the truncating pretty formats
[2] https://lore.kernel.org/git/20221101225724.2165-1-philipoakley@iee.email/ [PATCH v2 0/1] extend the truncating pretty formats
[3] https://lore.kernel.org/git/20221102120853.2013-1-philipoakley@iee.email/ [PATCH v3] pretty-formats: add hard truncation, without ellipsis, options
[4] https://lore.kernel.org/git/20221112143616.1429-1-philipoakley@iee.email/ [PATCH v4] pretty-formats: add hard truncation, without ellipsis, options
[5] https://lore.kernel.org/git/Y17PS%2F2LgRIJKGoo@nand.local/ (Taylor: t4205-log-pretty-formats.sh. Is that coverage ..)
[6] https://lore.kernel.org/git/xmqqfsedywli.fsf@gitster.g/ (Junio: Imagine there are series of wide characters,...)
[7] https://lore.kernel.org/git/093e1dca-b9d4-f1f2-0845-ad6711622cf5@iee.email/ (po: I'll at least work on the doc clarification..)
[8] https://lore.kernel.org/git/xmqq7cxl9h0i.fsf@gitster.g/
Philip Oakley (5):
doc: pretty-formats: separate parameters from placeholders
doc: pretty-formats: delineate `%<|(` parameter values
doc: pretty-formats document negative column alignments
doc: pretty-formats describe use of ellipsis in truncation
doc: pretty-formats note wide char limitations, and add tests
Documentation/pretty-formats.txt | 32 +++++++++++++++++++++-----------
t/t4205-log-pretty-formats.sh | 27 +++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 11 deletions(-)
--
2.39.1.windows.1
^ permalink raw reply
* [PATCH v5 3/5] doc: pretty-formats document negative column alignments
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20230119181827.1319-1-philipoakley@iee.email>
Commit 066790d7cb0 (pretty.c: support <direction>|(<negative number>) forms,
2016-06-16) added the option for right justified column alignment without
updating the documentation.
Add an explanation of its use of negative column values.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
Documentation/pretty-formats.txt | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8cc1072196..cbca60a196 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -158,7 +158,9 @@ The placeholders are:
Note 2: spaces around the N and M (see below)
values are optional.
'%<|( <M> )':: make the next placeholder take at least until Mth
- display column, padding spaces on the right if necessary
+ display column, padding spaces on the right if necessary.
+ Use negative M values for column positions measured
+ from the right hand edge of the terminal window.
'%>( <N> )', '%>|( <M> )':: similar to '%<( <N> )', '%<|( <M> )' respectively,
but padding spaces on the left
'%>>( <N> )', '%>>|( <M> )':: similar to '%>( <N> )', '%>|( <M> )'
--
2.39.1.windows.1
^ permalink raw reply related
* Redirect isn't working:
From: Jack Adrian Zappa @ 2023-01-19 18:06 UTC (permalink / raw)
To: git-mailing-list
Here is the filled out form for submitting a bug report:
What did you do before the bug happened? (Steps to reproduce your issue)
At the command line, I typed the following command:
$ git diff --no-index --word-diff --color=always <(od -An -t x1
-w10000000000 file1.txt) <(od -An -t x1 -w10000000000 file2.txt)
What did you expect to happen? (Expected behavior)
I expected to see the diff of the two files.
What happened instead? (Actual behavior)
Instead I saw this:
error: Could not access '/proc/1961/fd/63'
What's different between what you expected and what actually happened?
The difference is that I would get the difference between the two
files as opposed to a critical failure message.
Anything else you want to add:
Nope.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.38.1.windows.1
cpu: x86_64
built from commit: b85c8f604d375d4d773a36842964e8a7ec056aae
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19044
compiler info: gnuc: 12.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply
* Re: [PATCH 2/2] [RFC] push: allow delete one level ref
From: ZheNing Hu @ 2023-01-19 17:39 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: ZheNing Hu via GitGitGadget, git, Junio C Hamano, Derrick Stolee,
Elijah Newren, Michael Haggerty, Christian Couder, Jeff King
In-Reply-To: <230117.861qntz4dc.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2023年1月17日周二 21:18写道：
>
>
> On Tue, Jan 17 2023, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Git will reject the deletion of one level refs e,g. "refs/foo"
> > through "git push -d", however, some users want to be able to
> > clean up these branches that were created unexpectedly on the
> > remote.
> >
> > Therefore, when updating branches on the server with
> > "git receive-pack", by checking whether it is a branch deletion
> > operation, it will determine whether to allow the update of
> > one level refs. This avoids creating/updating such one level
> > branches, but allows them to be deleted.
> >
> > On the client side, "git push" also does not properly fill in
> > the old-oid of one level refs, which causes the server-side
> > "git receive-pack" to think that the ref's old-oid has changed
> > when deleting one level refs, this causes the push to be rejected.
> >
> > So the solution is to fix the client to be able to delete
> > one level refs by properly filling old-oid.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> > builtin/receive-pack.c | 5 ++++-
> > connect.c | 2 +-
> > t/t5516-fetch-push.sh | 13 +++++++++++++
> > 3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 13ff9fae3ba..ad21877ea1b 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -1463,7 +1463,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)
> > find_shared_symref(worktrees, "HEAD", name);
> >
> > /* only refs/... are allowed */
> > - if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
> > + if (!starts_with(name, "refs/") ||
> > + check_refname_format(name + 5,
> > + is_null_oid(new_oid) ?
> > + REFNAME_ALLOW_ONELEVEL : 0)) {
>
> Style nit: We tend to wrap at 79 characters, adn with argument lists you
> "keep going" until you hit that limit.
>
> In this case strictly following that rule will lead to funny
> indentation, as we'll have to wrap at "is_null_oid(...)" etc.
>
> But even when avoiding that (which seems good in this case) this should
> be:
>
> if (!starts_with(name, "refs/") ||
> check_refname_format(name + 5, is_null_oid(new_oid) ?
> REFNAME_ALLOW_ONELEVEL : 0)) {
>
>
Make sense, I'm going to fix the formatting here.
>
> > rp_error("refusing to update funny ref '%s' remotely", name);
> > ret = "funny refname";
> > goto out;
> > diff --git a/connect.c b/connect.c
> > index 63e59641c0d..b841ae58e03 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -30,7 +30,7 @@ static int check_ref(const char *name, unsigned int flags)
> > return 0;
> >
> > /* REF_NORMAL means that we don't want the magic fake tag refs */
> > - if ((flags & REF_NORMAL) && check_refname_format(name, 0))
> > + if ((flags & REF_NORMAL) && check_refname_format(name, REFNAME_ALLOW_ONELEVEL))
>
> Here we should wrap after "name,", we end up with a too-long line.
>
To be honest, sometimes it's hard to notice if the current line is longer
than 79 characters, it's often a matter of intuition. Is there any ci script
that can detect this kind of problem?
> > return 0;
> >
> > /* REF_HEADS means that we want regular branch heads */
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index f37861efc40..dec8950a392 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -903,6 +903,19 @@ test_expect_success 'push --delete refuses empty string' '
> > test_must_fail git push testrepo --delete ""
> > '
> >
> > +test_expect_success 'push --delete onelevel refspecs' '
> > + mk_test testrepo heads/main &&
> > + (
> > + cd testrepo &&
> > + git update-ref refs/onelevel refs/heads/main
> > + ) &&
>
> Avoid the subshell here with:
>
> git -C update-ref ....
>
OK.
> > + git push testrepo --delete refs/onelevel &&
> > + (
> > + cd testrepo &&
> > + test_must_fail git rev-parse --verify refs/onelevel
>
> Ditto.
Thanks,
ZheNing Hu
^ permalink raw reply
* [PATCH v2] mingw: replace deprecated GetVersion with RtlGetVersion
From: Rose via GitGitGadget @ 2023-01-19 17:36 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
In-Reply-To: <pull.1438.git.git.1674148045970.gitgitgadget@gmail.com>
From: Seija Kijin <doremylover123@gmail.com>
The previous way is deprecated and returns
the wrong value in Windows 8 and up,
returning the manifest Windows data
as opposed to the actual Windows data.
RtlGetVersion is the correct way to get the Windows version now.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
mingw: replace deprecated GetVersion with RtlGetVersion
GetVersion has its behavior changed in Windows 8 and above anyway, so
this is the right way to do it now.
The previous way returns the wrong value in Windows 8 and up, returning
the manifest Windows data as opposed to the actual Windows data.
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1438%2FAtariDreams%2Fmingw-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1438/AtariDreams/mingw-v2
Pull-Request: https://github.com/git/git/pull/1438
Range-diff vs v1:
1: 8fe920c653e ! 1: e5457905028 mingw: replace deprecated GetVersion with RtlGetVersion
@@ compat/mingw.c: int wmain(int argc, const wchar_t **wargv)
}
+/*
-+ * For RtlGetVersion in uname
++ * for RtlGetVersion in uname
+ */
+
+typedef NTSTATUS(WINAPI *RtlGetVersionPtr)(PRTL_OSVERSIONINFOW);
@@ compat/mingw.c: int wmain(int argc, const wchar_t **wargv)
+ RtlGetVersionInternal.procaddr =
+ GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "RtlGetVersion");
+ if (!RtlGetVersionInternal.procaddr) {
-+ die_message_errno(
-+ "Could not get handle to RtlGetVersion in ntdll.dll");
++ /* if this is reached, something is seriously, seriously wrong
++ */
++ perror("Could not call RtlGetVersion in ntdll.dll");
++ abort();
+ }
+
+ version.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
compat/mingw.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index af397e68a1d..07d81fb8d69 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3081,15 +3081,38 @@ int wmain(int argc, const wchar_t **wargv)
return exit_status;
}
+/*
+ * for RtlGetVersion in uname
+ */
+
+typedef NTSTATUS(WINAPI *RtlGetVersionPtr)(PRTL_OSVERSIONINFOW);
+union winprocaddr {
+ FARPROC procaddr;
+ RtlGetVersionPtr procGetVersion;
+};
+
int uname(struct utsname *buf)
{
- unsigned v = (unsigned)GetVersion();
+ union winprocaddr RtlGetVersionInternal;
+ OSVERSIONINFOA version;
+
+ RtlGetVersionInternal.procaddr =
+ GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "RtlGetVersion");
+ if (!RtlGetVersionInternal.procaddr) {
+ /* if this is reached, something is seriously, seriously wrong
+ */
+ perror("Could not call RtlGetVersion in ntdll.dll");
+ abort();
+ }
+
+ version.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+ RtlGetVersionInternal.procGetVersion((PRTL_OSVERSIONINFOW)&version);
+
memset(buf, 0, sizeof(*buf));
xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows");
- xsnprintf(buf->release, sizeof(buf->release),
- "%u.%u", v & 0xff, (v >> 8) & 0xff);
- /* assuming NT variants only.. */
- xsnprintf(buf->version, sizeof(buf->version),
- "%u", (v >> 16) & 0x7fff);
+ xsnprintf(buf->release, sizeof(buf->release), "%lu.%lu",
+ version.dwMajorVersion, version.dwMinorVersion);
+ xsnprintf(buf->version, sizeof(buf->version), "%lu",
+ version.dwBuildNumber);
return 0;
}
base-commit: a7caae2729742fc80147bca1c02ae848cb55921a
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
From: ZheNing Hu via GitGitGadget @ 2023-01-19 17:34 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee, Victoria Dye, Elijah Newren,
Nguyễn Thái Ngọc Duy, ZheNing Hu, ZheNing Hu
In-Reply-To: <pull.1458.v2.git.1674149666.gitgitgadget@gmail.com>
From: ZheNing Hu <adlternative@gmail.com>
Because sometimes we want to check if the files in the
index match the sparse specification, so introduce
"%(skipworktree)" atom to git ls-files `--format` option.
When we use this option, if the file match the sparse
specification, it will output "1", otherwise, output
empty string "".
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
Documentation/git-ls-files.txt | 5 +++++
builtin/ls-files.c | 3 +++
t/t3013-ls-files-format.sh | 23 +++++++++++++++++++++++
3 files changed, 31 insertions(+)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 440043cdb8e..2540b404808 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -260,6 +260,11 @@ eolattr::
that applies to the path.
path::
The pathname of the file which is recorded in the index.
+skipworktree::
+ If the file in the index marked with SKIP_WORKTREE bit.
+ It means the file do not match the sparse specification.
+ See link:technical/sparse-checkout.txt[sparse-checkout]
+ for more information.
EXCLUDE PATTERNS
----------------
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a03b559ecaa..bbff868ae6b 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -280,6 +280,9 @@ static size_t expand_show_index(struct strbuf *sb, const char *start,
data->pathname));
else if (skip_prefix(start, "(path)", &p))
write_name_to_buf(sb, data->pathname);
+ else if (skip_prefix(start, "(skipworktree)", &p))
+ strbuf_addstr(sb, ce_skip_worktree(data->ce) ?
+ "1" : "");
else
die(_("bad ls-files format: %%%.*s"), (int)len, start);
diff --git a/t/t3013-ls-files-format.sh b/t/t3013-ls-files-format.sh
index efb7450bf1e..cd35dba5930 100755
--- a/t/t3013-ls-files-format.sh
+++ b/t/t3013-ls-files-format.sh
@@ -92,4 +92,27 @@ test_expect_success 'git ls-files --format with --debug' '
test_cmp expect actual
'
+test_expect_success 'git ls-files --format with skipworktree' '
+ test_when_finished "git sparse-checkout disable" &&
+ mkdir dir1 dir2 &&
+ echo "file1" >dir1/file1.txt &&
+ echo "file2" >dir2/file2.txt &&
+ git add dir1 dir2 &&
+ git commit -m skipworktree &&
+ git sparse-checkout set dir1 &&
+ git ls-files --format="%(path)%(skipworktree)" >actual &&
+ cat >expect <<-\EOF &&
+ dir1/file1.txt
+ dir2/file2.txt1
+ o1.txt
+ o2.txt
+ o3.txt
+ o4.txt
+ o5.txt
+ o6.txt
+ o7.txt
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/2] docs: fix sparse-checkout docs link
From: ZheNing Hu via GitGitGadget @ 2023-01-19 17:34 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee, Victoria Dye, Elijah Newren,
Nguyễn Thái Ngọc Duy, ZheNing Hu, ZheNing Hu
In-Reply-To: <pull.1458.v2.git.1674149666.gitgitgadget@gmail.com>
From: ZheNing Hu <adlternative@gmail.com>
There is a linking error when other documents want to refer to
technical/sparse-checkout.txt, Also, there is something wrong
with the format of the paragraphs in sparse-checkout.txt, which
makes acsiidoc compile errors.
So fix the format of sparse-checkout.txt, and link it in the
Makefile to correct that.
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
Documentation/Makefile | 1 +
Documentation/technical/sparse-checkout.txt | 43 ++++++++++++++-------
2 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 9c67c3a1c50..7540da27b8c 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -121,6 +121,7 @@ TECH_DOCS += technical/reftable
TECH_DOCS += technical/scalar
TECH_DOCS += technical/send-pack-pipeline
TECH_DOCS += technical/shallow
+TECH_DOCS += technical/sparse-checkout
TECH_DOCS += technical/trivial-merge
SP_ARTICLES += $(TECH_DOCS)
SP_ARTICLES += technical/api-index
diff --git a/Documentation/technical/sparse-checkout.txt b/Documentation/technical/sparse-checkout.txt
index fa0d01cbda4..52e86764a6c 100644
--- a/Documentation/technical/sparse-checkout.txt
+++ b/Documentation/technical/sparse-checkout.txt
@@ -1,3 +1,6 @@
+Sparse Checkout Design Notes
+============================
+
Table of contents:
* Terminology
@@ -14,7 +17,8 @@ Table of contents:
* Reference Emails
-=== Terminology ===
+Terminology
+-----------
cone mode: one of two modes for specifying the desired subset of files
in a sparse-checkout. In cone-mode, the user specifies
@@ -92,7 +96,8 @@ vivifying: When a command restores a tracked file to the working tree (and
file), this is referred to as "vivifying" the file.
-=== Purpose of sparse-checkouts ===
+Purpose of sparse-checkouts
+---------------------------
sparse-checkouts exist to allow users to work with a subset of their
files.
@@ -255,7 +260,8 @@ will perceive the checkout as dense, and commands should thus behave as if
all files are present.
-=== Usecases of primary concern ===
+Usecases of primary concern
+---------------------------
Most of the rest of this document will focus on Behavior A and Behavior
B. Some notes about the other two cases and why we are not focusing on
@@ -300,7 +306,8 @@ Behavior C do not assume they are part of the Behavior B camp and propose
patches that break things for the real Behavior B folks.
-=== Oversimplified mental models ===
+Oversimplified mental models
+----------------------------
An oversimplification of the differences in the above behaviors is:
@@ -313,7 +320,8 @@ An oversimplification of the differences in the above behaviors is:
they can later lazily be populated instead.
-=== Desired behavior ===
+Desired behavior
+----------------
As noted previously, despite the simple idea of just working with a subset
of files, there are a range of different behavioral changes that need to be
@@ -544,7 +552,8 @@ understanding these differences can be beneficial.
* gitk?
-=== Behavior classes ===
+Behavior classes
+----------------
From the above there are a few classes of behavior:
@@ -611,7 +620,8 @@ From the above there are a few classes of behavior:
specification.
-=== Subcommand-dependent defaults ===
+Subcommand-dependent defaults
+-----------------------------
Note that we have different defaults depending on the command for the
desired behavior :
@@ -751,7 +761,8 @@ desired behavior :
implemented.
-=== Sparse specification vs. sparsity patterns ===
+Sparse specification vs. sparsity patterns
+------------------------------------------
In a well-behaved situation, the sparse specification is given directly
by the $GIT_DIR/info/sparse-checkout file. However, it can transiently
@@ -823,7 +834,8 @@ under behavior B index operations are lumped with history and tend to
operate full-tree.
-=== Implementation Questions ===
+Implementation Questions
+------------------------
* Do the options --scope={sparse,all} sound good to others? Are there better
options?
@@ -894,7 +906,8 @@ operate full-tree.
is seamless for them.
-=== Implementation Goals/Plans ===
+Implementation Goals/Plans
+--------------------------
* Get buy-in on this document in general.
@@ -922,13 +935,14 @@ operate full-tree.
commands. IMPORTANT: make sure diff machinery changes don't mess with
format-patch, fast-export, etc.
-=== Known bugs ===
+Known bugs
+----------
This list used to be a lot longer (see e.g. [1,2,3,4,5,6,7,8,9]), but we've
been working on it.
-0. Behavior A is not well supported in Git. (Behavior B didn't used to
- be either, but was the easier of the two to implement.)
+Behavior A is not well supported in Git. (Behavior B didn't used to
+be either, but was the easier of the two to implement.)
1. am and apply:
@@ -1052,7 +1066,8 @@ been working on it.
https://lore.kernel.org/git/CABPp-BEkJQoKZsQGCYioyga_uoDQ6iBeW+FKr8JhyuuTMK1RDw@mail.gmail.com/
-=== Reference Emails ===
+Reference Emails
+----------------
Emails that detail various bugs we've had in sparse-checkout:
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/2] ls-files: add %(skipworktree) atom to format option
From: ZheNing Hu via GitGitGadget @ 2023-01-19 17:34 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee, Victoria Dye, Elijah Newren,
Nguyễn Thái Ngọc Duy, ZheNing Hu
In-Reply-To: <pull.1458.git.1673451741587.gitgitgadget@gmail.com>
Add a %(skipworktree) atom git ls-files --format to indicate whether the
file in the index match the sparse specification.
v1: add %(skipworktree) atom to git ls-files format option. v2:
1. no longer mentioned git ls-files -t.
2. change %(skipworktree) output from "true"/"false" to "1"/"".
3. fix the sparse-checkout docs link.
ZheNing Hu (2):
docs: fix sparse-checkout docs link
ls-files: add %(skipworktree) atom to format option
Documentation/Makefile | 1 +
Documentation/git-ls-files.txt | 5 +++
Documentation/technical/sparse-checkout.txt | 43 ++++++++++++++-------
builtin/ls-files.c | 3 ++
t/t3013-ls-files-format.sh | 23 +++++++++++
5 files changed, 61 insertions(+), 14 deletions(-)
base-commit: a7caae2729742fc80147bca1c02ae848cb55921a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1458%2Fadlternative%2Fzh%2Fls-file-format-skipworktree-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1458/adlternative/zh/ls-file-format-skipworktree-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1458
Range-diff vs v1:
-: ----------- > 1: cde4827da13 docs: fix sparse-checkout docs link
1: c4cd5b3a32f ! 2: 9ebd6b77a69 ls-files: add %(skipworktree) atom to format option
@@ Commit message
ls-files: add %(skipworktree) atom to format option
Because sometimes we want to check if the files in the
- index match the sparse specification by using
- `git ls-files -t`, but `-t` option have semi-deprecated,
-
- So introduce "%(skipworktree)" atom to git ls-files
- `--format` option. When we use this option, if the file
- match the sparse specification and removed from working
- tree, it will output "yes", othewise, output "no".
+ index match the sparse specification, so introduce
+ "%(skipworktree)" atom to git ls-files `--format` option.
+ When we use this option, if the file match the sparse
+ specification, it will output "1", otherwise, output
+ empty string "".
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
@@ Documentation/git-ls-files.txt: eolattr::
The pathname of the file which is recorded in the index.
+skipworktree::
+ If the file in the index marked with SKIP_WORKTREE bit.
-+ It means the file do not match the sparse specification
-+ and removed from working tree.
++ It means the file do not match the sparse specification.
+ See link:technical/sparse-checkout.txt[sparse-checkout]
+ for more information.
@@ builtin/ls-files.c: static size_t expand_show_index(struct strbuf *sb, const cha
write_name_to_buf(sb, data->pathname);
+ else if (skip_prefix(start, "(skipworktree)", &p))
+ strbuf_addstr(sb, ce_skip_worktree(data->ce) ?
-+ "true" : "false");
++ "1" : "");
else
die(_("bad ls-files format: %%%.*s"), (int)len, start);
@@ t/t3013-ls-files-format.sh: test_expect_success 'git ls-files --format with --de
'
+test_expect_success 'git ls-files --format with skipworktree' '
++ test_when_finished "git sparse-checkout disable" &&
+ mkdir dir1 dir2 &&
+ echo "file1" >dir1/file1.txt &&
+ echo "file2" >dir2/file2.txt &&
+ git add dir1 dir2 &&
+ git commit -m skipworktree &&
+ git sparse-checkout set dir1 &&
-+ git ls-files --format="%(path) %(skipworktree)" >actual &&
++ git ls-files --format="%(path)%(skipworktree)" >actual &&
+ cat >expect <<-\EOF &&
-+ dir1/file1.txt false
-+ dir2/file2.txt true
-+ o1.txt false
-+ o2.txt false
-+ o3.txt false
-+ o4.txt false
-+ o5.txt false
-+ o6.txt false
-+ o7.txt false
++ dir1/file1.txt
++ dir2/file2.txt1
++ o1.txt
++ o2.txt
++ o3.txt
++ o4.txt
++ o5.txt
++ o6.txt
++ o7.txt
+ EOF
+ test_cmp expect actual
+'
--
gitgitgadget
^ permalink raw reply
* Is GGG mislabeling topics?
From: Junio C Hamano @ 2023-01-19 17:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
These comments from GGG bot
https://github.com/git/git/pull/1435#issuecomment-1386301994
https://github.com/git/git/pull/1435#issuecomment-1386302018
add 'next' and 'seen' labels, citing merges e3ead5f and c52b021
respectively, but these merges are of a topic that has little to do
with this pull request (#1435). Is this expected?
^ permalink raw reply
* [PATCH] checkout: document -b/-B to highlight the differences from "git branch"
From: Junio C Hamano @ 2023-01-19 17:12 UTC (permalink / raw)
To: git
The existing text read as if "git checkout -b/-B name" were
equivalent to "git branch [-f] name", which clearly was not
what we wanted to say.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-checkout.txt | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git i/Documentation/git-checkout.txt w/Documentation/git-checkout.txt
index 4cb9d555b4..dcff44196d 100644
--- i/Documentation/git-checkout.txt
+++ w/Documentation/git-checkout.txt
@@ -146,14 +146,16 @@ on your side branch as `theirs` (i.e. "one contributor's work on top
of it").
-b <new-branch>::
- Create a new branch named `<new-branch>` and start it at
- `<start-point>`; see linkgit:git-branch[1] for details.
+ Create a new branch named `<new-branch>`, start it at
+ `<start-point>`, and check the resulting branch out;
+ see linkgit:git-branch[1] for details.
-B <new-branch>::
- Creates the branch `<new-branch>` and start it at `<start-point>`;
- if it already exists, then reset it to `<start-point>`. This is
- equivalent to running "git branch" with "-f"; see
- linkgit:git-branch[1] for details.
+ Creates the branch `<new-branch>`, start it at `<start-point>`;
+ if it already exists, then reset it to `<start-point>`. And then
+ check the resulting branch out. This is equivalent to running
+ "git branch" with "-f" followed by "git checkout" of that branch;
+ see linkgit:git-branch[1] for details.
-t::
--track[=(direct|inherit)]::
^ permalink raw reply related
* [PATCH] mingw: replace deprecated GetVersion with RtlGetVersion
From: Rose via GitGitGadget @ 2023-01-19 17:07 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
From: Seija Kijin <doremylover123@gmail.com>
The previous way is deprecated and returns
the wrong value in Windows 8 and up,
returning the manifest Windows data
as opposed to the actual Windows data.
RtlGetVersion is the correct way to get the Windows version now.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
mingw: replace deprecated GetVersion with RtlGetVersion
GetVersion has its behavior changed in Windows 8 and above anyway, so
this is the right way to do it now.
The previous way returns the wrong value in Windows 8 and up, returning
the manifest Windows data as opposed to the actual Windows data.
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1438%2FAtariDreams%2Fmingw-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1438/AtariDreams/mingw-v1
Pull-Request: https://github.com/git/git/pull/1438
compat/mingw.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index af397e68a1d..ebd5850002a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3081,15 +3081,36 @@ int wmain(int argc, const wchar_t **wargv)
return exit_status;
}
+/*
+ * For RtlGetVersion in uname
+ */
+
+typedef NTSTATUS(WINAPI *RtlGetVersionPtr)(PRTL_OSVERSIONINFOW);
+union winprocaddr {
+ FARPROC procaddr;
+ RtlGetVersionPtr procGetVersion;
+};
+
int uname(struct utsname *buf)
{
- unsigned v = (unsigned)GetVersion();
+ union winprocaddr RtlGetVersionInternal;
+ OSVERSIONINFOA version;
+
+ RtlGetVersionInternal.procaddr =
+ GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "RtlGetVersion");
+ if (!RtlGetVersionInternal.procaddr) {
+ die_message_errno(
+ "Could not get handle to RtlGetVersion in ntdll.dll");
+ }
+
+ version.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+ RtlGetVersionInternal.procGetVersion((PRTL_OSVERSIONINFOW)&version);
+
memset(buf, 0, sizeof(*buf));
xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows");
- xsnprintf(buf->release, sizeof(buf->release),
- "%u.%u", v & 0xff, (v >> 8) & 0xff);
- /* assuming NT variants only.. */
- xsnprintf(buf->version, sizeof(buf->version),
- "%u", (v >> 16) & 0x7fff);
+ xsnprintf(buf->release, sizeof(buf->release), "%lu.%lu",
+ version.dwMajorVersion, version.dwMinorVersion);
+ xsnprintf(buf->version, sizeof(buf->version), "%lu",
+ version.dwBuildNumber);
return 0;
}
base-commit: a7caae2729742fc80147bca1c02ae848cb55921a
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v8 3/4] worktree add: add --orphan flag
From: Junio C Hamano @ 2023-01-19 16:32 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jacob Abel, phillip.wood, git, Eric Sunshine, Phillip Wood,
Rubén Justo, Taylor Blau, rsbecker
In-Reply-To: <230119.86zgaev8ko.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> I also think that UX suggestion is sensible, but if we do that we
> shouldn't just apply that to "git worktree", but also change the the
> corresponding "git switch" UX, on which this new "git worktree --orphan"
> is modeled.
But the thing is that "worktree --orphan" that wants to implicitly
infer the name of the branch out of the basename of the worktree
cannot be sensibly modeled after "switch" or "checkout" that do not
have such a dwimmery.
In any case, fixing UI mistakes after the fact is always painful
once it is in the released version. When we know there is a new UI
mistake in an unreleased version, it is sensible to grab the rare
opportunity that we can avoid such a costly fixes later.
So, I am not quite convinced by what you said, at least not yet.
Thanks.
^ permalink raw reply
* Re: [PATCH] git-cat-file.txt: fix list continuations rendering literally
From: Junio C Hamano @ 2023-01-19 16:24 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Siddharth Asthana
In-Reply-To: <CAN0heSokODyuYUEgaU8Ym_Evvmdp_y1-V0LxbsTECgm3sP9d-g@mail.gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
> On Wed, 18 Jan 2023 at 17:23, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Are you comparing both roff output and html output, by the way?
>
> Yes, I'm trying to make sure all four of
>
> {asciidoc, asciidoctor} x {man, html}
>
> agree and look good. For the manpages, I use our doc-diff script. (In
> this case, I wanted an empty asciidoc diff (HEAD^ HEAD) and a good
> Asciidoctor diff (s/+//-ish).) For the html, it's a bit more manual
> labour, switching between files in a browser and convincing myself
> everything is good, visually.
>
> Luckily, once both tools agree on the manpages and they look good, in my
> experience, the html is probably also ok.
Wonderful. Thanks for your careful work, as always.
^ permalink raw reply
* Re: [PATCH v8 3/4] worktree add: add --orphan flag
From: Phillip Wood @ 2023-01-19 16:18 UTC (permalink / raw)
To: Jacob Abel
Cc: git, Ævar Arnfjörð Bjarmason, Eric Sunshine,
Junio C Hamano, Phillip Wood, Rubén Justo, Taylor Blau,
rsbecker
In-Reply-To: <20230118224020.vrytmeyt3vbanoh2@phi>
On 18/01/2023 22:40, Jacob Abel wrote:
> On 23/01/16 10:47AM, Phillip Wood wrote:
>> Hi Jacob
>>
>> On 14/01/2023 22:47, Jacob Abel wrote:
>>> On 23/01/13 10:20AM, Phillip Wood wrote:
>>>> Hi Jacob
>>>>
>>> [...]
>>>
>>> I'll reply to this message with the one-off patch to get feedback. Since this is
>>> essentially a discrete change on top of v8, I can either keep it as a separate
>>> patch or reroll depending on how much needs to be changed (and what would be
>>> easier for everyone).
>>>
>>>> git worktree add --orphan -b topic main
>>>> git worktree add --orphan -B topic main
>>>
>>> I am hesitant to add these as they break away from the syntax used in
>>> `git switch` and `git checkout`.
>>
>> When I wrote my original email I wrongly though that --orphan did not
>> take an argument for "git checkout". While I think it is a mistake for
>> checkout and switch to have --orphan take an argument they do at least
>> always need a branch name with that option. "git worktree" add already
>> has the branch name in the form of the worktree directory in the common
>> case.
>
> Understood.
>
> I'm not entirely opposed to making this change to OPT_BOOL but I have to wonder
> how often `--orphan` will actually be used by a given user and whether the
> slightly shorter invocation will be used regularly.
>
> With the base `git worktree add $path`, the shorthand/DWYM makes sense as it's
> used regularly but I don't see users working with `--orphan` outside of trying
> to create the first branch in a repository.
Your example use in the commit message shows the user using the same
name for the branch and worktree. If that really is the likely use than
I think we should make --orphan OPT_BOOL. If it is not the likely use
perhaps you could update the commit message to show how you think it
will be used.
> And I'd like that operation of creating the first branch in a repo to eventually
> "just work" with the base command, i.e. `git worktree add main/`. The reason I
> hadn't yet added that is because I've yet to figure out how to get it to work
> without accidentally introducing potentially confusing situations and I didn't
> want to hold up introducing the core functionality itself.
>
> Once that main use-case "just works", I don't see users utilising `--orphan`
> except in very rare circumstances. Doubly so since the average user likely
> shouldn't be using `--orphan` in most cases.
This brings us back to the question that we discussed earlier of whether
we need --orphan at all. If we can get the main use-case working without
it we'd perhaps be better doing that rather than adding an option no one
ends up using.
Best Wishes
Phillip
> Hence the question of whether this change would be worth it vs the existing
> `--orphan $branchname $path` which is (for better or worse) consistent with `-b`
> and `-B`.
>
>>
>>> Also apologies for the tangent but while researching this path, I noticed that
>>> --orphan behaves unexpectedly on both `git switch` and `git checkout` when mixed
>>> with `-c` and `-b` respectively.
>>>
>>> % git switch --orphan -c foobar
>>> fatal: invalid reference: foobar
>>>
>>> % git switch -c --orphan foobar
>>> fatal: invalid reference: foobar
>>> % git checkout -b --orphan foobar
>>> fatal: 'foobar' is not a commit and a branch '--orphan' cannot be created from it
>>>
>>> % git checkout --orphan -b foobar
>>> fatal: 'foobar' is not a commit and a branch '-b' cannot be created from it
>>
>> The messages for checkout look better than the switch ones to me as they
>> show the branch name which makes it clearer that we're treating what
>> looks like an option as an argument. What in particular is unexpected
>> here - --orphan and -b take an argument so they'll hoover up the next
>> thing on the commandline whatever it is.
>>
>> Best Wishes
>>
>> Phillip
>>
>>> [...]
>
> Agreed. I wasn't sure if this would be something worth addressing in a patch but
> at the very least I can work on putting together a small patch for `git switch`
> since it doesn't seem to be hoovering the flags like `git checkout` does.
>
^ permalink raw reply
* Re: [PATCH v8 3/4] worktree add: add --orphan flag
From: Ævar Arnfjörð Bjarmason @ 2023-01-19 15:32 UTC (permalink / raw)
To: Jacob Abel
Cc: Junio C Hamano, phillip.wood, git, Eric Sunshine, Phillip Wood,
Rubén Justo, Taylor Blau, rsbecker
In-Reply-To: <20230118221745.wovefwx6vhcm3zzk@phi>
On Wed, Jan 18 2023, Jacob Abel wrote:
> On 23/01/14 07:09PM, Junio C Hamano wrote:
>> Jacob Abel <jacobabel@nullpo.dev> writes:
>>
>> >> git worktree add --orphan -b topic main
>> >> git worktree add --orphan -B topic main
>> >
>> > I am hesitant to add these as they break away from the syntax used in
>> > `git switch` and `git checkout`.
>>
>> Not that I care too deeply, but doesn't it introduce end-user
>> confusion if we try to be compatible with "git checkout --orphan
>> <branch>", while allowing this to be compatible with the default
>> choice of the branch name done by "git worktree add"? "--orphan" in
>> "git checkout" behaves similar to "-b|-B" in that it always wants a
>> name, but "git worktree add" wants to make it optional.
>
> Yes. I think it's a fairly minor degree of confusion but I agree that it adds
> potentially unneeded confusion.
I think this topic is ready to advance as-is without Phillip's upthread
suggestion (<e5aadd5d-9b85-4dc9-e9f7-117892b4b283@dunelm.org.uk>) to
allow us to combine --orphan and -b and -B.
I also think that UX suggestion is sensible, but if we do that we
shouldn't just apply that to "git worktree", but also change the the
corresponding "git switch" UX, on which this new "git worktree --orphan"
is modeled.
I don't think it's worth it to make the UX between the two inconsistent
in this regard, so if "switch" doesn't learn to do this we'd be better
off with not making "--orphan" a flag.
But if we are going to make it a flag let's have both support the same
sort of invocation.
Therefore I think this series is ready as-is without this proposed UX
change. We should first support the same sort of invocations that
"swich" already supports.
If we then want to change the UX later we should change it for both, not
leave the two inconsistent.
^ permalink raw reply
* Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees
From: Phillip Wood @ 2023-01-19 14:21 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón, git
Cc: pclouds, gitster, Jinwook Jeong, Rubén Justo, Eric Sunshine
In-Reply-To: <20230119055325.1013-1-carenas@gmail.com>
Hi Carlo
Thanks for working on this
On 19/01/2023 05:53, Carlo Marcelo Arenas Belón wrote:
> Commands `git switch -C` and `git checkout -B` neglect to check whether
> the provided branch is already checked out in some other worktree, as
> shown by the following:
>
> $ git worktree list
> .../foo beefb00f [main]
> $ git worktree add ../other
> Preparing worktree (new branch 'other')
> HEAD is now at beefb00f first
> $ cd ../other
> $ git switch -C main
> Switched to and reset branch 'main'
> $ git worktree list
> .../foo beefb00f [main]
> .../other beefb00f [main]
>
> Fix this problem by teaching `git switch -C` and `git checkout -B` to
> check whether the branch in question is already checked out elsewhere
> by expanding on the existing checks that are being used by their non
> force variants.
>
> Unlike what it is done for `git switch` and `git checkout`, that have
> an historical exception to ignore other workspaces if the branch to
> check is the current one (as required when called as part of other
> tools), the logic implemented is more strict and will require the user
> to invoke the command with `--ignore-other-worktrees` to explicitly
> indicate they want the risky behaviour.
> This matches the current behaviour of `git branch -f` and is safer, for
> more details see the tests in t2400.
I think it would be helpful to spell out the behavior of
git checkout $current_branch
git checkout -B $current_branch [<commit>]
git checkout -B $current_branch --ignore-other-worktrees [<commit>]
when the current branch is and is not checked out in another worktree
as the tests are hard to follow because they rely on worktrees set up
previous tests.
> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Helped-by: Rubén Justo <rjusto@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> @@ -1818,10 +1831,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> strbuf_release(&buf);
> }
>
> - if (opts->patch_mode || opts->pathspec.nr)
> + if (opts->patch_mode || opts->pathspec.nr) {
> + free(check_branch_path);
> return checkout_paths(opts, new_branch_info);
> + }
> else
> - return checkout_branch(opts, new_branch_info);
> + return checkout_branch(opts, new_branch_info, check_branch_path);
> }
I found the ownership of check_branch_path confusing here. I think it
would be clearer to do
if (opts->patch_mode || opts->pathspec.nr)
ret = checkout_path(...);
else
ret = checkout_branch(...);
free(check_branch_path);
return ret;
> [...]
> +test_expect_success 'but die if using force without --ignore-other-worktrees' '
I'm not sure from the title what this test is checking. Having added
"git worktree list" and run it is checking that when the current branch
is checked out elsewhere we require --ignore-other-worktrees when
resetting the current branch.
> + (
> + cd there &&
> + test_must_fail git checkout -B newmain &&
> + test_must_fail git switch -C newmain &&
> + git checkout --ignore-other-worktrees -B newmain &&
> + git switch --ignore-other-worktrees -C newmaain > )
I tried running
git switch -C main newbranch
from the main worktree (which is the only worktree that has branch
'main' checked out) to check that we did not error out when the branch
is not checked out elsewhere and was surprised it failed with
fatal: 'newbranch' is already checked out at '/dev/shm/trash
directory.t2400-worktree-add/short-hand'
It works as expected without this patch so it looks like there is a bug
somewhere.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree
From: Phillip Wood @ 2023-01-19 10:48 UTC (permalink / raw)
To: Rubén Justo, Junio C Hamano; +Cc: Git List, Eric Sunshine
In-Reply-To: <f209958f-e824-181b-f9d5-6e4bc8e53646@gmail.com>
Hi Rubén
On 18/01/2023 23:50, Rubén Justo wrote:
> On 17-ene-2023 15:27:29, Junio C Hamano wrote:
>
>> being solved. Rather, it is unclear what problem you are solving.
>
> Sorry, I didn't explain the motivation well in the message.
I'm afraid I'm still not totally clear from this message exactly what
the problem is. Having looked at die_if_checked_out() I think it maybe
the second of Junio's options
- If the current branch the the current worktree is checked out in
a different worktree, we get the current worktree back.
Here is the function:
void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
struct worktree **worktrees = get_worktrees();
const struct worktree *wt;
wt = find_shared_symref(worktrees, "HEAD", branch);
if (wt && (!ignore_current_worktree || !wt->is_current)) {
skip_prefix(branch, "refs/heads/", &branch);
die(_("'%s' is already checked out at '%s'"), branch, wt->path);
}
free_worktrees(worktrees);
}
It takes a flag to ignore the current worktree but uses
find_shared_symref() which does not have such a flag to see if the
branch is checked out. This means that if a branch is checkout out twice
find_shared_symref() may return the current worktree rather than the one
we're interested in.
If that is the problem you're trying to solve I think it would be better
to keep the signature of find_shared_symref() the same and add a helper
function that is called by die_if_checked_out() and find_shared_symref()
which can optionally ignore the current worktree. The commit message for
such a patch should explain you're fixing a bug rather than trying to
change the existing behavior.
Best Wishes
Phillip
> First, I'm not sure if "ignore_current_worktree" is correct, maybe needs
> to be "prefer_current_worktree". Having said that, let me use an example.
>
> With...
>
> $ git worktree add wt1 -b main
> $ git worktree add wt2 -f main
>
> ... we get confuse results:
>
> $ git -C wt1 rebase main main
> Current branch main is up to date.
> $ git -C wt2 rebase main main
> fatal: 'main' is already checked out...
>
> The problem I'm trying to solve is that find_shared_symref() returns the
> first matching worktree, and a possible matching "current worktree"
> might not be the first matching one. That's why die_if_checked_out()
> dies with "git -C wt2".
>
> find_shared_symref() searches through the list of worktrees that
> get_worktrees() composes: first the main worktree and then, as getdir()
> returns them, those in .git/worktrees/*. The search is sequential and
> once a match is found, it is returned. And so die_if_checked_out(),
> when asked to "ignore_current_worktree", is going to consider for
> "is_current" the worktree which may or may not be the "current" one,
> depending on the sequence from get_worktree(), and getdir() ultimately.
>
> If we want to disallow operations on a worktree with a checked out
> branch also on another worktree, we need the "ignore_current_worktree".
> But, and now I'm more in favor of this, if we prefer to allow the
> operation, we need a "prefer_current_worktree", to induce
> find_shared_symref() to return the "current" one if it matches, or any
> other one that matches.
^ permalink raw reply
* Re: [PATCH] grep: fall back to interpreter mode if JIT fails
From: Mathias Krause @ 2023-01-19 9:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Carlo Arenas, Junio C Hamano, git
In-Reply-To: <230118.86lelzx2c4.gmgdl@evledraar.gmail.com>
On 18.01.23 16:44, Ævar Arnfjörð Bjarmason wrote:
>>> [snip]
>>> But silently falling back kind of sucks, but unfortunately pcre2 doesn't
>>> provide a way to say "failed because of SELinux" v.s. "failed because
>>> the pattern is crazy", except that we could try to compile a known-good
>>> pattern with the JIT, to disambiguate the two.
>>
>> Exactly, so what about something like this:
>>
>> If JIT is generally available, try to JIT the user supplied pattern:
>> 1/ If it fails with PCRE2_ERROR_NOMEMORY, try to compile a know good
>> pattern, e.g. ".":
>> 1a/ If this fails with PCRE2_ERROR_NOMEMORY as well, fall back to
>> interpreter, as JIT is non-functional because of SELinux / PaX.
>> 1b/ If not, it's a "crazy" pattern, suggest '(*NO_JIT)'.
>> 2/ If it succeeds or fails with a different error, continue as of now,
>> i.e. use JIT on success or die() on error, optionally suggesting
>> '(*NO_JIT)'.
>>
>> That should handle the case you're concerned about and only fall back to
>> interpreter mode if JIT won't be functional anyway. Admitted, this would
>> also allow crazy patterns, but there's simply no way to detect these
>> under such constraints.
>
> That sounds good, i.e. we could narrow the JIT falling back case to
> these SELinux cases and the like, distinct from generic internal errors.
>
> Maybe it's too much paranoia, but it should work & get rid of the
> ambiguity.
Nah, it's fine.
>>> [snip]
>>> See above, but maybe it's the least sucky thing (and definitely
>>> simpler). I'm mainly checking that we're doing that we want here, and
>>> that we're going into it with eyes open.
>>>
>>> That we're now discussing a topic entirely different from SELinux on a
>>> thread where we're (according to the commit message) fixing pcre2 where
>>> the JIT is "unusable on such systems" is my main concern here.
>>
>> Yeah, I overlooked that angle initially, but it's a valid concern.
>> However, limiting the functional change of falling back to interpreter
>> mode on "JIT's broken anyway" systems only should address these and get
>> me a functional 'git grep' again.
>
> *nod*
>
>>>>> [snip]
>>>
>>> To summarize some of the above, I think performance also matters, we
>>> have cases where:
>>>
>>> A. We could use the non-JIT
>>> B. We could use the JIT, and it's a *lot* faster
>>> C. We can't use the JIT at all
>>> D. We can't use the JIT because we run into its limits
>>>
>>> I think it's fair to die on "D" as in practice you only (I think!) run
>>> into it on pathological patterns, but yes, another option would be to
>>> fall back to "A".
>>>
>>> But thinking you're doing "B" and not wanting to implicitly fall back to
>>> "A" is also a valid use-case.
>>
>> Agreed. My above sketched handling should do that, as in not falling
>> back to interpreter mode when the JIT would be functional per se, but
>> just failed on this particular pattern.
>>
>>> So I'm inclined to suggest that we should be less helpful with automatic
>>> fallbacks, and just suggest a "try it with '(*NO_JIT)'" advice() or
>>> something.
>>
>> Well, that's a real pain from a user's (my) point of view. Trust me, I'm
>> suffering from this, otherwise I wouldn't have brought up the issue ;)
>
> That's fair enough, falling back in the "D" case sounds good.
>
>>> But as noted above needing to always disable an apparently "available"
>>> JIT on some systems (SELinux) does throw a monkey wrench into that
>>> particular suggestion :(
>>
>> Yep.
>>
>>> So I'm not sure, I'm mainly trying to encourage you to think through the
>>> edge cases, and to summarize the full impact of the change in a re-roll.
>>
>> Yeah, I agree. The implied fallback to the interpreter, even for the
>> more obscure cases should have been mentioned in the changelog, but I
>> overlooked / ignored that case initially.
>>
>> My take from the discussion is to do a re-roll with something like this:
>>
>> if (p->pcre2_jit_on) {
>> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
>> if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) {
>> /* JIT's non-functional because of SELinux / PaX */
>> p->pcre2_jit_on = 0;
>> return;
>> } else if (jitret) {
>> die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n"
>> "Try prefixing the pattern with '(*NO_JIT)'\n",
>> p->pattern, jitret);
>> }
>> ...
>> }
>>
>> ...with pcre2_jit_functional() being something like this:
>>
>> static int pcre2_jit_functional(void)
>> {
>> pcre2_code *code;
>> size_t off;
>> int err;
>>
>> code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL);
>> if (!code)
>> return 0;
>>
>> err = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE);
>> pcre2_code_free(code);
>>
>> return err == 0;
>> }
>
> This seems sensible as pseudocode, although please don't add another
> pcre2_compile() entry point (as e.g. passing NULL here will bypass the
> context we carefully set up, and if we have a custom allocator...).
That was intentional, actually. I specifically want to use the most
basic API to test for "general JIT availability." That test should tell
if PCRE2 JIT is working /in general/, not specifically how we make use
of it in git, which might not. However, that would be a different error,
i.e. not because PCRE2 failed to allocate JIT memory but us using the
API wrong, hitting limitations, etc.
Making use of the PCRE2 context we set up in compile_pcre2_pattern()
(and thereby possibly generating errors because of it) could mask API
usage errors resulting from these and may lead to a false fallback to
interpreter mode when we want to die() instead?
> Instead you could just re-enter the API itself via
> compile_grep_patterns(), or perhaps lower via
> compile_pcre2_pattern(). Then add some flag to "struct grep_opt" to
> indicate that we shouldn't die() or BUG() there, but just run a "jit
> test".
This feels kinda hacky and overkill, actually. A dedicated simplistic
test looks much cleaner, IMHO.
> This code also treats all failures of pcre2_jit_compile() the same, but
> here we only want PCRE2_ERROR_NOMEMORY.
Does it really matter? I mean, we could change the final test to 'err !=
PCRE2_ERROR_NOMEMORY' but we also return early when we're unable to
generate PCRE2 code for the "." pattern -- not strictly a JIT error as well.
I'd say instead of splitting hairs, pcre2_jit_functional() should stay
as above, as a failure to compile "." as a pattern can only happen (a)
under very tight memory constraints or (b) for JIT internal reasons,
which would boil down to not being able to allocate the required
resources to generate JIT code --- it's such a simple pattern it's not
expected to fail in any other way. Case (a) can be ignored, IMHO, as
that would lead to follow up errors soon anyway.
Thanks,
Mathias
^ permalink raw reply
* Re: [PATCH] hash-object: fix descriptor leak with --literally
From: Jeff King @ 2023-01-19 8:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtu0n2g67.fsf@gitster.g>
On Wed, Jan 18, 2023 at 10:26:40PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > In hash_object(), we open a descriptor for each file to hash (whether we
> > got the filename from the command line or --stdin-paths), but never
> > close it. For the traditional code path which feeds the result to
> > index_fd(), this is OK; it closes the descriptor for us.
> >
> > But 5ba9a93b39 (hash-object: add --literally option, 2014-09-11) a
> > second code path which does not close the descriptor.
>
> A sentence without verb? "5ba9 (hash-...) added a second code path,
> which does not close the descriptor." or something?
Yes, the missing word was "added". Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH] git-cat-file.txt: fix list continuations rendering literally
From: Martin Ågren @ 2023-01-19 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Siddharth Asthana
In-Reply-To: <xmqqzgaf4xrf.fsf@gitster.g>
On Wed, 18 Jan 2023 at 17:23, Junio C Hamano <gitster@pobox.com> wrote:
>
> Are you comparing both roff output and html output, by the way?
Yes, I'm trying to make sure all four of
{asciidoc, asciidoctor} x {man, html}
agree and look good. For the manpages, I use our doc-diff script. (In
this case, I wanted an empty asciidoc diff (HEAD^ HEAD) and a good
Asciidoctor diff (s/+//-ish).) For the html, it's a bit more manual
labour, switching between files in a browser and convincing myself
everything is good, visually.
Luckily, once both tools agree on the manpages and they look good, in my
experience, the html is probably also ok.
Martin
^ permalink raw reply
* Re: Re* [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees
From: Carlo Arenas @ 2023-01-19 7:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, pclouds, Jinwook Jeong, Rubén Justo, Eric Sunshine
In-Reply-To: <xmqqa62f2dj1.fsf_-_@gitster.g>
Thanks, that should round up the documentation for this behaviour nicely.
Carlo
^ permalink raw reply
* Re* [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees
From: Junio C Hamano @ 2023-01-19 7:23 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón
Cc: git, pclouds, Jinwook Jeong, Rubén Justo, Eric Sunshine
In-Reply-To: <20230119055325.1013-1-carenas@gmail.com>
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> Changes since v2
> * A leak free implementation
> * More details in commit as suggested by Junio
I meant to say we may need more details in the documentation, but
after reading the existing documentation, we say that
- "-B <name>" is equivalent to run "branch -f <name>", which is
sufficient to hint that it will fail to recreate and check out an
existing branch that is checked out elsewhere, because "branch
-f" would fail in such a situation.
and that
- "--ignore-other-worktrees" defeats the safety that makes "git
checkout refuses when the wanted ref is already checked out".
so the existing documentation of "git checkout" may already be OK.
As long as it is well known that "git branch -f" still fails in the
situation, that is. After re-reading "git branch --help" twice,
however, I am not sure if it is so clear, though.
How about adding something like this, as an independent
documentation improvement?
----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] branch: document `-f` and linked worktree behaviour
"git branch -f name start" forces to recreate the named branch, but
the forcing does not defeat the "do not touch a branch that is
checked out elsewhere" safety valve.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-branch.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git c/Documentation/git-branch.txt w/Documentation/git-branch.txt
index aa2f78c4c2..b12e7940d3 100644
--- c/Documentation/git-branch.txt
+++ w/Documentation/git-branch.txt
@@ -123,6 +123,10 @@ OPTIONS
points to a valid commit. In combination with
`-m` (or `--move`), allow renaming the branch even if the new
branch name already exists, the same applies for `-c` (or `--copy`).
++
+Note that 'git branch -f <branchname> [<start-point>]' refuses to change
+an existing branch `<branchname>` that is checked out in another worktree
+linked to the same repository.
-m::
--move::
^ permalink raw reply related
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