* Re: [GSOC][PATCH] userdiff: Add JavaScript function patterns
From: Patrick Steinhardt @ 2024-03-04 9:04 UTC (permalink / raw)
To: Sergius Nyah; +Cc: git, christian.couder, gitster, pk, shyamthakkar001
In-Reply-To: <20240301074048.188835-1-sergiusnyah@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]
On Fri, Mar 01, 2024 at 08:40:48AM +0100, Sergius Nyah wrote:
> This commit adds a patterns used to match JavaScript functions.
> It now correctly identifies function declarations, function expressions,
> and functions defined inside blocks. Add test for corresponding change in userdiff.
>
> Signed-off-by: Sergius Nyah <sergiusnyah@gmail.com>
> ---
> t/t4018-diff-funcname.sh | 22 ++++++++++++++++++++++
> userdiff.c | 12 ++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
> index e026fac1f4..d35cce18a0 100755
> --- a/t/t4018-diff-funcname.sh
> +++ b/t/t4018-diff-funcname.sh
> @@ -120,3 +120,25 @@ do
> done
>
> test_done
> +
> +test_expect_success 'identify builtin patterns in JavaScript' '
> + # setup
> + echo "function myFunction() { return true; }" > test.js &&
> + echo "var myVar = function() { return false; }" >> test.js &&
> + git add test.js &&
> + git commit -m "add test.js" &&
> +
> + # modify the file
> + echo "function myFunction() { return false; }" > test.js &&
> + echo "var myVar = function() { return true; }" >> test.js &&
> +
> + # command under test
> + git diff >output &&
> +
> + # check results
> + test_i18ngrep "function myFunction() { return true; }" output &&
> + test_i18ngrep "function myFunction() { return false; }" output &&
> + test_i18ngrep "var myVar = function() { return false; }" output &&
> + test_i18ngrep "var myVar = function() { return true; }" output
> +'
> +test_done
> \ No newline at end of file
This `test_done` only needs to be added because you add the new test
before the preceding `test_done`. Instead, you should move up this test
so that it comes before it.
> diff --git a/userdiff.c b/userdiff.c
> index 2b1dab2649..bbe2bcb9a3 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> +PATTERNS("javascript",
> + /* Looks for lines that start with optional whitespace, followed
Multi-line comments should start with their delimiters on separate
lines. So the "/*" should be on its own line.
Also, the code should be indented with tabs and not spaces. It might
help to read through Documentation/CodingGuidelines to get more familiar
with Git's coding style.
Patrick
> + * by 'function'* and any characters (for function declarations),
> + * or valid JavaScript identifiers, equals sign '=', 'function' keyword
> + * and any characters (for function expressions).
> + * Also considers functions defined inside blocks with '{...}'.
> + */
> + "^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
> + /* This pattern matches JavaScript identifiers */
> + "[a-zA-Z_$][0-9a-zA-Z_$]*"
> + "|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
> + "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
> --
> 2.43.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject
From: Patrick Steinhardt @ 2024-03-04 9:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, Kaartic Sivaraam
In-Reply-To: <xmqqzfvjf5tq.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]
One of our proposed microprojects is to convert instances of `test -e`
and related functions to instead use `test_path_exists` or similar. This
conversion is only feasible when `test -e` is not used as part of a
control statement, as the replacement is used to _assert_ a condition
instead of merely testing for it.
Clarify the microproject's description accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
SoC-2024-Microprojects.md | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md
index 644c0a6..782441f 100644
--- a/SoC-2024-Microprojects.md
+++ b/SoC-2024-Microprojects.md
@@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea.
Find one test script that verifies the presence/absence of
files/directories with 'test -(e|f|d|...)' and replace them with the
appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper
-functions.
+functions. Note that this conversion does not directly apply to control
+flow constructs like `if test -e ./path; then ...; fi` because the
+replacements are intended to assert the condition instead of merely
+testing for it.
If you can't find one please tell us, along with the command you used
to search, so that we can remove this microproject idea.
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command
From: Patrick Steinhardt @ 2024-03-04 9:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, shejialuo, git
In-Reply-To: <xmqqzfvjf5tq.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]
On Thu, Feb 29, 2024 at 11:06:41AM -0800, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> >> @@ -175,7 +175,7 @@ match() {
> >> test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
> >> - if test -e .git/created_test_file
> >> + if test_path_exists .git/created_test_file
> >> then
> >> git reset &&
> >
> > ... which _do_ use test_path_exists() within a `test_expect_success`
> > block. However, the changes are still undesirable because, as above,
> > this `test -e` is merely part of the normal control-flow; it's not
> > acting as an assertion, thus test_path_exists() -- which is an
> > assertion -- is not correct.
> >
> > Unfortunately, none of the uses of`test -e` in t3070 are being used as
> > assertions worthy of replacement with test_path_exists(), thus this
> > isn't a good script in which to make such changes.
>
> It seems that there is a recurring confusion among mentorship
> program applicants that use test_path_* helpers as their practice
> material. Perhaps the source of the information that suggests it as
> a microproject is poorly phrased and needs to be rewritten to avoid
> misleading them.
>
> I found one at https://git.github.io/Outreachy-23-Microprojects/,
> which can be one source of such confusion:
>
> Find one test script that verifies the presence/absence of
> files/directories with ‘test -(e|f|d|…)’ and replace them
> with the appropriate test_path_is_file, test_path_is_dir,
> etc. helper functions.
>
> but there may be others.
>
> This task specification does not differenciate "test -[efdx]" used
> as a conditional of a control flow statement (which should never be
> replaced by test_path_* helpers) and those used to directly fail the
> &&-chain in test_expect_success with their exit status (which is the
> target that test_path_* helpers are meant to improve).
Good point. I've sent a patch in reply to your message that hopefully
clarifies this a bit. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/1] [PATCH] t9117: prefer test_path_* helper functions
From: Patrick Steinhardt @ 2024-03-04 9:24 UTC (permalink / raw)
To: shejialuo; +Cc: git, Eric Sunshine, Junio C Hamano
In-Reply-To: <20240301130334.135773-2-shejialuo@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]
On Fri, Mar 01, 2024 at 09:03:34PM +0800, shejialuo wrote:
> test -(e|f) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir instead.
Nit: you mention `test -e` and `test -f`, but then talk about
`test_path_exists` (correct) and `test_path_is_dir` (wrong). You
probably meant to write `test -(e|d)`.
Other than that all the conversions look correct to me. Thanks!
Patrick
>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
> t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
> index 62de819a44..3b038c338f 100755
> --- a/t/t9117-git-svn-init-clone.sh
> +++ b/t/t9117-git-svn-init-clone.sh
> @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
> test_expect_success 'basic clone' '
> test ! -d trunk &&
> git svn clone "$svnrepo"/project/trunk &&
> - test -d trunk/.git/svn &&
> - test -e trunk/foo &&
> + test_path_is_dir trunk/.git/svn &&
> + test_path_exists trunk/foo &&
> rm -rf trunk
> '
>
> test_expect_success 'clone to target directory' '
> test ! -d target &&
> git svn clone "$svnrepo"/project/trunk target &&
> - test -d target/.git/svn &&
> - test -e target/foo &&
> + test_path_is_dir target/.git/svn &&
> + test_path_exists target/foo &&
> rm -rf target
> '
>
> test_expect_success 'clone with --stdlayout' '
> test ! -d project &&
> git svn clone -s "$svnrepo"/project &&
> - test -d project/.git/svn &&
> - test -e project/foo &&
> + test_path_is_dir project/.git/svn &&
> + test_path_exists project/foo &&
> rm -rf project
> '
>
> test_expect_success 'clone to target directory with --stdlayout' '
> test ! -d target &&
> git svn clone -s "$svnrepo"/project target &&
> - test -d target/.git/svn &&
> - test -e target/foo &&
> + test_path_is_dir target/.git/svn &&
> + test_path_exists target/foo &&
> rm -rf target
> '
>
> --
> 2.44.0
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/4] t-ctype: simplify unit test definitions
From: Christian Couder @ 2024-03-04 9:25 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Phillip Wood, Josh Steadmon, Achu Luma
In-Reply-To: <20240303101330.20187-1-l.s.r@web.de>
On Sun, Mar 3, 2024 at 11:13 AM René Scharfe <l.s.r@web.de> wrote:
>
> Simplify the ctype unit tests to allow combining specification strings
> in any order and no longer require repeating class names.
>
> Changes since v1:
> * Added checks to guard string length calculation using sizeof.
> * Kept the definition string in the error output.
> * Added patches 2 and 3 with further cosmetic changes.
> * Dropped the last patch with the huge output for now, as it needs
> further thought.
This V2 looks good to me. Acked.
^ permalink raw reply
* Re: [GSOC][PATCH v2 1/1] t7301: use test_path_is_(missing|file)
From: Patrick Steinhardt @ 2024-03-04 9:31 UTC (permalink / raw)
To: Vincenzo Mezzela; +Cc: git
In-Reply-To: <20240227161734.52830-2-vincenzo.mezzela@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 21248 bytes --]
On Tue, Feb 27, 2024 at 05:17:34PM +0100, Vincenzo Mezzela wrote:
> Refactor test -(f|e) to utilize the corresponding helper functions from
Nit: you didn't convert any instances of `test -e`, so I'd simplify the
message to just say `test -f` here.
> test-lib-functions.sh. These functions perform indentical operations
> while enhancing debugging capabilities in case of test failures.
>
> In the context of this file, 'test ! -f' is meant to check if the file
> has been correctly cleaned, thus its usage is replaced with
> 'test_path_is_missing' instead of '! test_path_is_file'.
>
>
Another nit: There should only be a single empty line between body and
trailer lines.
Other than that this patch looks good to me, thanks!
Patrick
> Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
> ---
> t/t7301-clean-interactive.sh | 490 +++++++++++++++++------------------
> 1 file changed, 245 insertions(+), 245 deletions()
>
> diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
> index d82a3210a1..4afe53c66a 100755
> --- a/t/t7301-clean-interactive.sh
> +++ b/t/t7301-clean-interactive.sh
> @@ -25,18 +25,18 @@ test_expect_success 'git clean -i (c: clean hotkey)' '
> touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
> docs/manual.txt obj.o build/lib.so &&
> echo c | git clean -i &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test ! -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_missing src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -46,18 +46,18 @@ test_expect_success 'git clean -i (cl: clean prefix)' '
> touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
> docs/manual.txt obj.o build/lib.so &&
> echo cl | git clean -i &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test ! -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_missing src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -67,18 +67,18 @@ test_expect_success 'git clean -i (quit)' '
> touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
> docs/manual.txt obj.o build/lib.so &&
> echo quit | git clean -i &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -88,18 +88,18 @@ test_expect_success 'git clean -i (Ctrl+D)' '
> touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
> docs/manual.txt obj.o build/lib.so &&
> echo "\04" | git clean -i &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -110,18 +110,18 @@ test_expect_success 'git clean -id (filter all)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines f "*" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -132,18 +132,18 @@ test_expect_success 'git clean -id (filter patterns)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines f "part3.* *.out" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test ! -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test ! -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_missing docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_missing src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -154,18 +154,18 @@ test_expect_success 'git clean -id (filter patterns 2)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines f "* !*.out" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -176,18 +176,18 @@ test_expect_success 'git clean -id (select - all)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s "*" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test ! -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test ! -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_missing docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_missing src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -198,18 +198,18 @@ test_expect_success 'git clean -id (select - none)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -220,18 +220,18 @@ test_expect_success 'git clean -id (select - number)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s 3 "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -242,18 +242,18 @@ test_expect_success 'git clean -id (select - number 2)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s "2 3" 5 "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test ! -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_missing docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -264,18 +264,18 @@ test_expect_success 'git clean -id (select - number 3)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s "3,4 5" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -285,11 +285,11 @@ test_expect_success 'git clean -id (select - filenames)' '
> touch a.out foo.txt bar.txt baz.txt &&
> test_write_lines s "a.out fo ba bar" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test ! -f a.out &&
> - test ! -f foo.txt &&
> - test ! -f bar.txt &&
> - test -f baz.txt &&
> + test_path_is_file Makefile &&
> + test_path_is_missing a.out &&
> + test_path_is_missing foo.txt &&
> + test_path_is_missing bar.txt &&
> + test_path_is_file baz.txt &&
> rm baz.txt
>
> '
> @@ -301,18 +301,18 @@ test_expect_success 'git clean -id (select - range)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s "1,3-4" 2 "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test ! -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test ! -f docs/manual.txt &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_missing docs/manual.txt &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -323,18 +323,18 @@ test_expect_success 'git clean -id (select - range 2)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s "4- 1" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test ! -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_missing src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -345,18 +345,18 @@ test_expect_success 'git clean -id (inverse select)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines s "*" "-5- 1 -2" "" c |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -367,18 +367,18 @@ test_expect_success 'git clean -id (ask)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines a Y y no yes bad "" |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test ! -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_missing docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -389,18 +389,18 @@ test_expect_success 'git clean -id (ask - Ctrl+D)' '
> docs/manual.txt obj.o build/lib.so &&
> test_write_lines a Y no yes "\04" |
> git clean -id &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -412,18 +412,18 @@ test_expect_success 'git clean -id with prefix and path (filter)' '
> (cd build/ &&
> test_write_lines f docs "*.h" "" c |
> git clean -id ..) &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -435,18 +435,18 @@ test_expect_success 'git clean -id with prefix and path (select by name)' '
> (cd build/ &&
> test_write_lines s ../docs/ ../src/part3.c ../src/part4.c "" c |
> git clean -id ..) &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test -f a.out &&
> - test ! -f docs/manual.txt &&
> - test ! -f src/part3.c &&
> - test -f src/part3.h &&
> - test ! -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_file a.out &&
> + test_path_is_missing docs/manual.txt &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_file src/part3.h &&
> + test_path_is_missing src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> @@ -458,18 +458,18 @@ test_expect_success 'git clean -id with prefix and path (ask)' '
> (cd build/ &&
> test_write_lines a Y y no yes bad "" |
> git clean -id ..) &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test ! -f docs/manual.txt &&
> - test -f src/part3.c &&
> - test ! -f src/part3.h &&
> - test -f src/part4.c &&
> - test -f src/part4.h &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_missing docs/manual.txt &&
> + test_path_is_file src/part3.c &&
> + test_path_is_missing src/part3.h &&
> + test_path_is_file src/part4.c &&
> + test_path_is_file src/part4.h &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so
>
> '
>
> --
> 2.34.1
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] Fix git-p4 decode() missing an assignment
From: Patrick Steinhardt @ 2024-03-04 9:36 UTC (permalink / raw)
To: W Sero via GitGitGadget; +Cc: git, W Sero
In-Reply-To: <pull.1683.git.git.1709542455728.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2306 bytes --]
On Mon, Mar 04, 2024 at 08:54:15AM +0000, W Sero via GitGitGadget wrote:
> From: SaNeOr <sane0r@outlook.com>
>
> bugfix: When using git-p4 in the python2 environment,
> some places decode() missing an assignment.
>
> Signed-off-by: W Sero <sane0r@outlook.com>
> ---
> Fix git-p4 decode_path() missing an assignment
>
> When using git-p4 in the python2 environment, some places decode( )
> missing an assignment.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1683%2FSaNeOr%2Fmaster-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1683/SaNeOr/master-v1
> Pull-Request: https://github.com/git/git/pull/1683
>
> git-p4.py | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 28ab12c72b6..9fa4b9b104e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -307,7 +307,7 @@ def decode_path(path):
> return path.decode(encoding, errors='replace') if isinstance(path, bytes) else path
> else:
> try:
> - path.decode('ascii')
> + path = path.decode('ascii')
Is this fixing an actual bug that you have encountered? I'm mostly
asking because I think this actually works as inteded: we only try to
decode the path using the specified encoding in case where it is not
representable as ASCII. If it contained e.g. Unicode characters, then
this statement here would throw and we end up decoding in the `except`
branch. Otherwise, we know that the path only contains ASCII characters
and thus we don't have to change it in the first place.
Whether that complexity is sensible might be a different question. But
it at least shouldn't result in any user visible bug, no?
Patrick
> except:
> path = path.decode(encoding, errors='replace')
> if verbose:
> @@ -3114,7 +3114,7 @@ def writeToGitStream(self, gitMode, relPath, contents):
>
> def encodeWithUTF8(self, path):
> try:
> - path.decode('ascii')
> + path = path.decode('ascii')
> except:
> encoding = 'utf8'
> if gitConfig('git-p4.pathEncoding'):
>
> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
> --
> gitgitgadget
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Query about gitignore
From: Patrick Steinhardt @ 2024-03-04 9:51 UTC (permalink / raw)
To: Divyaditya Singh; +Cc: git
In-Reply-To: <A0502A2F-970B-46F3-B216-E82B2258DF3A@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4824 bytes --]
Hi,
adding the Git mailing list back to Cc.
On Thu, Feb 22, 2024 at 06:10:18PM +0530, Divyaditya Singh wrote:
> Hello there, I wish you a wonderful day. It’s been a while since I
> have written to you. I was curious if you had any suggestions or ideas
> for my particular need?
>
> Thank You
> Divyaditya
sorry for my late reply, but I've been in office only sporadically
during the last two weeks.
My original question for your actual usecase was written with the intent
to trigger other answers on the mailing list from potentially-interested
folks. So kind of trying to gather ideas or alternative solutions from
folks other than myself. Well, that didn't quite work out :)
> > On 07-Feb-2024, at 11:52 AM, Divyaditya Singh
> > <divyadityasnaruka@gmail.com> wrote:
> >
> > Hello Patrick,
> >
> > Thank you so much for taking time to respond to my email.
> >
> > Yes, I basically want to ignore “dir/“ if and only if if “dir/a”
> > exists to match the pattern in the gitignore. I am not great at
> > explaining but I would try to explain my use case to the best of my
> > abilities.
Okay. That is not currently possible with gitignore patterns.
> > Some Context: I am a undergrad student and I am trying to currently
> > build my portfolio website and I have planned some interesting stuff
> > for the same and to showcase my abilities and working to potential
> > future employers I have made the repository public.
> >
> > Use Case: While I am working on it, I plan to write some blogs about
> > some interesting projects that I have been building along side that
> > I am passionate about for which I am using markdown files to
> > generate blog posts.
> >
> > Now, I set up my Next.js project such as the server scans the
> > “posts/“ directory which will contain all my posts. Each post is a
> > directory with the post slug as the directory’s name and each
> > directory contains the structure for my post with index.md as the
> > post text and along with other files related to the post. Now, I
> > don’t want to upload unfinished blogs to the github and want the
> > ability to work on different blogs simultaneously with the changes
> > reflecting on the local device while working on the portfolio
> > website itself.
> >
> > So, in my original approach I thought it would be really simple it I
> > could just name it as “index.draft.md” and the entire post it not
> > saved to git, instead of creating branches or work trees or creating
> > an gitignored directory to store all incomplete posts and moving
> > directories all the time.
Well, my first thought regarding your usecase is "branches". They seem
to fit the bill quite nicely: you have separate tracks of unfinished
work which you don't yet want to be part of your main branch. So if
every blog post lived on its own branch where you refine it until you
are happy with it, then you can merge them into the main branch once
done.
The huge downside of gitignore is that those not-yet-finished blog posts
would... well, be ignored. That is you cannot save intermediate state
that you have, you cannot see how those blog posts evolved over time,
you basically lose all that Git is designed to help you with.
So I'd really recommend to give them a second thought, but in any case
your mileage may vary.
> > I apologise again if my approach or way of thinking is stupid and
> > for bothering you with this long email.
No worries, neither your approach nor your way of thinking is stupid.
Everybody will have different workflows, and what works best for one
person may not work at all for a different one.
Patrick
> >
> > Thank You,
> > Divyaditya Singh
> >
> >
> >> On 07-Feb-2024, at 11:13 AM, Patrick Steinhardt <ps@pks.im> wrote:
> >>
> >> Hi Divyaditya,
> >>
> >> On Wed, Feb 07, 2024 at 01:56:34AM +0530, Divyaditya Singh wrote:
> >>> Hello there,
> >>>
> >>> I hope you are having a wonderful day.
> >>>
> >>> I apologize if this is inappropriate but I wanted to ask is there a
> >>> way that I can make my .gitignore such that it ignores the entire
> >>> parent directory of a matching file.
> >>
> >> Not inappropriate at all!
> >>
> >> Rephrasing what you say, you basically want to ignore "dir/" if and only
> >> if "dir/a" exists and matches a specific pattern in your gitignore,
> >> right? If so, this is not something that you can currently achieve with
> >> gitignores.
> >>
> >> Would you mind maybe explaining your particular usecase a bit more? This
> >> _could_ help to find a different solution for you. But even if there is
> >> none it might motivate others to think about possible ways to implement
> >> this in Git if there is interest.
> >>
> >> Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names
From: Phillip Wood @ 2024-03-04 9:51 UTC (permalink / raw)
To: René Scharfe, git; +Cc: Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <20240303101330.20187-5-l.s.r@web.de>
Hi René
On 03/03/2024 10:13, René Scharfe wrote:
> TEST_CTYPE_FUNC defines a function for testing a character classifier,
> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.
>
> Avoid the need to define a class-specific function by letting
> TEST_CHAR_CLASS do all the work. This is done by using the internal
> functions test__run_begin() and test__run_end(), but they do exist to be
> used in test macros after all.
Those internal functions exist to implement the TEST() macro, they are
not really intended for use outside that (which is why they are marked
as private in the header file). If we ever want to update the
implementation of TEST() it will be a lot harder if we're using the
internal implementation directly in test files. Unit tests should be
wrapping TEST() if it is appropriate but not the internal implementation
directly.
Ideally we wouldn't need TEST_CTYPE_FUNC as there would only be a single
function that was passed a ctype predicate, an input array and an array
of expected results. Unfortunately I don't think that is possible due
the the way the ctype predicates are implemented. Having separate macros
to define the test function and to run the test is annoying but I don't
think it is really worth exposing the internal implementation just to
avoid it.
The other patches here look like useful improvements - thanks.
Best Wishes
Phillip
> Alternatively we could unroll the loop to provide a very long expression
> that tests all 256 characters and EOF and hand that to TEST, but that
> seems awkward and hard to read.
>
> No change of behavior or output intended.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> t/unit-tests/t-ctype.c | 64 ++++++++++++++++--------------------------
> 1 file changed, 24 insertions(+), 40 deletions(-)
>
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> index 02d8569aa3..d6ac1fe678 100644
> --- a/t/unit-tests/t-ctype.c
> +++ b/t/unit-tests/t-ctype.c
> @@ -1,19 +1,19 @@
> #include "test-lib.h"
>
> -/* Macro to test a character type */
> -#define TEST_CTYPE_FUNC(func, string) \
> -static void test_ctype_##func(void) { \
> +#define TEST_CHAR_CLASS(class, string) do { \
> size_t len = ARRAY_SIZE(string) - 1 + \
> BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \
> BUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \
> - for (int i = 0; i < 256; i++) { \
> - if (!check_int(func(i), ==, !!memchr(string, i, len))) \
> - test_msg(" i: 0x%02x", i); \
> + int skip = test__run_begin(); \
> + if (!skip) { \
> + for (int i = 0; i < 256; i++) { \
> + if (!check_int(class(i), ==, !!memchr(string, i, len)))\
> + test_msg(" i: 0x%02x", i); \
> + } \
> + check(!class(EOF)); \
> } \
> - check(!func(EOF)); \
> -}
> -
> -#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
> + test__run_end(!skip, TEST_LOCATION(), #class " works"); \
> +} while (0)
>
> #define DIGIT "0123456789"
> #define LOWER "abcdefghijklmnopqrstuvwxyz"
> @@ -33,37 +33,21 @@ static void test_ctype_##func(void) { \
> "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> "\x7f"
>
> -TEST_CTYPE_FUNC(isdigit, DIGIT)
> -TEST_CTYPE_FUNC(isspace, " \n\r\t")
> -TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
> -TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
> -TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
> -TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
> -TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
> -TEST_CTYPE_FUNC(isascii, ASCII)
> -TEST_CTYPE_FUNC(islower, LOWER)
> -TEST_CTYPE_FUNC(isupper, UPPER)
> -TEST_CTYPE_FUNC(iscntrl, CNTRL)
> -TEST_CTYPE_FUNC(ispunct, PUNCT)
> -TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
> -TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
> -
> int cmd_main(int argc, const char **argv) {
> - /* Run all character type tests */
> - TEST_CHAR_CLASS(isspace);
> - TEST_CHAR_CLASS(isdigit);
> - TEST_CHAR_CLASS(isalpha);
> - TEST_CHAR_CLASS(isalnum);
> - TEST_CHAR_CLASS(is_glob_special);
> - TEST_CHAR_CLASS(is_regex_special);
> - TEST_CHAR_CLASS(is_pathspec_magic);
> - TEST_CHAR_CLASS(isascii);
> - TEST_CHAR_CLASS(islower);
> - TEST_CHAR_CLASS(isupper);
> - TEST_CHAR_CLASS(iscntrl);
> - TEST_CHAR_CLASS(ispunct);
> - TEST_CHAR_CLASS(isxdigit);
> - TEST_CHAR_CLASS(isprint);
> + TEST_CHAR_CLASS(isspace, " \n\r\t");
> + TEST_CHAR_CLASS(isdigit, DIGIT);
> + TEST_CHAR_CLASS(isalpha, LOWER UPPER);
> + TEST_CHAR_CLASS(isalnum, LOWER UPPER DIGIT);
> + TEST_CHAR_CLASS(is_glob_special, "*?[\\");
> + TEST_CHAR_CLASS(is_regex_special, "$()*+.?[\\^{|");
> + TEST_CHAR_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
> + TEST_CHAR_CLASS(isascii, ASCII);
> + TEST_CHAR_CLASS(islower, LOWER);
> + TEST_CHAR_CLASS(isupper, UPPER);
> + TEST_CHAR_CLASS(iscntrl, CNTRL);
> + TEST_CHAR_CLASS(ispunct, PUNCT);
> + TEST_CHAR_CLASS(isxdigit, DIGIT "abcdefABCDEF");
> + TEST_CHAR_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
>
> return test_done();
> }
> --
> 2.44.0
>
^ permalink raw reply
* [PATCH v4 0/1] Change commit message
From: shejialuo @ 2024-03-04 9:54 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Patrick Steinhardt, shejialuo
In-Reply-To: <20240301130334.135773-1-shejialuo@gmail.com>
This version changes the last version's error message.
shejialuo (1):
t9117: prefer test_path_* helper functions
t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
2.44.0
^ permalink raw reply
* [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-04 9:54 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, Patrick Steinhardt, shejialuo
In-Reply-To: <20240304095436.56399-1-shejialuo@gmail.com>
test -(e|d) does not provide a nice error message when we hit test
failures, so use test_path_exists, test_path_is_dir instead.
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 62de819a44..3b038c338f 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
test_expect_success 'basic clone' '
test ! -d trunk &&
git svn clone "$svnrepo"/project/trunk &&
- test -d trunk/.git/svn &&
- test -e trunk/foo &&
+ test_path_is_dir trunk/.git/svn &&
+ test_path_exists trunk/foo &&
rm -rf trunk
'
test_expect_success 'clone to target directory' '
test ! -d target &&
git svn clone "$svnrepo"/project/trunk target &&
- test -d target/.git/svn &&
- test -e target/foo &&
+ test_path_is_dir target/.git/svn &&
+ test_path_exists target/foo &&
rm -rf target
'
test_expect_success 'clone with --stdlayout' '
test ! -d project &&
git svn clone -s "$svnrepo"/project &&
- test -d project/.git/svn &&
- test -e project/foo &&
+ test_path_is_dir project/.git/svn &&
+ test_path_exists project/foo &&
rm -rf project
'
test_expect_success 'clone to target directory with --stdlayout' '
test ! -d target &&
git svn clone -s "$svnrepo"/project target &&
- test -d target/.git/svn &&
- test -e target/foo &&
+ test_path_is_dir target/.git/svn &&
+ test_path_exists target/foo &&
rm -rf target
'
--
2.44.0
^ permalink raw reply related
* Re: [PATCH 1/1] rebase: teach `--exec` about `GIT_REBASE_BRANCH`
From: Phillip Wood @ 2024-03-04 9:56 UTC (permalink / raw)
To: Junio C Hamano, Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <xmqqo7buuce7.fsf@gitster.g>
On 03/03/2024 23:24, Junio C Hamano wrote:
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>
> So, isn't it just the matter of surfacing the information that we
> are already recording and is already available in a fashion that is
> easier to use? For example, if "git status --porcelain=[version]"
> does not give the information, perhaps you can add a line or two to
> it, instead of duplicating the same information in two places?
That was my thought as well. I also don't think it is helpful to think
of a single branch being associated with a rebase these days. If we
update the output of "git stasus --porcelain" we should show all the
refs that are being rewritten by reading the contents of
rebase_path_update_refs() as well as the head-name file.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 9/9] upload-pack: free tree buffers after parsing
From: Jeff King @ 2024-03-04 9:57 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Benjamin Flesch
In-Reply-To: <ZeWHdaZnhOHKs5QP@tanuki>
On Mon, Mar 04, 2024 at 09:33:57AM +0100, Patrick Steinhardt wrote:
> > + if (skip_hash && discard_tree &&
> > + (!obj || obj->type == OBJ_TREE) &&
> > + oid_object_info(r, oid, NULL) == OBJ_TREE) {
> > + return &lookup_tree(r, oid)->object;
> > + }
>
> The other condition for blobs does the same, but the condition here
> confuses me. Why do we call `oid_object_info()` if we have already
> figured out that `obj->type == OBJ_TREE`? Feels like wasted effort if
> the in-memory object has been determined to be a tree already anyway.
>
> I'd rather have expected it to look like the following:
>
> if (skip_hash && discard_tree &&
> ((obj && obj->type == OBJ_TREE) ||
> (!obj && oid_object_info(r, oid, NULL)) == OBJ_TREE)) {
> return &lookup_tree(r, oid)->object;
> }
>
> Am I missing some side effect that `oid_object_info()` provides?
Calling oid_object_info() will make sure the on-disk object exists and
has the expected type. Keep in mind that an in-memory "struct object"
may have a type that was just implied by another reference. E.g., if a
commit references some object X in its tree field, then we'll call
lookup_tree(X) to get a "struct tree" without actually touching the odb
at all. When it comes time to parse that object, that's when we'll see
if we really have it and if it's a tree.
In the case of skip_hash (and discard_tree) it might be OK to skip both
of those checks. If we do, I think we should probably do the same for
blobs (in the skip_hash case, we could just return the object we found
already).
But I'd definitely prefer to do that as a separate step (if at all).
-Peff
^ permalink raw reply
* Re: [PATCH 6/9] upload-pack: disallow object-info capability by default
From: Jeff King @ 2024-03-04 9:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git, Benjamin Flesch
In-Reply-To: <ZeWHlknuWMvRiFtC@tanuki>
On Mon, Mar 04, 2024 at 09:34:30AM +0100, Patrick Steinhardt wrote:
> > +test_expect_success 'object-info missing from capabilities when disabled' '
> > + test_config transfer.advertiseObjectInfo false &&
> > +
> > + GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
> > + --advertise-capabilities >out &&
> > + test-tool pkt-line unpack <out >actual &&
> > +
> > + ! grep object.info actual
> > +'
>
> Is it intentional that you grep for "object.info" instead of
> "object-info"?
I didn't even notice this. It should be equivalent because of the regex,
but I don't think there's a particular reason to be more loose (and
unlike single-quote, which we sometimes match with "." for shell
readability, it should be fine to say "object-info" here).
+cc Taylor, who wrote the original.
-Peff
^ permalink raw reply
* Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions
From: Patrick Steinhardt @ 2024-03-04 9:59 UTC (permalink / raw)
To: shejialuo; +Cc: git, Eric Sunshine, Junio C Hamano
In-Reply-To: <20240304095436.56399-2-shejialuo@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]
On Mon, Mar 04, 2024 at 05:54:36PM +0800, shejialuo wrote:
> test -(e|d) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir instead.
>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
This version looks good to me, thanks!
One suggestion for potential future contributions by you: it's always
helpful to create a "range-diff" of what has changed between the
previous version of your patch series and the next one. Like this,
reviewers can immediately see what the difference is between the two
versions, which helps them to get the review done faster.
Assuming you use git-format-patch(1) you can generate such a range diff
with the `--range-diff=` parameter.
Patrick
> ---
> t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
> index 62de819a44..3b038c338f 100755
> --- a/t/t9117-git-svn-init-clone.sh
> +++ b/t/t9117-git-svn-init-clone.sh
> @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
> test_expect_success 'basic clone' '
> test ! -d trunk &&
> git svn clone "$svnrepo"/project/trunk &&
> - test -d trunk/.git/svn &&
> - test -e trunk/foo &&
> + test_path_is_dir trunk/.git/svn &&
> + test_path_exists trunk/foo &&
> rm -rf trunk
> '
>
> test_expect_success 'clone to target directory' '
> test ! -d target &&
> git svn clone "$svnrepo"/project/trunk target &&
> - test -d target/.git/svn &&
> - test -e target/foo &&
> + test_path_is_dir target/.git/svn &&
> + test_path_exists target/foo &&
> rm -rf target
> '
>
> test_expect_success 'clone with --stdlayout' '
> test ! -d project &&
> git svn clone -s "$svnrepo"/project &&
> - test -d project/.git/svn &&
> - test -e project/foo &&
> + test_path_is_dir project/.git/svn &&
> + test_path_exists project/foo &&
> rm -rf project
> '
>
> test_expect_success 'clone to target directory with --stdlayout' '
> test ! -d target &&
> git svn clone -s "$svnrepo"/project target &&
> - test -d target/.git/svn &&
> - test -e target/foo &&
> + test_path_is_dir target/.git/svn &&
> + test_path_exists target/foo &&
> rm -rf target
> '
>
> --
> 2.44.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 9/9] upload-pack: free tree buffers after parsing
From: Patrick Steinhardt @ 2024-03-04 10:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, Benjamin Flesch
In-Reply-To: <20240304095736.GA3723539@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]
On Mon, Mar 04, 2024 at 04:57:36AM -0500, Jeff King wrote:
> On Mon, Mar 04, 2024 at 09:33:57AM +0100, Patrick Steinhardt wrote:
>
> > > + if (skip_hash && discard_tree &&
> > > + (!obj || obj->type == OBJ_TREE) &&
> > > + oid_object_info(r, oid, NULL) == OBJ_TREE) {
> > > + return &lookup_tree(r, oid)->object;
> > > + }
> >
> > The other condition for blobs does the same, but the condition here
> > confuses me. Why do we call `oid_object_info()` if we have already
> > figured out that `obj->type == OBJ_TREE`? Feels like wasted effort if
> > the in-memory object has been determined to be a tree already anyway.
> >
> > I'd rather have expected it to look like the following:
> >
> > if (skip_hash && discard_tree &&
> > ((obj && obj->type == OBJ_TREE) ||
> > (!obj && oid_object_info(r, oid, NULL)) == OBJ_TREE)) {
> > return &lookup_tree(r, oid)->object;
> > }
> >
> > Am I missing some side effect that `oid_object_info()` provides?
>
> Calling oid_object_info() will make sure the on-disk object exists and
> has the expected type. Keep in mind that an in-memory "struct object"
> may have a type that was just implied by another reference. E.g., if a
> commit references some object X in its tree field, then we'll call
> lookup_tree(X) to get a "struct tree" without actually touching the odb
> at all. When it comes time to parse that object, that's when we'll see
> if we really have it and if it's a tree.
>
> In the case of skip_hash (and discard_tree) it might be OK to skip both
> of those checks. If we do, I think we should probably do the same for
> blobs (in the skip_hash case, we could just return the object we found
> already).
>
> But I'd definitely prefer to do that as a separate step (if at all).
Thanks for the explanation!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: Christian Couder @ 2024-03-04 10:00 UTC (permalink / raw)
To: René Scharfe; +Cc: Josh Steadmon, git, Phillip Wood, Achu Luma
In-Reply-To: <bd48f19b-0600-4e64-835b-98d3a97bb7f2@web.de>
On Sat, Mar 2, 2024 at 11:00 PM René Scharfe <l.s.r@web.de> wrote:
>
> Am 27.02.24 um 11:04 schrieb Christian Couder:
> > Yeah, I know about TAP harnesses like prove, but the most
> > straightforward way to run the unit tests is still `make unit-tests`
> > in the t/ directory. Also when you add or change some tests, it's a
> > good idea to run `make unit-tests` to see what the output is, so you
> > still have to see that output quite often when you work on tests and
> > going through 3598 of mostly useless output instead of just 14 isn't
> > nice.
>
> I was starting the programs from t/unit-tests/bin/ individually because
> I didn't know 'make unit-tests' exists. This is much nicer, thank you!
> Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to
> config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long
> ago. It would be even nicer if the former was the default when the
> latter is set.
>
> As unit tests are added, their output is surely going to grow to
> multiple screens with or without prove, no? So someone writing or
> debugging tests will still go back to starting then individually
> eventually.
When t-ctype will be run individually from t/unit-tests/bin/, for
example when adding or debugging ctype tests, it would still be better
if there are only 14 lines in its output rather than 3598.
> The size of the output in itself is not a problem, I assume, but that
> most of it is useless -- details of successful tests are uninteresting.
> A test harness can aggregate the output, but prove annoyed me when used
> with the regular tests by also aggregating error output and only showing
> the numbers of failed tests. Finding their names involved running the
> test script again without prove. Turns out it has an option for that.
> Added 'GIT_PROVE_OPTS = --failures' to config.mak as well, will see if
> it helps.
>
> Is it too much to ask developers to use a test harness? Perhaps: It's
> yet another dependency and not enabled by default.
Yeah, it's a dependency, and when running CI tests, it's sometimes
better and simpler to have the canonical output rather than having the
output processed by a test harness.
Also if we add some verbose or immediate modes (like -v and -i with
the shell test framework) or perhaps other kinds of modes (checking
for memory leaks or other errors using existing tools for example),
these modes might not interact nicely with test harnesses but still be
useful. Requiring to always use a test harness might restrict us for
no good reason.
And anyway it doesn't make sense to have meaningful messages as second
arguments to the TEST() macros if we always want to use a test harness
that just discards them. Either:
- we decide that we will always use some test harness, and then we
might just want to remove that second argument and yeah we can have
thousands of tests output lines from a single binary, or
- we acknowledge that we don't always want to use a test harness,
and then we want a relatively short and meaningful output from a
single binary.
> What's the right level of aggregation and how do we achieve it?
> Grouping by class is natural and follows the test definition. We
> could stop after patch 2. Dunno.
I am Ok with just removing this patch like you did in v2. Thanks.
^ permalink raw reply
* Re: [PATCH 0/4] some v2 capability advertisement cleanups
From: Jeff King @ 2024-03-04 10:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZeV7-NkUwT53IvIa@tanuki>
On Mon, Mar 04, 2024 at 08:44:56AM +0100, Patrick Steinhardt wrote:
> On Wed, Feb 28, 2024 at 05:46:25PM -0500, Jeff King wrote:
> > While working on another series, I noticed that upload-pack will accept
> > the "packfile-uris" directive even if we didn't advertise it. That's not
> > a huge deal in practice, but the spec says we're not supposed to. And
> > while cleaning that up, I noticed a bit of duplication in the existing
> > advertisement/allow code.
> >
> > So patches 1-3 clean up the situation a bit, and then patch 4 tightens
> > up the packfile-uris handling.
>
> This patch series was really easy to follow and feels like the right
> thing to do. I've got a couple of nits, but none of them are important
> enough to warrant a reroll.
Thanks for reviewing. The typos you found are definitely all wrong, but
I think the topic is in 'next' already (and they were all just in the
commit messages).
-Peff
^ permalink raw reply
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Phillip Wood @ 2024-03-04 10:12 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget, git; +Cc: Philippe Blain
In-Reply-To: <pull.1682.git.1709396291693.gitgitgadget@gmail.com>
Hi Philippe
On 02/03/2024 16:18, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Allow disabling the advice shown when a squencer operation results in a
> merge conflict through a new config 'advice.sequencerConflict'.
We already have "advice.resolveConflict" to suppress conflict advice.
Can we extend that to these conflict messages rather than introducing a
new category? As far as the user is concerned they are all messages
about resolving conflicts - I don't really see why they'd want to
suppress the messages from "git merge" separately to "git rebase" (and
if they do then why is it ok to suppress the messages from "git merge",
"git rebase" and "git cherry-pick" with a single setting). It would also
be good to update the "rebase --apply" implementation to respect this
advice config to be consistent with "rebase --merge".
Best Wishes
Phillip
> Update the tests accordingly. Note that the body of the second test in
> t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must
> escape them in the added line.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> sequencer: allow disabling conflict advice
>
> CC: Elijah Newren newren@gmail.com CC: Phillip Wood
> phillip.wood@dunelm.org.uk CC: Johannes Schindelin
> Johannes.Schindelin@gmx.de CC: ZheNing Hu adlternative@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1682
>
> Documentation/config/advice.txt | 3 +++
> advice.c | 1 +
> advice.h | 1 +
> sequencer.c | 33 ++++++++++++++++++---------------
> t/t3501-revert-cherry-pick.sh | 1 +
> t/t3507-cherry-pick-conflict.sh | 2 ++
> 6 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index c7ea70f2e2e..736b88407a4 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -104,6 +104,9 @@ advice.*::
> rmHints::
> In case of failure in the output of linkgit:git-rm[1],
> show directions on how to proceed from the current state.
> + sequencerConflict::
> + Advice shown when a sequencer operation stops because
> + of conflicts.
> sequencerInUse::
> Advice shown when a sequencer command is already in progress.
> skippedCherryPicks::
> diff --git a/advice.c b/advice.c
> index 6e9098ff089..23e48194e74 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -71,6 +71,7 @@ static struct {
> [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
> [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
> [ADVICE_RM_HINTS] = { "rmHints" },
> + [ADVICE_SEQUENCER_CONFLICT] = { "sequencerConflict" },
> [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" },
> [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
> [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
> diff --git a/advice.h b/advice.h
> index 9d4f49ae38b..98966f8991d 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -40,6 +40,7 @@ enum advice_type {
> ADVICE_RESOLVE_CONFLICT,
> ADVICE_RM_HINTS,
> ADVICE_SEQUENCER_IN_USE,
> + ADVICE_SEQUENCER_CONFLICT,
> ADVICE_SET_UPSTREAM_FAILURE,
> ADVICE_SKIPPED_CHERRY_PICKS,
> ADVICE_STATUS_AHEAD_BEHIND_WARNING,
> diff --git a/sequencer.c b/sequencer.c
> index f49a871ac06..3e2f028ce2d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -467,7 +467,7 @@ static void print_advice(struct repository *r, int show_hint,
> char *msg = getenv("GIT_CHERRY_PICK_HELP");
>
> if (msg) {
> - advise("%s\n", msg);
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT, "%s\n", msg);
> /*
> * A conflict has occurred but the porcelain
> * (typically rebase --interactive) wants to take care
> @@ -480,22 +480,25 @@ static void print_advice(struct repository *r, int show_hint,
>
> if (show_hint) {
> if (opts->no_commit)
> - advise(_("after resolving the conflicts, mark the corrected paths\n"
> - "with 'git add <paths>' or 'git rm <paths>'"));
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
> + _("after resolving the conflicts, mark the corrected paths\n"
> + "with 'git add <paths>' or 'git rm <paths>'"));
> else if (opts->action == REPLAY_PICK)
> - advise(_("After resolving the conflicts, mark them with\n"
> - "\"git add/rm <pathspec>\", then run\n"
> - "\"git cherry-pick --continue\".\n"
> - "You can instead skip this commit with \"git cherry-pick --skip\".\n"
> - "To abort and get back to the state before \"git cherry-pick\",\n"
> - "run \"git cherry-pick --abort\"."));
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
> + _("After resolving the conflicts, mark them with\n"
> + "\"git add/rm <pathspec>\", then run\n"
> + "\"git cherry-pick --continue\".\n"
> + "You can instead skip this commit with \"git cherry-pick --skip\".\n"
> + "To abort and get back to the state before \"git cherry-pick\",\n"
> + "run \"git cherry-pick --abort\"."));
> else if (opts->action == REPLAY_REVERT)
> - advise(_("After resolving the conflicts, mark them with\n"
> - "\"git add/rm <pathspec>\", then run\n"
> - "\"git revert --continue\".\n"
> - "You can instead skip this commit with \"git revert --skip\".\n"
> - "To abort and get back to the state before \"git revert\",\n"
> - "run \"git revert --abort\"."));
> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
> + _("After resolving the conflicts, mark them with\n"
> + "\"git add/rm <pathspec>\", then run\n"
> + "\"git revert --continue\".\n"
> + "You can instead skip this commit with \"git revert --skip\".\n"
> + "To abort and get back to the state before \"git revert\",\n"
> + "run \"git revert --abort\"."));
> else
> BUG("unexpected pick action in print_advice()");
> }
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index aeab689a98d..bc7c878b236 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -170,6 +170,7 @@ test_expect_success 'advice from failed revert' '
> hint: You can instead skip this commit with "git revert --skip".
> hint: To abort and get back to the state before "git revert",
> hint: run "git revert --abort".
> + hint: Disable this message with "git config advice.sequencerConflict false"
> EOF
> test_commit --append --no-tag "double-add dream" dream dream &&
> test_must_fail git revert HEAD^ 2>actual &&
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index c88d597b126..a643893dcbd 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -60,6 +60,7 @@ test_expect_success 'advice from failed cherry-pick' '
> hint: You can instead skip this commit with "git cherry-pick --skip".
> hint: To abort and get back to the state before "git cherry-pick",
> hint: run "git cherry-pick --abort".
> + hint: Disable this message with "git config advice.sequencerConflict false"
> EOF
> test_must_fail git cherry-pick picked 2>actual &&
>
> @@ -74,6 +75,7 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
> error: could not apply \$picked... picked
> hint: after resolving the conflicts, mark the corrected paths
> hint: with 'git add <paths>' or 'git rm <paths>'
> + hint: Disable this message with \"git config advice.sequencerConflict false\"
> EOF
> test_must_fail git cherry-pick --no-commit picked 2>actual &&
>
>
> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
^ permalink raw reply
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Phillip Wood @ 2024-03-04 10:27 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget, git; +Cc: Philippe Blain
In-Reply-To: <3df4790a-7ee1-4c72-a3da-ba8a48d546b8@gmail.com>
> We already have "advice.resolveConflict" to suppress conflict advice.
Oh looking more closely that is doing something slightly different - it
suppresses advice about pre-existing conflicts in the index when
starting a merge etc. So we probably do need a new config variable but I
think it should have a generic name - not be sequencer specific so we
can extend its scope in the future to "git merge", "git am -3", "git
stash" etc.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 11/12] reftable/record: decode keys in place
From: Patrick Steinhardt @ 2024-03-04 10:39 UTC (permalink / raw)
To: James Liu; +Cc: git
In-Reply-To: <CZGA0UX31LAO.1QWOAGQ6BUKIS@jamesliu.io>
[-- Attachment #1: Type: text/plain, Size: 616 bytes --]
On Wed, Feb 28, 2024 at 11:13:49AM +1100, James Liu wrote:
> On Wed Feb 14, 2024 at 6:46 PM AEDT, Patrick Steinhardt wrote:
> > - strbuf_reset(key);
> > - strbuf_add(key, last_key.buf, prefix_len);
> > - strbuf_add(key, in.buf, suffix_len);
> > + strbuf_setlen(last_key, prefix_len);
> > + strbuf_add(last_key, in.buf, suffix_len);
> > string_view_consume(&in, suffix_len);
> >
> > return start_len - in.len;
>
> Since we're using `strbuf`, there's no need to worry about extra bytes
> for the null terminator here right?
Exactly, the `struct strbuf` interface handles this for us.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 09/12] reftable/record: reuse refname when decoding
From: Patrick Steinhardt @ 2024-03-04 10:39 UTC (permalink / raw)
To: James Liu; +Cc: git
In-Reply-To: <CZG9VJKP1EA8.432Y9U39737O@jamesliu.io>
[-- Attachment #1: Type: text/plain, Size: 1663 bytes --]
On Wed, Feb 28, 2024 at 11:06:52AM +1100, James Liu wrote:
> On Wed Feb 14, 2024 at 6:46 PM AEDT, Patrick Steinhardt wrote:
> > This refactoring is safe to do because all functions that assigning to
> > the refname will first call `release_reftable_record()`, which will zero
> > out the complete record after releasing memory.
>
> s/release_reftable_record/reftable_ref_record_release
>
> > Furthermore, with this change we now perform a mostly constant number of
> > allocations when iterating.
>
> That's awesome!
>
> > + SWAP(refname, r->refname);
> > + SWAP(refname_cap, r->refname_cap);
> > reftable_ref_record_release(r);
> > + SWAP(refname, r->refname);
> > + SWAP(refname_cap, r->refname_cap);
>
> What do you think about reversing the order of the `SWAP` arguments in
> the last two invocations? If my understanding is correct that we're
> preserving the `refname` and `refname_cap` fields so we can set them back
> into a freshly initialised `r`, reversing the args might make that intent
> a bit clearer.
Yeah, fair enough.
> Also, since we're unconditionally `memcpy`ing the key into `r->refname`
> below, can we avoid the `SWAP(refname, r->refname)` call altogether?
No, otherwise `reftable_ref_record_release()` would have already
released the underlying pointer of `r->refname` and the call to
`REFTABLE_ALLOC_GROW()` would always have to reallocate it.
Patrick
> > - assert(hash_size > 0);
> > -
> > - r->refname = reftable_malloc(key.len + 1);
> > + REFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap);
> > memcpy(r->refname, key.buf, key.len);
> > r->refname[key.len] = 0;
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Support with finding git objects
From: brian m. carlson @ 2024-03-04 10:34 UTC (permalink / raw)
To: Ayoub, Toufic; +Cc: git@vger.kernel.org
In-Reply-To: <AMXP122MB01810E9B1195953D9AD0C8C48E5E2@AMXP122MB0181.EURP122.PROD.OUTLOOK.COM>
[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]
On 2024-03-01 at 19:06:29, Ayoub, Toufic wrote:
> Hello everyone,
Hey,
> Hope this message finds you. I need your support regarding an issue my team is facing.
> We have a code in our SW which shall display the date of the git commit. There is a path in the code that if it is executed on a platform where git is not installed, that this commit date shall be found by looking at:
> 1. Reading from HEAD file the commit hash
> 2. Searching for this commit hash under .git/objects
> The problem we are facing is that sometimes, when the default master branch is updated with the newest commit, this commit will not be found locally, even after doing git pull and so on...
> The commit can be found with:
> 1. git log
> 2. git cat-file -t <commit hash> -> it returns "commit"
> 3. git reflog
>
> but the strange thing is that we can neither find it under .git/objects/ nor under .git/objects/pack (although that is the newest commit and shall not be packed).
I think you're maybe making some assumptions about when loose objects
are and are not created that Git doesn't guarantee. For example, if
you're doing a `git pull` or `git fetch`, it is entirely possible that
the objects downloaded remain in a pack. In fact, by default, Git will
leave large fetches as packs to avoid an explosion of loose objects.
Note that the name of a pack is based on the hash of the entire pack
contents, not based on the items it contains (although obviously those
influence the pack contents).
> Can you please provide support by:
> 1. telling me it that is an issue or is it expected?
I believe this is expected.
> 2. If that is expected, where shall we find this commit object to retrieve the commit message and date from it (without running git commands)?
Sorry, but you're going to need to use Git commands to do this. You
could also use libgit2, but that doesn't work with SHA-256 repositories
or certain other features.
> 3. Or recommending me some other solution
I assume that you're building this software on a machine that _does_
have Git, so you could generate a file during the build process that
includes the commit date, commit hash, and any other information you
want. This is easy to do for compiled languages, but it's also possible
to generate something like a version.rb file for Ruby or the like as
part of your build process.
In the interests of reproducible builds, you will of course want to use
the commit metadata and not the actual timestamp of building.
If that doesn't work for you, maybe you could tell us a little bit about
your goal with this process and what you're using it for, and we can
help you more then.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* [PATCH v3 00/13] reftable: improve ref iteration performance (pt.2)
From: Patrick Steinhardt @ 2024-03-04 10:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, James Liu
In-Reply-To: <cover.1707895758.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5332 bytes --]
Hi,
this is the third version of my patch series that aims to improve raw
ref iteration performance with the reftable backend. Changes compared to
v2:
- Reversed the order of the second set of `SWAP()` macro calls.
- Fixed typos in commit messages.
Thanks!
Patrick
Patrick Steinhardt (13):
reftable/pq: use `size_t` to track iterator index
reftable/merged: make `merged_iter` structure private
reftable/merged: advance subiter on subsequent iteration
reftable/merged: make subiters own their records
reftable/merged: remove unnecessary null check for subiters
reftable/merged: handle subiter cleanup on close only
reftable/merged: circumvent pqueue with single subiter
reftable/merged: avoid duplicate pqueue emptiness check
reftable/record: reuse refname when decoding
reftable/record: reuse refname when copying
reftable/record: decode keys in place
reftable: allow inlining of a few functions
refs/reftable: precompute prefix length
refs/reftable-backend.c | 6 +-
reftable/block.c | 25 +++----
reftable/block.h | 2 -
reftable/iter.c | 5 --
reftable/iter.h | 4 --
reftable/merged.c | 139 +++++++++++++++++++------------------
reftable/merged.h | 11 +--
reftable/pq.c | 18 +----
reftable/pq.h | 16 +++--
reftable/pq_test.c | 41 +++++------
reftable/record.c | 64 +++++++++--------
reftable/record.h | 21 ++++--
reftable/record_test.c | 3 +-
reftable/reftable-record.h | 1 +
14 files changed, 175 insertions(+), 181 deletions(-)
Range-diff against v2:
1: 292e5f8888 = 1: c998039333 reftable/pq: use `size_t` to track iterator index
2: 95e1ccafc4 = 2: cb144e28a1 reftable/merged: make `merged_iter` structure private
3: 0e327e5fe3 = 3: 1bf09661e5 reftable/merged: advance subiter on subsequent iteration
4: 494d74deff = 4: 9aa1733aef reftable/merged: make subiters own their records
5: 0adf34d08b = 5: b413006159 reftable/merged: remove unnecessary null check for subiters
6: 01152ce130 = 6: 0ab1be740e reftable/merged: handle subiter cleanup on close only
7: 370b6cfc6c = 7: 2199881d47 reftable/merged: circumvent pqueue with single subiter
8: 1e279f21e6 ! 8: 04435f515c reftable/merged: avoid duplicate pqueue emptiness check
@@ Commit message
down the stack in `merged_iter_next_entry()` though, which makes this
check redundant.
- Now if this check was there to accellerate the common case it might have
+ Now if this check was there to accelerate the common case it might have
made sense to keep it. But the iterator being exhausted is rather the
uncommon case because you can expect most reftable stacks to contain
more than two refs.
9: 15a8cbf678 ! 9: 92f83dd404 reftable/record: reuse refname when decoding
@@ Commit message
to the required number of bytes via `REFTABLE_ALLOC_GROW()`.
This refactoring is safe to do because all functions that assigning to
- the refname will first call `release_reftable_record()`, which will zero
- out the complete record after releasing memory.
+ the refname will first call `reftable_ref_record_release()`, which will
+ zero out the complete record after releasing memory.
This change results in a nice speedup when iterating over 1 million
refs:
@@ reftable/record.c: static int reftable_ref_record_decode(void *rec, struct strbu
+ SWAP(refname, r->refname);
+ SWAP(refname_cap, r->refname_cap);
reftable_ref_record_release(r);
-+ SWAP(refname, r->refname);
-+ SWAP(refname_cap, r->refname_cap);
++ SWAP(r->refname, refname);
++ SWAP(r->refname_cap, refname_cap);
- assert(hash_size > 0);
-
10: 35b1af2f06 ! 10: eb600f3bf3 reftable/record: reuse refname when copying
@@ reftable/record.c: static void reftable_ref_record_copy_from(void *rec, const vo
+ SWAP(refname, ref->refname);
+ SWAP(refname_cap, ref->refname_cap);
reftable_ref_record_release(ref);
-+ SWAP(refname, ref->refname);
-+ SWAP(refname_cap, ref->refname_cap);
++ SWAP(ref->refname, refname);
++ SWAP(ref->refname_cap, refname_cap);
+
if (src->refname) {
- ref->refname = xstrdup(src->refname);
11: d7151ef361 = 11: f7915f1df8 reftable/record: decode keys in place
12: 99b238a40d = 12: 527c15e5da reftable: allow inlining of a few functions
13: 627bd1f5f7 ! 13: de4a1e2239 refs/reftable: precompute prefix length
@@ refs/reftable-backend.c: static int reftable_ref_iterator_advance(struct ref_ite
}
@@ refs/reftable-backend.c: static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_
iter = xcalloc(1, sizeof(*iter));
- base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable, 1);
+ base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable);
iter->prefix = prefix;
+ iter->prefix_len = prefix ? strlen(prefix) : 0;
iter->base.oid = &iter->oid;
base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 01/13] reftable/pq: use `size_t` to track iterator index
From: Patrick Steinhardt @ 2024-03-04 10:48 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, James Liu
In-Reply-To: <cover.1709548907.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 862 bytes --]
The reftable priority queue is used by the merged iterator to yield
records from its sub-iterators in the expected order. Each entry has a
record corresponding to such a sub-iterator as well as an index that
indicates which sub-iterator the record belongs to. But while the
sub-iterators are tracked with a `size_t`, we store the index as an
`int` in the entry.
Fix this and use `size_t` consistently.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/pq.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/reftable/pq.h b/reftable/pq.h
index e85bac9b52..9e25a43a36 100644
--- a/reftable/pq.h
+++ b/reftable/pq.h
@@ -12,7 +12,7 @@ license that can be found in the LICENSE file or at
#include "record.h"
struct pq_entry {
- int index;
+ size_t index;
struct reftable_record rec;
};
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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