* Re: Bug report: Incorrect GIT_FLUSH behavior in 2.43.1 (regression and breaking)
From: Xiaoguang WANG @ 2024-02-12 17:18 UTC (permalink / raw)
To: git
In-Reply-To: <xmqq8r3p7glr.fsf@gitster.g>
On Tue, Feb 13, 2024 at 1:11 AM Junio C Hamano <gitster@pobox.com> wrote:
> > And by the way, only accepting GIT_FLUSH=true is quite breaking, it
> > drops the compatibility of GIT_FLUSH=1
>
> I do not think so. If the polarity is corrected, git_env_bool()
> would say "that's affirmative" when any one of the "1", "true",
> "yes", "on", etc. is given. If you have been passing "1", you
> should get the "always flush" behaviour.
Oh yes, you are right. There is a git_parse_int call in
git_parse_maybe_bool, so if the "flipping" could be fixed, then
everything should be fine.
^ permalink raw reply
* Re: [PATCH v2 7/7] reftable/reader: add comments to `table_iter_next()`
From: Junio C Hamano @ 2024-02-12 17:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Eric Sunshine, John Cai
In-Reply-To: <167f67fad841ad06535a5532088fa6c9125fb1cd.1707726654.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> While working on the optimizations in the preceding patches I stumbled
> upon `table_iter_next()` multiple times. It is quite easy to miss the
> fact that we don't call `table_iter_next_in_block()` twice, but that the
> second call is in fact `table_iter_next_block()`.
>
> Add comments to explain what exactly is going on here to make things
> more obvious. While at it, touch up the code to conform to our code
> style better.
>
> Note that one of the refactorings merges two conditional blocks into
> one. Before, we had the following code:
>
> ```
> err = table_iter_next_block(&next, ti
");"???
> if (err != 0) {
> ti->is_finished = 1;
> }
> table_iter_block_done(ti);
> if (err != 0) {
> return err;
> }
> ```
>
> As `table_iter_block_done()` does not care about `is_finished`, the
> conditional blocks can be merged into one block:
>
> ```
> err = table_iter_next_block(&next, ti
> table_iter_block_done(ti);
> if (err != 0) {
> ti->is_finished = 1;
> return err;
> }
> ```
>
> This is both easier to reason about and more performant because we have
> one branch less.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> reftable/reader.c | 26 +++++++++++++++++---------
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/reftable/reader.c b/reftable/reader.c
> index 64dc366fb1..add7d57f0b 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -357,24 +357,32 @@ static int table_iter_next(struct table_iter *ti, struct reftable_record *rec)
>
> while (1) {
> struct table_iter next = TABLE_ITER_INIT;
> - int err = 0;
> - if (ti->is_finished) {
> + int err;
> +
> + if (ti->is_finished)
> return 1;
> - }
>
> + /*
> + * Check whether the current block still has more records. If
> + * so, return it. If the iterator returns positive then the
> + * current block has been exhausted.
> + */
> err = table_iter_next_in_block(ti, rec);
> - if (err <= 0) {
> + if (err <= 0)
> return err;
> - }
>
> + /*
> + * Otherwise, we need to continue to the next block in the
> + * table and retry. If there are no more blocks then the
> + * iterator is drained.
> + */
> err = table_iter_next_block(&next, ti);
> - if (err != 0) {
> - ti->is_finished = 1;
> - }
> table_iter_block_done(ti);
> - if (err != 0) {
> + if (err) {
> + ti->is_finished = 1;
> return err;
> }
> +
> table_iter_copy_from(ti, &next);
> block_iter_close(&next.bi);
> }
^ permalink raw reply
* RE: [BUG] git 2.44.0-rc0 t0080.1 Breaks on NonStop x86 and ia64
From: rsbecker @ 2024-02-12 17:59 UTC (permalink / raw)
To: 'Junio C Hamano'; +Cc: git
In-Reply-To: <xmqqzfw57hw0.fsf@gitster.g>
On Monday, February 12, 2024 11:43 AM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>>>This looks like a good plan.
>>
>> This might be trivial, but I cannot tell. The #ifndef should be changed
as
>> follows:
>
>https://lore.kernel.org/git/xmqqttmf9y46.fsf@gitster.g/
I applied this fix but there is no improvement in the result from the last
report. actual just has two lines. expect looks reasonable. I still had to
modify the #ifndef.
I have tried cherry-picking the change (no effect), building on master,
next... am lost.
^ permalink raw reply
* Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`
From: Junio C Hamano @ 2024-02-12 18:05 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps, phillip.wood123, Jeff King
In-Reply-To: <20240211183923.131278-2-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> + if (ends_with(refname, "_HEAD")) {
> + refs_resolve_ref_unsafe(refs, refname,
> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + &oid, NULL);
> + return !is_null_oid(&oid);
> + }
FYI. I see
.git/rebase-apply/patch:31: space before tab in indent.
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
.git/rebase-apply/patch:32: space before tab in indent.
&oid, NULL);
.git/rebase-apply/patch:33: space before tab in indent.
return !is_null_oid(&oid);
around here.
^ permalink raw reply
* Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Junio C Hamano @ 2024-02-12 18:27 UTC (permalink / raw)
To: Philippe Blain, Johannes Sixt
Cc: Elijah Newren, Michael Lohmann, Phillip Wood, Patrick Steinhardt,
Michael Lohmann, git
In-Reply-To: <529c7b42-c606-408e-b6a3-fe189c28db9b@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 11.02.24 um 17:43 schrieb Philippe Blain:
>> Hi Johannes,
>>
>> Le 2024-02-11 à 03:34, Johannes Sixt a écrit :
>>
>>>> Adjust the documentation of this option accordingly.
>>>>
>>>> Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
>>>> Co-authored-by: Johannes Sixt <j6t@kdbg.org>
>>>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>>>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
>>>> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
>>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>>
>>> Signed-off-by trailers should occur in temporal order. Therefore, when
>>> you pick up a commit and resend it, you should keep existing
>>> Signed-off-by and add yours last.
>>
>> Thank you, I did not know that. I guess Junio should be kept last though ?
>> Or maybe I should remove Junio's sign-off if I send a new version of the
>> patch ?
>
> You should *not* remove Junio's Signed-off-by, because the patch went
> through his hands before you picked it up. Then you add your own
> sign-off below. Later, Junio will sign it off again.
In the meantime, this is how I tweaked while queuing.
Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
[jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
[pb: greatly enhanced the log message]
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [BUG] git 2.44.0-rc0 t0080.1 Breaks on NonStop x86 and ia64
From: Junio C Hamano @ 2024-02-12 19:02 UTC (permalink / raw)
To: rsbecker; +Cc: git
In-Reply-To: <010601da5ddd$3dec41a0$b9c4c4e0$@nexbridge.com>
<rsbecker@nexbridge.com> writes:
> On Monday, February 12, 2024 11:43 AM, Junio C Hamano wrote:
>><rsbecker@nexbridge.com> writes:
>>
>>>>This looks like a good plan.
>>>
>>> This might be trivial, but I cannot tell. The #ifndef should be changed
> as
>>> follows:
>>
>>https://lore.kernel.org/git/xmqqttmf9y46.fsf@gitster.g/
>
> I applied this fix but there is no improvement in the result from the last
> report. actual just has two lines. expect looks reasonable. I still had to
> modify the #ifndef.
>
> I have tried cherry-picking the change (no effect), building on master,
> next... am lost.
We seem to be looking at something totally different. The later
patch in question (not the "looks like a good plan" outline) does
not have any #ifndef and applies the make_relative() logic
everywhere.
I would suspect that cherry-picking f6628636 (unit-tests: do show
relative file paths on non-Windows, too, 2024-02-11) would be the
simplest.
--- >8 ---
Subject: [PATCH] unit-tests: do show relative file paths on non-Windows, too
There are compilers other than Visual C that want to show absolute
paths. Generalize the helper introduced by a2c5e294 (unit-tests: do
show relative file paths, 2023-09-25) so that it can also work with
a path that uses slash as the directory separator, and becomes
almost no-op once one-time preparation finds out that we are using a
compiler that already gives relative paths. Incidentally, this also
should do the right thing on Windows with a compiler that shows
relative paths but with backslash as the directory separator (if
such a thing exists and is used to build git).
Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/unit-tests/test-lib.c | 61 +++++++++++++++++++++++++++++++----------
1 file changed, 47 insertions(+), 14 deletions(-)
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 7bf9dfdb95..66d6980ffb 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -21,12 +21,11 @@ static struct {
.result = RESULT_NONE,
};
-#ifndef _MSC_VER
-#define make_relative(location) location
-#else
/*
* Visual C interpolates the absolute Windows path for `__FILE__`,
* but we want to see relative paths, as verified by t0080.
+ * There are other compilers that do the same, and are not for
+ * Windows.
*/
#include "dir.h"
@@ -34,32 +33,66 @@ static const char *make_relative(const char *location)
{
static char prefix[] = __FILE__, buf[PATH_MAX], *p;
static size_t prefix_len;
+ static int need_bs_to_fs = -1;
- if (!prefix_len) {
+ /* one-time preparation */
+ if (need_bs_to_fs < 0) {
size_t len = strlen(prefix);
- const char *needle = "\\t\\unit-tests\\test-lib.c";
+ char needle[] = "t\\unit-tests\\test-lib.c";
size_t needle_len = strlen(needle);
- if (len < needle_len || strcmp(needle, prefix + len - needle_len))
- die("unexpected suffix of '%s'", prefix);
+ if (len < needle_len)
+ die("unexpected prefix '%s'", prefix);
+
+ /*
+ * The path could be relative (t/unit-tests/test-lib.c)
+ * or full (/home/user/git/t/unit-tests/test-lib.c).
+ * Check the slash between "t" and "unit-tests".
+ */
+ prefix_len = len - needle_len;
+ if (prefix[prefix_len + 1] == '/') {
+ /* Oh, we're not Windows */
+ for (size_t i = 0; i < needle_len; i++)
+ if (needle[i] == '\\')
+ needle[i] = '/';
+ need_bs_to_fs = 0;
+ } else {
+ need_bs_to_fs = 1;
+ }
- /* let it end in a directory separator */
- prefix_len = len - needle_len + 1;
+ /*
+ * prefix_len == 0 if the compiler gives paths relative
+ * to the root of the working tree. Otherwise, we want
+ * to see that we did find the needle[] at a directory
+ * boundary. Again we rely on that needle[] begins with
+ * "t" followed by the directory separator.
+ */
+ if (fspathcmp(needle, prefix + prefix_len) ||
+ (prefix_len && prefix[prefix_len - 1] != needle[1]))
+ die("unexpected suffix of '%s'", prefix);
}
- /* Does it not start with the expected prefix? */
- if (fspathncmp(location, prefix, prefix_len))
+ /*
+ * Does it not start with the expected prefix?
+ * Return it as-is without making it worse.
+ */
+ if (prefix_len && fspathncmp(location, prefix, prefix_len))
return location;
- strlcpy(buf, location + prefix_len, sizeof(buf));
+ /*
+ * If we do not need to munge directory separator, we can return
+ * the substring at the tail of the location.
+ */
+ if (!need_bs_to_fs)
+ return location + prefix_len;
+
/* convert backslashes to forward slashes */
+ strlcpy(buf, location + prefix_len, sizeof(buf));
for (p = buf; *p; p++)
if (*p == '\\')
*p = '/';
-
return buf;
}
-#endif
static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
{
--
2.44.0-rc0
^ permalink raw reply related
* [PATCH v2] t9146: replace test -d/-e/-f with appropriate test_path_is_* function
From: Chandra Pratap via GitGitGadget @ 2024-02-12 19:17 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1661.git.1707663197543.gitgitgadget@gmail.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
The helper functions test_path_is_* provide better debugging
information than test -d/-e/-f.
Replace "if ! test -d then <error message>" with "test_path_exists"
and "test -d" with "test_path_is_dir" at places where we check for
existent directories.
Replace "test -f" with "test_path_is_file" at places where we check
for existent files.
Replace "test ! -e" with "test_path_is_missing" where we check for
non-existent directories.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
t9146: replace test -d/-f with appropriate test_path_is_* function
cc: Eric Sunshine sunshine@sunshineco.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1661
Range-diff vs v1:
1: 93fe9e9eef7 ! 1: 5734b9edd61 t9146: replace test -d/-f with appropriate test_path_is_* function
@@ Metadata
Author: Chandra Pratap <chandrapratap3519@gmail.com>
## Commit message ##
- t9146: replace test -d/-f with appropriate test_path_is_* function
+ t9146: replace test -d/-e/-f with appropriate test_path_is_* function
The helper functions test_path_is_* provide better debugging
information than test -d/-e/-f.
- Replace "! test -d" with "test_path_is_missing" at places where
- we check for non-existent directories.
+ Replace "if ! test -d then <error message>" with "test_path_exists"
+ and "test -d" with "test_path_is_dir" at places where we check for
+ existent directories.
- Replace "test -f" with "test_path_is_file" and "test -d" with
- "test_path_is_dir" at places where we expect files or directories
- to exist.
+ Replace "test -f" with "test_path_is_file" at places where we check
+ for existent files.
+ Replace "test ! -e" with "test_path_is_missing" where we check for
+ non-existent directories.
+
+ Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
## t/t9146-git-svn-empty-dirs.sh ##
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories exist' '
for i in a b c d d/e d/e/f "weird file name"
do
- if ! test -d "$i"
-+ if test_path_is_missing "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
+- then
+- echo >&2 "$i does not exist" &&
+- exit 1
+- fi
++ test_path_exists "$i" || exit 1
+ done
+ )
+ '
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'option automkdirs set to false' '
git svn fetch &&
for i in a b c d d/e d/e/f "weird file name"
do
- if test -d "$i"
-+ if test_path_is_dir "$i"
- then
- echo >&2 "$i exists" &&
- exit 1
+- then
+- echo >&2 "$i exists" &&
+- exit 1
+- fi
++ test_path_is_missing "$i" || exit 1
+ done
+ )
+ '
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'more emptiness' '
test_expect_success 'git svn rebase creates empty directory' '
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs recreates emp
for i in a b c d d/e d/e/f "weird file name" "! !"
do
- if ! test -d "$i"
-+ if test_path_is_missing "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
+- then
+- echo >&2 "$i does not exist" &&
+- exit 1
+- fi
++ test_path_exists "$i" || exit 1
+ done
+ )
+ '
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs -r works' '
git svn mkdirs -r7 &&
for i in a b c d d/e d/e/f "weird file name"
do
- if ! test -d "$i"
-+ if test_path_is_missing "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
- fi
+- then
+- echo >&2 "$i does not exist" &&
+- exit 1
+- fi
++ test_path_exists "$i" || exit 1
done &&
- if test -d "! !"
-+ if test_path_is_dir "! !"
- then
- echo >&2 "$i should not exist" &&
- exit 1
- fi &&
+- then
+- echo >&2 "$i should not exist" &&
+- exit 1
+- fi &&
++ test_path_is_missing "! !" || exit 1 &&
git svn mkdirs -r8 &&
- if ! test -d "! !"
-+ if test_path_is_missing "! !"
- then
- echo >&2 "$i not exist" &&
- exit 1
+- then
+- echo >&2 "$i not exist" &&
+- exit 1
+- fi
++ test_path_exists "! !" || exit 1
+ )
+ '
+
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories in trunk exist' '
cd trunk &&
for i in a "weird file name"
do
- if ! test -d "$i"
-+ if test_path_is_missing "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
+- then
+- echo >&2 "$i does not exist" &&
+- exit 1
+- fi
++ test_path_exists "$i" || exit 1
+ done
+ )
+ '
+@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'remove a top-level directory from svn' '
+
+ test_expect_success 'removed top-level directory does not exist' '
+ git svn clone "$svnrepo" removed &&
+- test ! -e removed/d
++ test_path_is_missing removed/d
+
+ '
+ unhandled=.git/svn/refs/remotes/git-svn/unhandled.log
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn gc-ed files work' '
- cd removed &&
- git svn gc &&
- : Compress::Zlib may not be available &&
-- if test -f "$unhandled".gz
-+ if test_path_is_file "$unhandled".gz
- then
svn_cmd mkdir -m gz "$svnrepo"/gz &&
git reset --hard $(git rev-list HEAD | tail -1) &&
git svn rebase &&
@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn gc-ed files work' '
for i in a b c "weird file name" gz "! !"
do
- if ! test -d "$i"
-+ if test_path_is_missing "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
+- then
+- echo >&2 "$i does not exist" &&
+- exit 1
+- fi
++ test_path_exists "$i" || exit 1
+ done
+ fi
+ )
t/t9146-git-svn-empty-dirs.sh | 56 ++++++++---------------------------
1 file changed, 12 insertions(+), 44 deletions(-)
diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh
index 09606f1b3cf..6bf94ad802c 100755
--- a/t/t9146-git-svn-empty-dirs.sh
+++ b/t/t9146-git-svn-empty-dirs.sh
@@ -20,11 +20,7 @@ test_expect_success 'empty directories exist' '
cd cloned &&
for i in a b c d d/e d/e/f "weird file name"
do
- if ! test -d "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
- fi
+ test_path_exists "$i" || exit 1
done
)
'
@@ -37,11 +33,7 @@ test_expect_success 'option automkdirs set to false' '
git svn fetch &&
for i in a b c d d/e d/e/f "weird file name"
do
- if test -d "$i"
- then
- echo >&2 "$i exists" &&
- exit 1
- fi
+ test_path_is_missing "$i" || exit 1
done
)
'
@@ -52,7 +44,7 @@ test_expect_success 'more emptiness' '
test_expect_success 'git svn rebase creates empty directory' '
( cd cloned && git svn rebase ) &&
- test -d cloned/"! !"
+ test_path_is_dir cloned/"! !"
'
test_expect_success 'git svn mkdirs recreates empty directories' '
@@ -62,11 +54,7 @@ test_expect_success 'git svn mkdirs recreates empty directories' '
git svn mkdirs &&
for i in a b c d d/e d/e/f "weird file name" "! !"
do
- if ! test -d "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
- fi
+ test_path_exists "$i" || exit 1
done
)
'
@@ -78,25 +66,13 @@ test_expect_success 'git svn mkdirs -r works' '
git svn mkdirs -r7 &&
for i in a b c d d/e d/e/f "weird file name"
do
- if ! test -d "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
- fi
+ test_path_exists "$i" || exit 1
done &&
- if test -d "! !"
- then
- echo >&2 "$i should not exist" &&
- exit 1
- fi &&
+ test_path_is_missing "! !" || exit 1 &&
git svn mkdirs -r8 &&
- if ! test -d "! !"
- then
- echo >&2 "$i not exist" &&
- exit 1
- fi
+ test_path_exists "! !" || exit 1
)
'
@@ -114,11 +90,7 @@ test_expect_success 'empty directories in trunk exist' '
cd trunk &&
for i in a "weird file name"
do
- if ! test -d "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
- fi
+ test_path_exists "$i" || exit 1
done
)
'
@@ -129,7 +101,7 @@ test_expect_success 'remove a top-level directory from svn' '
test_expect_success 'removed top-level directory does not exist' '
git svn clone "$svnrepo" removed &&
- test ! -e removed/d
+ test_path_is_missing removed/d
'
unhandled=.git/svn/refs/remotes/git-svn/unhandled.log
@@ -143,15 +115,11 @@ test_expect_success 'git svn gc-ed files work' '
svn_cmd mkdir -m gz "$svnrepo"/gz &&
git reset --hard $(git rev-list HEAD | tail -1) &&
git svn rebase &&
- test -f "$unhandled".gz &&
- test -f "$unhandled" &&
+ test_path_is_file "$unhandled".gz &&
+ test_path_is_file "$unhandled" &&
for i in a b c "weird file name" gz "! !"
do
- if ! test -d "$i"
- then
- echo >&2 "$i does not exist" &&
- exit 1
- fi
+ test_path_exists "$i" || exit 1
done
fi
)
base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821
--
gitgitgadget
^ permalink raw reply related
* RE: [BUG] git 2.44.0-rc0 t0080.1 Breaks on NonStop x86 and ia64
From: rsbecker @ 2024-02-12 19:26 UTC (permalink / raw)
To: 'Junio C Hamano'; +Cc: git
In-Reply-To: <xmqqil2t5wwk.fsf@gitster.g>
On Monday, February 12, 2024 2:02 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> On Monday, February 12, 2024 11:43 AM, Junio C Hamano wrote:
>>><rsbecker@nexbridge.com> writes:
>>>
>>>>>This looks like a good plan.
>>>>
>>>> This might be trivial, but I cannot tell. The #ifndef should be
>>>> changed
>> as
>>>> follows:
>>>
>>>https://lore.kernel.org/git/xmqqttmf9y46.fsf@gitster.g/
>>
>> I applied this fix but there is no improvement in the result from the
>> last report. actual just has two lines. expect looks reasonable. I
>> still had to modify the #ifndef.
>>
>> I have tried cherry-picking the change (no effect), building on
>> master, next... am lost.
>
>We seem to be looking at something totally different. The later patch in
question
>(not the "looks like a good plan" outline) does not have any #ifndef and
applies the
>make_relative() logic everywhere.
>
>I would suspect that cherry-picking f6628636 (unit-tests: do show relative
file
>paths on non-Windows, too, 2024-02-11) would be the simplest.
Would you mind pushing that commit to github, please? It is not in the
current history.
^ permalink raw reply
* Re: git-gui desktop launcher
From: Tobias Boesch @ 2024-02-12 20:23 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <993e6823-7fa7-4130-8c0a-69ed31da5fbe@kdbg.org>
On Tue, 2024-02-06 at 07:50 +0100, Johannes Sixt wrote:
> Am 05.02.24 um 21:12 schrieb Tobias Boesch:
> > Hello everyone,
> >
> > quoting from downstream issue:
> > https://gitlab.archlinux.org/archlinux/packaging/packages/git/-/issues/5
> >
> > -------------------------
> >
> > "As far as I can see git gui cannot easily be used by me on arch.
> > A .desktop entry is missing for me.
> > I created one that opens git gui.
> > It also adds an entry in the "Open With..." menu of file managers
> > (I
> > tested only with Nautilus). Opeing git gui with this entry git gui
> > is
> > opened in the folder where the menu was opened.
> > If it is a git repository git gui open it. If it is no git
> > repository
> > git gui opens just as if it was called from the desktop launcher.
> > Since it took a while to create it and adds value for me I would
> > like
> > to share it to be added to the git package by default.
> > It is far from being perfect. It's a first working version. For me
> > personally it is enough.
> > Before tweaking it further to fit the packaging standards I would
> > like
> > to ask if is desired to be added.
> >
> > .desktop file proposal
>
> Thank you, this is certainly helpful. To get a .desktop file
> accepted,
> you would have to submit it in patch form. Additionally, since there
> is
> a dependence on the install location, it must be included in the
> build
> process.
>
I plan to work on this. It might take some time since I cannot spend
much time on it.
> >
> > [Desktop Entry]
> > Name=git gui
>
> When I launch the program on my openSUSE desktop, the titlebar uses
> the
> name "Git Gui". IMO, that would make it more consistent.
>
"Git Gui" is fine for me. I thought I took the name from the official
docs at https://git-scm.com/docs/git-gui, but there is called "git-
gui".
> > Comment=A portable graphical interface to Git
>
> I have two gripes with this Comment:
>
> - That the program is portable is irrelevant for the user. The word
> need
> not occur in this Comment.
>
> - I had hoped for a more precise description. In particular, when a
> program is advertised as "graphical interface to Git", then I would
> expect that it can do a bit more than initialize repositories and
> make
> commits. At a minimum, I would expect a history viewer; but Git Gui
> doesn't have one. Unless you count the two "Visualize" entries in the
> "Repository" menu that invoke gitk as such. So, I dunno.
>
This is the one line desciption of the official documentation.
I would like to keep this description, to be consistent with the docs.
> > Exec=/bin/bash -c 'if [[ "$0" = "/bin/bash" ]]; then git gui; else
> > cd
> > "$0" && git gui; fi' %F
> > Icon=/usr/share/git-gui/lib/git-gui.ico
> > Type=Application
> > Terminal=false
> > Categories=Development;
> >
> >
> > I think upstream has any interest to add this. Therefore I ask
> > here."
> >
> > -------------------------
> >
> > The arch package maintainer proposed to try to to add this to
> > upstream
> > before just putting it into the arch package.
> > Here I am asking if it could be added to git.
> >
> > If it's worth to add it, I would take the time to improve it if
> > there
> > are suggestions or comments on the current version.
> >
> > Best wishes and thanks for developing git.
> > Tobias
>
> -- Hannes
>
^ permalink raw reply
* Re: [PATCH v2] t9146: replace test -d/-e/-f with appropriate test_path_is_* function
From: Junio C Hamano @ 2024-02-12 20:31 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1661.v2.git.1707765433663.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> The helper functions test_path_is_* provide better debugging
> information than test -d/-e/-f.
Correct.
> Replace "if ! test -d then <error message>" with "test_path_exists"
> and "test -d" with "test_path_is_dir" at places where we check for
> existent directories.
The former could result in misconversion, if the intention of the
test was "we cannot have directory here; a regular file is OK", so
we have to be a bit more careful than mechanical conversion.
> Replace "test -f" with "test_path_is_file" at places where we check
> for existent files.
OK.
> Replace "test ! -e" with "test_path_is_missing" where we check for
> non-existent directories.
OK.
> for i in a b c d d/e d/e/f "weird file name"
> do
> - if ! test -d "$i"
> - then
> - echo >&2 "$i does not exist" &&
> - exit 1
> - fi
> + test_path_exists "$i" || exit 1
We were saying that we are OK if "$i" existed as a file (not a
directory), but now we complain regardless of what "$i" is. Is that
closer to what the test originally wanted to do? Just checking.
> done
> )
> '
> @@ -37,11 +33,7 @@ test_expect_success 'option automkdirs set to false' '
> git svn fetch &&
> for i in a b c d d/e d/e/f "weird file name"
> do
> - if test -d "$i"
> - then
> - echo >&2 "$i exists" &&
> - exit 1
> - fi
> + test_path_is_missing "$i" || exit 1
Ditto; are we sure the intention of the original is that nothing
should be at "$i" (instead of "as long as it is not a directory,
we are OK")? Just checking.
The same comment applies to all conversions to test_path_exists and
test_path_is_missing where the original was not "test -e" or "! test -e".
The other ones, like the change from "test -f" to "test_path_is_file",
looked all correct.
Thanks.
^ permalink raw reply
* Re: git-gui desktop launcher
From: Tobias Boesch @ 2024-02-12 20:35 UTC (permalink / raw)
To: Dragan Simic, Junio C Hamano; +Cc: brian m. carlson, git
In-Reply-To: <c6be276bfc3c219e1a0ca1619f56c165@manjaro.org>
On Tue, 2024-02-06 at 20:12 +0100, Dragan Simic wrote:
>
> AFAICT, Linux distributions provided their own version(s) of the
> .desktop file. Perhaps the version provided by Fedora [1] could be
> consulted, for example, to see what's already expected there, and
> to provide parity in the version supplied by us.
>
> [1]
> https://koji.fedoraproject.org/koji/fileinfo?rpmID=37302272&filename=git-gui.desktop
The file contents of the fedora desktop launcher is here:
/home/tobiasb/.local/share/applications/git gui.desktop
It differs
- in the program name. "Git GUI" instead of "Git Gui".
- It leaves out the "portable" in the description ;-).
- I has no option to be launched from a location in file browser (open
with...)
I will adopt mine to match their file as good as possible.
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Josh Steadmon @ 2024-02-12 20:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqsf246ll7.fsf@gitster.g>
On 2024.02.07 12:55, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > -UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
> > -UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
> > +UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
>
> Nice that we no longer need the special casing.
>
> > diff --git a/t/unit-tests/t-basic.c b/t/helper/test-example-tap.c
> > similarity index 95%
> > rename from t/unit-tests/t-basic.c
> > rename to t/helper/test-example-tap.c
> > index fda1ae59a6..21e4848e78 100644
> > --- a/t/unit-tests/t-basic.c
> > +++ b/t/helper/test-example-tap.c
> > @@ -1,4 +1,5 @@
> > -#include "test-lib.h"
> > +#include "t/unit-tests/test-lib.h"
> > +#include "test-tool.h"
>
> As the first thing both of these headers include is
> "git-compat-util.h", so the ordering should be safe either way,
> because everybody else in the directory seems to include
> "test-tool.h" before including headers that are specific to the
> subsystem it is testing, and "t/unit-tests/test-lib.h" in this case
> is the header that is specific to the unit-test subsystem being
> tested, it may raise fewer eyebrows if we swapped the order of the
> inclusion here.
>
> Other than that, looks good to me.
>
> Thanks.
Fixed in V3.
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Josh Steadmon @ 2024-02-12 20:53 UTC (permalink / raw)
To: Jeff King; +Cc: git, johannes.schindelin, phillip.wood, gitster
In-Reply-To: <20240207225802.GA538110@coredump.intra.peff.net>
On 2024.02.07 17:58, Jeff King wrote:
> On Fri, Feb 02, 2024 at 04:50:26PM -0800, Josh Steadmon wrote:
>
> > This has the additional benefit that test harnesses seeking to run all
> > unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
> > with no exceptions needed. This will be important in a later patch where
> > we add support for running the unit tests via a test-tool subcommand.
>
> Is this last paragraph still accurate? I think in this rebased version
> of the series, we'll continue to use $(UNIT_TESTS) derived from the
> source list rather than a glob in bin/.
Right, thanks for the catch. Removed in V3.
> > --- a/t/Makefile
> > +++ b/t/Makefile
> > @@ -44,8 +44,7 @@ TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
> > CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
> > CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
> > UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> > -UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
> > -UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
> > +UNIT_TESTS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
>
> This drops the intermediate UNIT_TEST_PROGRAMS, which makes sense. It
> was only used to keep the long lines a bit more readable. But it also
> drops the $(sort) call. Do we need to keep it?
>
> Certainly I'd think we want the contents of $(UNIT_TESTS) to be in a
> deterministic order. Does the $(wildcard) function already return things
> in sorted order? I can't find any mention in the documention. It seems
> to do so for me in a simple test, but aae5239be2 (t/Makefile: Use $(sort
> ...) explicitly where needed, 2011-09-04) argues otherwise.
I see this line in the docs [1]: "As with wildcard expansion in rules,
the results of the wildcard function are sorted". GNU Make has restored
the sorted behavior of $(wildcard) since 2018 [2]. I'll leave the sort
off for now, but if folks feel like we need to support older versions of
`make`, I'll add it back.
[1] https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html
[2] https://savannah.gnu.org/bugs/index.php?52076
^ permalink raw reply
* Re: [RFC PATCH v2 4/6] test-tool run-command testsuite: support unit tests
From: Josh Steadmon @ 2024-02-12 21:15 UTC (permalink / raw)
To: phillip.wood; +Cc: git, johannes.schindelin, peff, gitster
In-Reply-To: <4e2cbadd-1e0c-4526-a50f-9ba8600e7788@gmail.com>
On 2024.02.05 16:16, phillip.wood123@gmail.com wrote:
> Hi Josh
>
> On 03/02/2024 00:50, Josh Steadmon wrote:
> > Teach the testsuite runner in `test-tool run-command testsuite` how to
> > run unit tests: if TEST_SHELL_PATH is not set, assume that we're running
> > the programs directly from CWD, rather than defaulting to "sh" as an
> > interpreter.
>
> Judging from the last patch in this series it seems likely that we'll want
> to run unit tests and integration tests parallel. In which case it might be
> better to look at the filename extension to decide whether to sh as an
> interpreter so that we can avoid having to use a wrapper script. Then
>
> cd t
> helper/test-tool run-command testsuite 't[0-9]*.sh' 'unit-tests/bin/*'
>
> would run the integration tests via "sh" and the unit-tests directly. We'd
> need to figure out how to look for tests in both directories as well
> though...
At the moment, I'm not planning on trying to make unit tests and shell
tests run under the same test-tool process. If that is a valuable
change, hopefully the Windows / CI experts can use this series as a
starting point to make additional test-tool changes. However, I will
probably not spend any further time on this area.
^ permalink raw reply
* Re: git-gui desktop launcher
From: Dragan Simic @ 2024-02-12 21:17 UTC (permalink / raw)
To: Tobias Boesch; +Cc: Junio C Hamano, brian m. carlson, git
In-Reply-To: <eb122f205692ef4848e3adf792d67067bb52dd1c.camel@googlemail.com>
Hello Tobias,
On 2024-02-12 21:35, Tobias Boesch wrote:
> On Tue, 2024-02-06 at 20:12 +0100, Dragan Simic wrote:
>>
>> AFAICT, Linux distributions provided their own version(s) of the
>> .desktop file. Perhaps the version provided by Fedora [1] could be
>> consulted, for example, to see what's already expected there, and
>> to provide parity in the version supplied by us.
>>
>> [1]
>> https://koji.fedoraproject.org/koji/fileinfo?rpmID=37302272&filename=git-gui.desktop
>
> The file contents of the fedora desktop launcher is here:
> /home/tobiasb/.local/share/applications/git gui.desktop
>
> It differs
> - in the program name. "Git GUI" instead of "Git Gui".
To me, "GUI" looks much better than "Gui" in a window title.
> - It leaves out the "portable" in the description ;-).
Perhaps Fedora is right there, because it refers to a version
packaged for a specific architecture.
> - I has no option to be launched from a location in file browser (open
> with...)
Maybe it could be better to take that approach and add the
"Open with..." feature later, just to play it safe.
> I will adopt mine to match their file as good as possible.
Good luck and have fun! :)
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] t0080: turn t-basic unit test into a helper
From: Junio C Hamano @ 2024-02-12 21:27 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jeff King, git, johannes.schindelin, phillip.wood
In-Reply-To: <ZcqFOVuR0sxFDDUv@google.com>
Josh Steadmon <steadmon@google.com> writes:
> I see this line in the docs [1]: "As with wildcard expansion in rules,
> the results of the wildcard function are sorted". GNU Make has restored
> the sorted behavior of $(wildcard) since 2018 [2]. I'll leave the sort
> off for now, but if folks feel like we need to support older versions of
> `make`, I'll add it back.
>
> [1] https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html
> [2] https://savannah.gnu.org/bugs/index.php?52076
Thanks for digging. I thought I was certain that woldcard is sorted
and stable and was quite perplexed when I could not find the mention
in a version of doc I had handy ("""This is Edition 0.75, last
updated 19 January 2020, of 'The GNU Make Manual', for GNU 'make'
version 4.3.""").
^ permalink raw reply
* Re: [PATCH v2] column: disallow negative padding
From: Rubén Justo @ 2024-02-12 21:28 UTC (permalink / raw)
To: Kristoffer Haugsbakk, git; +Cc: Tiago Pascoal, Chris Torek, Junio C Hamano
In-Reply-To: <09b1ab48-b58f-458c-89f5-0c419d92f61a@app.fastmail.com>
On 12-feb-2024 17:50:54, Kristoffer Haugsbakk wrote:
> On Sun, Feb 11, 2024, at 23:47, Rubén Justo wrote:
> > While we're here, I wonder if silently ignoring a negative value in
> > .padding is the right thing to do.
> >
> > There are several callers of print_columns():
> >
> > builtin/branch.c: print_columns(&output, colopts, NULL);
> > builtin/clean.c: print_columns(&list, colopts, &copts);
> > builtin/clean.c: print_columns(menu_list, local_colopts, &copts);
> > builtin/column.c: print_columns(&list, colopts, &copts);
> > help.c: print_columns(&list, colopts, &copts);
> > wt-status.c: print_columns(&output, s->colopts, &copts);
> >
> > I haven't checked it thoroughly but it seems we don't need to add the
> > check we're adding to builtin/column.c, to any of the other callers.
> > However, it is possible that these or other new callers may need it in
> > the future. If so, we should consider doing something like:
> >
> > diff --git a/column.c b/column.c
> > index c723428bc7..4f870c725f 100644
> > --- a/column.c
> > +++ b/column.c
> > @@ -186,6 +186,9 @@ void print_columns(const struct string_list *list,
> > unsigned int colopts,
> > return;
> > assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
> >
> > + if (opts && (0 > opts->padding))
;-) (fixed)
> > + BUG("padding must be non-negative");
> > +
>
> Sure, I could add a `BUG` for `0 > opts->padding` in v3.
Thank you for considering it.
^ permalink raw reply
* Re: [RFC PATCH v2 2/6] test-tool run-command testsuite: get shell from env
From: Josh Steadmon @ 2024-02-12 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqle7w6lkt.fsf@gitster.g>
On 2024.02.07 12:55, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > When running tests through `test-tool run-command testsuite`, we
> > currently hardcode `sh` as the command interpreter. As discussed in [1],
> > this is incorrect, and we should be using the shell set in
> > TEST_SHELL_PATH instead.
> >
> > Add a shell_path field in struct testsuite so that we can pass this to
> > the task runner callback. If this is non-null, we'll use it as the
> > argv[0] of the subprocess. Otherwise, we'll just execute the test
> > program directly.
>
> That sounds nice in theory, but ...
>
> > When setting up the struct testsuite in testsuite(), use the value
> > of TEST_SHELL_PATH if it's set, otherwise default to `sh`.
>
> ... this done in the testsuite() function, doesn't suite.shell_path
> always gets some non-NULL value? Perhaps in a later step we will
> add a mechanism to allow suite.shell_path to be NULL when we know
> we are running an executable, or something?
>
> Leaving readers in a bit of suspense may, especially in a series
> that is short like this, be a good technique to entice them to keep
> reading, perhaps, but anyway, if that is what is intended, a simple
> "this feature is not used in this step, but we will take advantage
> of it soon in a later step" would be a good idea.
Reworded in V3.
^ permalink raw reply
* Re: [PATCH v5 1/2] add-patch: classify '@' as a synonym for 'HEAD'
From: Junio C Hamano @ 2024-02-12 21:45 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, phillip.wood123, sunshine, ps
In-Reply-To: <20240211202035.7196-4-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> add-patch.c | 8 -------
> builtin/checkout.c | 4 +++-
> builtin/reset.c | 4 +++-
> t/t2016-checkout-patch.sh | 46 +++++++++++++++++++++-----------------
> t/t2020-checkout-detach.sh | 12 ++++++++++
> t/t2071-restore-patch.sh | 18 +++++++++------
> t/t7105-reset-patch.sh | 15 ++++++++-----
> 7 files changed, 64 insertions(+), 43 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 79eda168eb..68f525b35c 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1729,14 +1729,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> if (mode == ADD_P_STASH)
> s.mode = &patch_mode_stash;
> else if (mode == ADD_P_RESET) {
> - /*
> - * NEEDSWORK: Instead of comparing to the literal "HEAD",
> - * compare the commit objects instead so that other ways of
> - * saying the same thing (such as "@") are also handled
> - * appropriately.
> - *
> - * This applies to the cases below too.
> - */
> if (!revision || !strcmp(revision, "HEAD"))
> s.mode = &patch_mode_reset_head;
> else
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a6e30931b5..067c251933 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1224,7 +1224,9 @@ static void setup_new_branch_info_and_source_tree(
> struct tree **source_tree = &opts->source_tree;
> struct object_id branch_rev;
>
> - new_branch_info->name = xstrdup(arg);
> + /* treat '@' as a shortcut for 'HEAD' */
> + new_branch_info->name = !strcmp(arg, "@") ? xstrdup("HEAD") :
> + xstrdup(arg);
> setup_branch_path(new_branch_info);
>
> if (!check_refname_format(new_branch_info->path, 0) &&
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 8390bfe4c4..f0bf29a478 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -281,7 +281,9 @@ static void parse_args(struct pathspec *pathspec,
> verify_filename(prefix, argv[0], 1);
> }
> }
> - *rev_ret = rev;
> +
> + /* treat '@' as a shortcut for 'HEAD' */
> + *rev_ret = !strcmp("@", rev) ? "HEAD" : rev;
>
> parse_pathspec(pathspec, 0,
> PATHSPEC_PREFER_FULL |
Nice. Things have become so much simpler, without having to touch
millions of strcmp() with "HEAD" everywhere in the code paths.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too
From: Junio C Hamano @ 2024-02-12 22:41 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Johannes Schindelin, Randall S. Becker
In-Reply-To: <c625239a-a847-475a-a228-9deb622c67bf@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>> There is a larger clean-up opportunity to drop the need for making a
>> copy, which probably is worth doing, so I folded the above into this
>> version.
>
> Ooh, that's nice. This version looks good, I found the code comments
> very helpful
Thanks.
Judging from https://github.com/git/git/actions/runs/7878254534/job/21496314393#step:5:142
I do not seen to have broken Windows with this change, so let's
fast-track and merge it down to 'master' before -rc1.
> Best Wishes
>
> Phillip
>
>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> There are compilers other than Visual C that want to show absolute
>> paths. Generalize the helper introduced by a2c5e294 (unit-tests: do
>> show relative file paths, 2023-09-25) so that it can also work with
>> a path that uses slash as the directory separator, and becomes
>> almost no-op once one-time preparation finds out that we are using a
>> compiler that already gives relative paths. Incidentally, this also
>> should do the right thing on Windows with a compiler that shows
>> relative paths but with backslash as the directory separator (if
>> such a thing exists and is used to build git).
>> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
>> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> * I found that the diff relative to the result of applying v1 was
>> easier to follow than the range-diff, so here it is.
>> diff --git c/t/unit-tests/test-lib.c w/t/unit-tests/test-lib.c
>> index 83c9eb8c59..66d6980ffb 100644
>> --- c/t/unit-tests/test-lib.c
>> +++ w/t/unit-tests/test-lib.c
>> @@ -64,34 +64,33 @@ static const char *make_relative(const char *location)
>> * prefix_len == 0 if the compiler gives paths relative
>> * to the root of the working tree. Otherwise, we want
>> * to see that we did find the needle[] at a directory
>> - * boundary.
>> + * boundary. Again we rely on that needle[] begins with
>> + * "t" followed by the directory separator.
>> */
>> if (fspathcmp(needle, prefix + prefix_len) ||
>> - (prefix_len &&
>> - prefix[prefix_len - 1] != '/' &&
>> - prefix[prefix_len - 1] != '\\'))
>> + (prefix_len && prefix[prefix_len - 1] != needle[1]))
>> die("unexpected suffix of '%s'", prefix);
>> -
>> }
>> /*
>> - * If our compiler gives relative paths and we do not need
>> - * to munge directory separator, we can return location as-is.
>> + * Does it not start with the expected prefix?
>> + * Return it as-is without making it worse.
>> */
>> - if (!prefix_len && !need_bs_to_fs)
>> + if (prefix_len && fspathncmp(location, prefix, prefix_len))
>> return location;
>> - /* Does it not start with the expected prefix? */
>> - if (fspathncmp(location, prefix, prefix_len))
>> - return location;
>> + /*
>> + * If we do not need to munge directory separator, we can return
>> + * the substring at the tail of the location.
>> + */
>> + if (!need_bs_to_fs)
>> + return location + prefix_len;
>> - strlcpy(buf, location + prefix_len, sizeof(buf));
>> /* convert backslashes to forward slashes */
>> - if (need_bs_to_fs) {
>> - for (p = buf; *p; p++)
>> - if (*p == '\\')
>> - *p = '/';
>> - }
>> + strlcpy(buf, location + prefix_len, sizeof(buf));
>> + for (p = buf; *p; p++)
>> + if (*p == '\\')
>> + *p = '/';
>> return buf;
>> }
>> t/unit-tests/test-lib.c | 61
>> +++++++++++++++++++++++++++++++----------
>> 1 file changed, 47 insertions(+), 14 deletions(-)
>> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
>> index 7bf9dfdb95..66d6980ffb 100644
>> --- a/t/unit-tests/test-lib.c
>> +++ b/t/unit-tests/test-lib.c
>> @@ -21,12 +21,11 @@ static struct {
>> .result = RESULT_NONE,
>> };
>> -#ifndef _MSC_VER
>> -#define make_relative(location) location
>> -#else
>> /*
>> * Visual C interpolates the absolute Windows path for `__FILE__`,
>> * but we want to see relative paths, as verified by t0080.
>> + * There are other compilers that do the same, and are not for
>> + * Windows.
>> */
>> #include "dir.h"
>> @@ -34,32 +33,66 @@ static const char *make_relative(const char
>> *location)
>> {
>> static char prefix[] = __FILE__, buf[PATH_MAX], *p;
>> static size_t prefix_len;
>> + static int need_bs_to_fs = -1;
>> - if (!prefix_len) {
>> + /* one-time preparation */
>> + if (need_bs_to_fs < 0) {
>> size_t len = strlen(prefix);
>> - const char *needle = "\\t\\unit-tests\\test-lib.c";
>> + char needle[] = "t\\unit-tests\\test-lib.c";
>> size_t needle_len = strlen(needle);
>> - if (len < needle_len || strcmp(needle, prefix + len -
>> needle_len))
>> - die("unexpected suffix of '%s'", prefix);
>> + if (len < needle_len)
>> + die("unexpected prefix '%s'", prefix);
>> +
>> + /*
>> + * The path could be relative (t/unit-tests/test-lib.c)
>> + * or full (/home/user/git/t/unit-tests/test-lib.c).
>> + * Check the slash between "t" and "unit-tests".
>> + */
>> + prefix_len = len - needle_len;
>> + if (prefix[prefix_len + 1] == '/') {
>> + /* Oh, we're not Windows */
>> + for (size_t i = 0; i < needle_len; i++)
>> + if (needle[i] == '\\')
>> + needle[i] = '/';
>> + need_bs_to_fs = 0;
>> + } else {
>> + need_bs_to_fs = 1;
>> + }
>> - /* let it end in a directory separator */
>> - prefix_len = len - needle_len + 1;
>> + /*
>> + * prefix_len == 0 if the compiler gives paths relative
>> + * to the root of the working tree. Otherwise, we want
>> + * to see that we did find the needle[] at a directory
>> + * boundary. Again we rely on that needle[] begins with
>> + * "t" followed by the directory separator.
>> + */
>> + if (fspathcmp(needle, prefix + prefix_len) ||
>> + (prefix_len && prefix[prefix_len - 1] != needle[1]))
>> + die("unexpected suffix of '%s'", prefix);
>> }
>> - /* Does it not start with the expected prefix? */
>> - if (fspathncmp(location, prefix, prefix_len))
>> + /*
>> + * Does it not start with the expected prefix?
>> + * Return it as-is without making it worse.
>> + */
>> + if (prefix_len && fspathncmp(location, prefix, prefix_len))
>> return location;
>> - strlcpy(buf, location + prefix_len, sizeof(buf));
>> + /*
>> + * If we do not need to munge directory separator, we can return
>> + * the substring at the tail of the location.
>> + */
>> + if (!need_bs_to_fs)
>> + return location + prefix_len;
>> +
>> /* convert backslashes to forward slashes */
>> + strlcpy(buf, location + prefix_len, sizeof(buf));
>> for (p = buf; *p; p++)
>> if (*p == '\\')
>> *p = '/';
>> -
>> return buf;
>> }
>> -#endif
>> static void msg_with_prefix(const char *prefix, const char
>> *format, va_list ap)
>> {
^ permalink raw reply
* Re: [RFC PATCH v2 3/6] test-tool run-command testsuite: remove hardcoded filter
From: Josh Steadmon @ 2024-02-12 22:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqeddo6lkk.fsf@gitster.g>
On 2024.02.07 12:55, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > `test-tool run-command testsuite` currently assumes that it will only be
> > running the shell test suite, and therefore filters out anything that
> > does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".
> >
> > Later in this series, we'll adapt `test-tool run-command testsuite` to
> > also support unit tests, which do not follow the same naming conventions
> > as the shell tests, so this hardcoded pattern is inconvenient.
>
> Makes sense to explain what future steps this prepares the codebase
> for like this.
>
> > Since `testsuite` also allows specifying patterns on the command-line,
> > let's just remove this pattern. As noted in [1], there are no longer any
> > uses of `testsuite` in our codebase, it should be OK to break backwards
> > compatibility in this case. We also add a new filter to avoid trying to
> > execute "." and "..", so that users who wish to execute every test in a
> > directory can do so without specifying a pattern.
>
> As we discussed in Peff's Makefile change that enumerates "which are
> the unit-test programs?" Generally, $(wildcard) and readdir() to
> slurp everything in a directory, including stuff that is an
> untracked cruft, is not an excellent idea.
>
> This is not an end-user facing program and we are in full control of
> its input (most notably, "which ones should we be running?"), I do
> not think it would be a huge issue, though.
Would you prefer if I remove the default behavior of "run everything in
the CWD" and require passing in at least one filename filter?
^ permalink raw reply
* Re: [RFC PATCH v2 0/6] test-tool: add unit test suite runner
From: Josh Steadmon @ 2024-02-12 22:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <xmqqjznlqt1k.fsf@gitster.g>
On 2024.02.03 10:52, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> > * We should determine whether it is confusing or otherwise harmful to
> > people's workflow to have the unit tests run in parallel with shell
> > tests when using prove as the default test target.
>
> I do not know much about "confusing" thing, but if the user
> allocates, say, 16 jobs to run tests in parallel, and one of them
> drives the "unit test suite runner" that wants to do its own
> parallelism, we'd easily end up busting the resource limit the
> end-user desires. It does not necessarily mean that we should limit
> the parallelism of "unit test suite runner" to 1 under prove, though.
The current `prove` helper script does not do any separate parallelism,
so we are fine in that case, and the new test-tool runner does not
currently support running both unit tests and shell tests in the same
process, so we should be OK in either case.
^ permalink raw reply
* Re: [RFC PATCH v2 3/6] test-tool run-command testsuite: remove hardcoded filter
From: Junio C Hamano @ 2024-02-12 22:51 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, johannes.schindelin, peff, phillip.wood
In-Reply-To: <ZcqgMgFx97DbH94y@google.com>
Josh Steadmon <steadmon@google.com> writes:
>> As we discussed in Peff's Makefile change that enumerates "which are
>> the unit-test programs?" Generally, $(wildcard) and readdir() to
>> slurp everything in a directory, including stuff that is an
>> untracked cruft, is not an excellent idea.
>>
>> This is not an end-user facing program and we are in full control of
>> its input (most notably, "which ones should we be running?"), I do
>> not think it would be a huge issue, though.
>
> Would you prefer if I remove the default behavior of "run everything in
> the CWD" and require passing in at least one filename filter?
No preference either way at all.
^ permalink raw reply
* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Johannes Schindelin @ 2024-02-12 23:16 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <CAOLa=ZQOALZRNqp7dDH0qDWoHwo6_3G8VgVuMbb3C20UdJ4C5A@mail.gmail.com>
Hi Karthik,
On Mon, 5 Feb 2024, Karthik Nayak wrote:
> Hello,
>
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > diff --git a/bisect.c b/bisect.c
> > index f1273c787d9..f75e50c3397 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,
> > const char *subject_start;
> > int subject_len;
> >
> > + if (!buf)
> > + die(_("unable to read %s"), oid_to_hex(&commit->object.oid));
> > +
>
> Nit: We know that `repo_read_object_file()` fails on corrupt objects, so
> this means that this is only happening when the object doesn't exist. I
> wonder if it makes more sense to replace "unable to read %s" which is a
> little ambiguous with something like "object %q doesn't exist".
I specifically copied this error message from existing code that already
deals with these errors, so as not to cause unnecessary translator
friction.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] Always check the return value of `repo_read_object_file()`
From: Johannes Schindelin @ 2024-02-12 23:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqqplx9fdyk.fsf@gitster.g>
Hi Junio,
On Tue, 6 Feb 2024, Junio C Hamano wrote:
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index c8e33f97755..982bcfc4b1d 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt,
> >
> > data = repo_read_object_file(the_repository, &ce->oid,
> > &type, &size);
> > + if (!data)
> > + die(_("unable to read tree %s"), oid_to_hex(&ce->oid));
> > init_tree_desc(&tree, data, size);
> >
> > hit |= grep_tree(opt, pathspec, &tree, &name, 0, 0);
>
> This caught my attention during today's integration cycle. Checking
> nullness for data certainly is an improvement, but shouldn't we be
> checking type as well to make sure it is a tree and not a random
> tree-ish or even blob?
That sounds right, but I am already stretching this patch beyond what I am
funded to work on, and therefore I need to leave it at the current state.
Ciao,
Johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox