* [PATCH 6/6] builtin/reflog: introduce subcommand to list reflogs
From: Patrick Steinhardt @ 2024-02-19 14:35 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1708353264.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6086 bytes --]
While the git-reflog(1) command has subcommands to show reflog entries
or check for reflog existence, it does not have any subcommands that
would allow the user to enumerate all existing reflogs. This makes it
quite hard to discover which reflogs a repository has. While this can
be worked around with the "files" backend by enumerating files in the
".git/logs" directory, users of the "reftable" backend don't enjoy such
a luxury.
Introduce a new subcommand `git reflog list` that lists all reflogs the
repository knows of to fill this gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-reflog.txt | 3 ++
builtin/reflog.c | 34 ++++++++++++++++++
t/t1410-reflog.sh | 69 ++++++++++++++++++++++++++++++++++++
3 files changed, 106 insertions(+)
diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index ec64cbff4c..a929c52982 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -10,6 +10,7 @@ SYNOPSIS
--------
[verse]
'git reflog' [show] [<log-options>] [<ref>]
+'git reflog list'
'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]
[--rewrite] [--updateref] [--stale-fix]
[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
@@ -39,6 +40,8 @@ actions, and in addition the `HEAD` reflog records branch switching.
`git reflog show` is an alias for `git log -g --abbrev-commit
--pretty=oneline`; see linkgit:git-log[1] for more information.
+The "list" subcommand lists all refs which have a corresponding reflog.
+
The "expire" subcommand prunes older reflog entries. Entries older
than `expire` time, or entries older than `expire-unreachable` time
and not reachable from the current tip, are removed from the reflog.
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3a0c4d4322..63cd4d8b29 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -7,11 +7,15 @@
#include "wildmatch.h"
#include "worktree.h"
#include "reflog.h"
+#include "refs.h"
#include "parse-options.h"
#define BUILTIN_REFLOG_SHOW_USAGE \
N_("git reflog [show] [<log-options>] [<ref>]")
+#define BUILTIN_REFLOG_LIST_USAGE \
+ N_("git reflog list")
+
#define BUILTIN_REFLOG_EXPIRE_USAGE \
N_("git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\n" \
" [--rewrite] [--updateref] [--stale-fix]\n" \
@@ -29,6 +33,11 @@ static const char *const reflog_show_usage[] = {
NULL,
};
+static const char *const reflog_list_usage[] = {
+ BUILTIN_REFLOG_LIST_USAGE,
+ NULL,
+};
+
static const char *const reflog_expire_usage[] = {
BUILTIN_REFLOG_EXPIRE_USAGE,
NULL
@@ -46,6 +55,7 @@ static const char *const reflog_exists_usage[] = {
static const char *const reflog_usage[] = {
BUILTIN_REFLOG_SHOW_USAGE,
+ BUILTIN_REFLOG_LIST_USAGE,
BUILTIN_REFLOG_EXPIRE_USAGE,
BUILTIN_REFLOG_DELETE_USAGE,
BUILTIN_REFLOG_EXISTS_USAGE,
@@ -238,6 +248,29 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)
return cmd_log_reflog(argc, argv, prefix);
}
+static int show_reflog(const char *refname, void *cb_data UNUSED)
+{
+ printf("%s\n", refname);
+ return 0;
+}
+
+static int cmd_reflog_list(int argc, const char **argv, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct ref_store *ref_store;
+
+ argc = parse_options(argc, argv, prefix, options, reflog_list_usage, 0);
+ if (argc)
+ return error(_("%s does not accept arguments: '%s'"),
+ "list", argv[0]);
+
+ ref_store = get_main_ref_store(the_repository);
+
+ return refs_for_each_reflog(ref_store, show_reflog, NULL);
+}
+
static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
{
struct cmd_reflog_expire_cb cmd = { 0 };
@@ -417,6 +450,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
OPT_SUBCOMMAND("show", &fn, cmd_reflog_show),
+ OPT_SUBCOMMAND("list", &fn, cmd_reflog_list),
OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index d2f5f42e67..6d8d5a253d 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '
test_must_be_empty err
'
+test_expect_success 'list reflogs' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git reflog list >actual &&
+ test_must_be_empty actual &&
+
+ test_commit A &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual &&
+
+ git branch b &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/b
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'reflog list returns error with additional args' '
+ cat >expect <<-EOF &&
+ error: list does not accept arguments: ${SQ}bogus${SQ}
+ EOF
+ test_must_fail git reflog list bogus 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success 'reflog for symref with unborn target can be listed' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ git symbolic-ref HEAD refs/heads/unborn &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'reflog with invalid object ID can be listed' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ test-tool ref-store main update-ref msg refs/heads/missing \
+ $(test_oid deadbeef) "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ refs/heads/missing
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH] completion: use awk for filtering the config entries
From: Junio C Hamano @ 2024-02-19 17:19 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Beat Bolli, git, Philippe Blain,
Ævar Arnfjörð Bjarmason
In-Reply-To: <aa3e36a1-52d3-0c15-b70b-83c6664757f5@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > Makes sense. I wonder if we can have some simple script sanity
>> > checker that catches things like this, e.g., catting a single file
>> > into pipe, grep appearing upstream of awk or sed, etc.
>>
>> Yes, there are quite a few cases of these in t/. I'm not sure if it's worth
>> the churn, though. At least it would make the tests faster on Windows...
>
> Thank you for caring about the speed on Windows!
Yup, that is why I asked ;-)
^ permalink raw reply
* Re: [GIT PULL] l10n updates for 2.44.0 round 3
From: Junio C Hamano @ 2024-02-19 17:21 UTC (permalink / raw)
To: Jiang Xin
Cc: Git l10n discussion group, Git List, Alexander Shopov,
Arkadii Yakovets, Bagas Sanjaya, Emir SARI, Jean-Noël Avila,
Johannes Schindelin, Jordi Mas, Peter Krefting, Ralf Thielow,
Teng Long, Yi-Jyun Pan
In-Reply-To: <20240219010141.5616-1-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Hi Junio,
>
> Please pull the following l10n updates for Git 2.44.0.
>
> The following changes since commit 3e0d3cd5c7def4808247caf168e17f2bbf47892b:
>
> Merge branch 'jx/dirstat-parseopt-help' (2024-02-15 15:14:48 -0800)
>
> are available in the Git repository at:
>
> git@github.com:git-l10n/git-po.git tags/l10n-2.44.0-rnd3
>
> for you to fetch changes up to 5fdd5b989cbe5096d44e89861a92b2dd47c279d9:
>
> l10n: zh_TW: Git 2.44 (2024-02-18 21:03:43 +0800)
Thanks.
^ permalink raw reply
* [GSOC][RFC PATCH 0/1] microproject: use test_path_is_* functions in test scripts
From: Vincenzo Mezzela @ 2024-02-19 17:22 UTC (permalink / raw)
To: git; +Cc: Vincenzo Mezzela
This patch is submitted as a microproject for the application to the GSOC.
Upon previous dicussion[1], this patch replace the test shell commands
with the helper functions as follows:
- 'test -f' --> 'test_path_is_file'
- 'test ! -f' --> 'test_path_is_missing'
In the context of this file, 'test ! -f' is meant to check if the file
has been correctly cleaned, thus its usage has been replaced with
'test_path_is_missing' instead of '! test_path_is_file'.
Submitting as RFC to ask whether there is room for further improvements:
Would you like to see something like
'''
test_path_is_missing file1 &&
test_path_is_file file2 &&
test_path_is_missing file3 &&
test_path_is_file file5
'''
changed into
'''
test_path_is_file file2 &&
test_path_is_file file5 &&
test_path_is_missing file3 &&
test_path_is_missing file1
'''
where all the test_path_is_file are grouped before and followed by all
the test_path_is_missing (or the other way around) to enhance
readability of the code?
Thanks,
Vincenzo
[1] https://lore.kernel.org/git/xmqqy1bo5k5h.fsf@gitster.g/
Vincenzo Mezzela (1):
t: t7301-clean-interactive: Use test_path_is_(missing|file)
t/t7301-clean-interactive.sh | 490 +++++++++++++++++------------------
1 file changed, 245 insertions(+), 245 deletions(-)
--
2.34.1
^ permalink raw reply
* [GSOC][RFC PATCH 1/1] t: t7301-clean-interactive: Use test_path_is_(missing|file)
From: Vincenzo Mezzela @ 2024-02-19 17:22 UTC (permalink / raw)
To: git; +Cc: Vincenzo Mezzela
In-Reply-To: <20240219172214.7644-1-vincenzo.mezzela@gmail.com>
Replace test -(f|e) with the appropriate helper functions provided by
test-lib-functions.sh
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
^ permalink raw reply related
* Re: [BUG] git commit --trailer --verbose puts trailer below scissors line
From: Philippe Blain @ 2024-02-19 17:41 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Arver, Git mailing list, Junio C Hamano
In-Reply-To: <20240217060434.GE539459@coredump.intra.peff.net>
Hi Peff,
Le 2024-02-17 à 01:04, Jeff King a écrit :
> On Fri, Feb 16, 2024 at 04:04:18PM -0500, Philippe Blain wrote:
>
>> Hello,
>>
>> I've just found a bug in the current master: running
>>
>> git commit --trailer="key: value" --verbose
>>
>> puts the trailer below the diff, and thus below the scissors
>> line (and so it is discarded).
>>
>> I checked that it works OK in 2.42.1 but not in 2.43.2 so it is not
>> a new regression in the 2.44 cycle, but I thought I'd write now in case
>> someone spots the culprit commit faster than me.
>>
>> I'll start a bisection now.
>
> Looks like it bisects to 97e9d0b78a (trailer: find the end of the log
> message, 2023-10-20). Here's a test that demonstrates it (signed-off-by:
> me in case anyone wants to incorporate it into a fix):
Yes, this is also what I found - not without fighting a bit with 'git bisect' though,
but I'll start a new thread for that.
So, it is indeed a regression in the 2.44 cycle.
>
> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> index b5bf7de7cd..d8e216613f 100755
> --- a/t/t7502-commit-porcelain.sh
> +++ b/t/t7502-commit-porcelain.sh
> @@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '
> test_cmp expected actual
> '
>
> +test_expect_success 'commit --trailer with --verbose' '
> + cat >msg <<-\EOF &&
> + subject
> +
> + body
> + EOF
> + GIT_EDITOR=: git commit --edit -F msg --allow-empty \
> + --trailer="my-trailer: value" --verbose &&
> + {
> + cat msg &&
> + echo &&
> + echo "my-trailer: value"
> + } >expected &&
> + git cat-file commit HEAD >commit.msg &&
> + sed -e "1,/^\$/d" commit.msg >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'multiple -m' '
>
> >negative &&
Thanks for the test case!
Philippe.
^ permalink raw reply
* Re: [BUG] git commit --trailer --verbose puts trailer below scissors line
From: Junio C Hamano @ 2024-02-19 18:42 UTC (permalink / raw)
To: Philippe Blain; +Cc: Jeff King, Linus Arver, Git mailing list
In-Reply-To: <ca6a73d3-58b4-e65c-4a8f-e6ddb3e86902@gmail.com>
Philippe Blain <levraiphilippeblain@gmail.com> writes:
> Hi Peff,
>> ...
>> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
>> index b5bf7de7cd..d8e216613f 100755
>> --- a/t/t7502-commit-porcelain.sh
>> +++ b/t/t7502-commit-porcelain.sh
>> @@ -485,6 +485,24 @@ test_expect_success 'commit --trailer not confused by --- separator' '
>> test_cmp expected actual
>> '
>>
>> +test_expect_success 'commit --trailer with --verbose' '
>> + cat >msg <<-\EOF &&
>> + subject
>> +
>> + body
>> + EOF
>> + GIT_EDITOR=: git commit --edit -F msg --allow-empty \
>> + --trailer="my-trailer: value" --verbose &&
>> + {
>> + cat msg &&
>> + echo &&
>> + echo "my-trailer: value"
>> + } >expected &&
>> + git cat-file commit HEAD >commit.msg &&
>> + sed -e "1,/^\$/d" commit.msg >actual &&
>> + test_cmp expected actual
>> +'
>> +
>> test_expect_success 'multiple -m' '
>>
>> >negative &&
>
> Thanks for the test case!
>
> Philippe.
Thanks, both, for finding a rather unfortunate regression. Perhaps
it is worth delaying 2.44 final by a week or so to include a fix (or
a revert if it comes to it).
^ permalink raw reply
* Re: [GSOC][RFC PATCH 0/1] microproject: use test_path_is_* functions in test scripts
From: Eric Sunshine @ 2024-02-19 19:36 UTC (permalink / raw)
To: Vincenzo Mezzela; +Cc: git
In-Reply-To: <20240219172214.7644-1-vincenzo.mezzela@gmail.com>
On Mon, Feb 19, 2024 at 12:22 PM Vincenzo Mezzela
<vincenzo.mezzela@gmail.com> wrote:
> Would you like to see something like
> '''
> test_path_is_missing file1 &&
> test_path_is_file file2 &&
> test_path_is_missing file3 &&
> test_path_is_file file5
> '''
> changed into
> '''
> test_path_is_file file2 &&
> test_path_is_file file5 &&
> test_path_is_missing file3 &&
> test_path_is_missing file1
> '''
> where all the test_path_is_file are grouped before and followed by all
> the test_path_is_missing (or the other way around) to enhance
> readability of the code?
Generally speaking, no, reviewers would not want to see such a change
because "enhanced readability" is often quite subjective. More
importantly, though, reviewers would especially not want this done in
the same patch which changes `test -blah` to `test_path_foo` because
it make it harder for the reviewer to associate and verify each `test
-blah` to `test_path_foo` replacement in the final result with the
source statement in the original file. It's much easier for a reviewer
to validate old against new when there is an obvious one-to-one
correspondence.
^ permalink raw reply
* Re: [PATCH] branch: rework the descriptions of rename and copy operations
From: Junio C Hamano @ 2024-02-19 19:49 UTC (permalink / raw)
To: Dragan Simic; +Cc: Rubén Justo, git
In-Reply-To: <be91f3ad9305366c1385c2da4881537e@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Regarding the branch copy and rename operations and their argument
> names, perhaps the following would be a good choice:
>
> --copy [<branch>] <destination>
> --move [<branch>] <destination>
>
> It would clearly reflect the nature of the performed operations, while
> still using "<branch>" consistently, this time to refer to the source
> branch. Using "<destination>" to select the destination name should
> be pretty much self-descriptive, if you agree.
Awful. While I was skimming the messages without reading the
Subject line (hence without realizing that this is about improving
the existing documentation and not adding new features), I thought
that these two are about moving branch to a remote repository that
is named with <destination>.
My advice would be to stick to <old> vs <new> that contrasts well.
Thanks.
^ permalink raw reply
* Re: [PATCH] branch: rework the descriptions of rename and copy operations
From: Dragan Simic @ 2024-02-19 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Rubén Justo, git
In-Reply-To: <xmqq8r3g8caz.fsf@gitster.g>
Hello Junio,
On 2024-02-19 20:49, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Regarding the branch copy and rename operations and their argument
>> names, perhaps the following would be a good choice:
>>
>> --copy [<branch>] <destination>
>> --move [<branch>] <destination>
>>
>> It would clearly reflect the nature of the performed operations, while
>> still using "<branch>" consistently, this time to refer to the source
>> branch. Using "<destination>" to select the destination name should
>> be pretty much self-descriptive, if you agree.
>
> Awful. While I was skimming the messages without reading the
> Subject line (hence without realizing that this is about improving
> the existing documentation and not adding new features), I thought
> that these two are about moving branch to a remote repository that
> is named with <destination>.
>
> My advice would be to stick to <old> vs <new> that contrasts well.
I appreciate the directness and honesty. How about using "<oldbranch>"
and "<newbranch>" instead, which, although more wordy, would be more
consistent with "<branch>" that's used in a number of other places?
Such consistency should make the users recognize the arguments better
at first glance.
^ permalink raw reply
* Re: [PATCH] libsecret: retrieve empty password
From: M Hickford @ 2024-02-19 20:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: M Hickford via GitGitGadget, git, M Hickford
In-Reply-To: <ZdLweyFLDtdMAq2x@tanuki>
> > + g_free(c->password);
> > + c->password = strdup("");
>
> Shouldn't we use `g_strdup()` here, like we do everywhere else in this
> credential helper?
You're right. I'll correct in patch v2.
^ permalink raw reply
* Re: Why does the includeif woks how it does?
From: brian m. carlson @ 2024-02-19 20:10 UTC (permalink / raw)
To: Dominik von Haller; +Cc: git@vger.kernel.org
In-Reply-To: <FR2P281MB1686B7258CFB60A0F33FE108BA522@FR2P281MB1686.DEUP281.PROD.OUTLOOK.COM>
[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]
On 2024-02-18 at 15:37:29, Dominik von Haller wrote:
> I have been playing around with the includeif from the .gitconfig. It did not work for me at first, but after some help, I did get it to work.
>
> If you are curious. My Problem and what else was discussed here: https://github.com/git-for-windows/git/issues/4823
>
> Anyway. So, I was trying to access the email property which was set through an includeif config. It did not work because I was in a non git directory. Yes, I do know that the property set in includeif is named gitdir, but it was not obvious to me that you need to be in a git tracked directory for it to work.
>
> I am trying to understand why it must be this way. Why does it not work in non git tracked directories?
The main reason it works this way is because the goal is to adjust
configuration based on the location of a repository. Thus, if I have
`~/checkouts/work/` with my work code and `~/checkouts/personal/` with
my personal code, I can set options that are appropriate in each case
(e.g., `user.email`, `user.signingkey`, `credential.helper`, etc.).
Also, except for reading and writing with `git config`, the
configuration is typically not used unless you're in a repository.
There are only a handful of Git commands that don't use a repository at
all, so usually setting configuration outside of a repository isn't very
useful.
Note that if it didn't require a repository, then it would have to work
on the current working directory. But, it should be noted, the gitdir
option specifically does not operate on the current working directory.
While it is customary to have one's working directory be inside the
repository, you can be elsewhere and use `git -C` to change into the
repository (internally, Git does indeed change the working directory,
but that's an implementation detail).
That's not to say a feature couldn't be added that operated based on the
current working directory (or some related constraint) instead, but no
such feature has been added.
--
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 v2] libsecret: retrieve empty password
From: M Hickford via GitGitGadget @ 2024-02-19 20:40 UTC (permalink / raw)
To: git; +Cc: M Hickford, M Hickford
In-Reply-To: <pull.1676.git.git.1708296694988.gitgitgadget@gmail.com>
From: M Hickford <mirth.hickford@gmail.com>
Since 0ce02e2f (credential/libsecret: store new attributes, 2023-06-16)
a test that stores empty username and password fails when
t0303-credential-external.sh is run with
GIT_TEST_CREDENTIAL_HELPER=libsecret.
Retrieve empty password carefully. This fixes test:
ok 14 - helper (libsecret) can store empty username
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
libsecret: retrieve empty password
cc: Patrick Steinhardt ps@pks.im
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1676%2Fhickford%2Flibsecret-empty-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1676/hickford/libsecret-empty-v2
Pull-Request: https://github.com/git/git/pull/1676
Range-diff vs v1:
1: 877bbfb72ed ! 1: 2cdcba20622 libsecret: retrieve empty password
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(
c->password = g_strdup(parts[0]);
+ } else {
+ g_free(c->password);
-+ c->password = strdup("");
++ c->password = g_strdup("");
}
for (int i = 1; i < g_strv_length(parts); i++) {
if (g_str_has_prefix(parts[i], "password_expiry_utc=")) {
contrib/credential/libsecret/git-credential-libsecret.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index 215a81d8bae..90034d0cf1e 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -164,6 +164,9 @@ static int keyring_get(struct credential *c)
if (g_strv_length(parts) >= 1) {
g_free(c->password);
c->password = g_strdup(parts[0]);
+ } else {
+ g_free(c->password);
+ c->password = g_strdup("");
}
for (int i = 1; i < g_strv_length(parts); i++) {
if (g_str_has_prefix(parts[i], "password_expiry_utc=")) {
base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
--
gitgitgadget
^ permalink raw reply related
* RE: Why does the includeif woks how it does?
From: rsbecker @ 2024-02-19 21:19 UTC (permalink / raw)
To: 'brian m. carlson', 'Dominik von Haller'; +Cc: git
In-Reply-To: <ZdO1oL73SF5ZKOJT@tapette.crustytoothpaste.net>
On Monday, February 19, 2024 3:10 PM, brian m. carlson wrote:
>On 2024-02-18 at 15:37:29, Dominik von Haller wrote:
>> I have been playing around with the includeif from the .gitconfig. It did not work
>for me at first, but after some help, I did get it to work.
>>
>> If you are curious. My Problem and what else was discussed here:
>> https://github.com/git-for-windows/git/issues/4823
>>
>> Anyway. So, I was trying to access the email property which was set through an
>includeif config. It did not work because I was in a non git directory. Yes, I do know
>that the property set in includeif is named gitdir, but it was not obvious to me that
>you need to be in a git tracked directory for it to work.
>>
>> I am trying to understand why it must be this way. Why does it not work in non git
>tracked directories?
>
>The main reason it works this way is because the goal is to adjust configuration
>based on the location of a repository. Thus, if I have `~/checkouts/work/` with my
>work code and `~/checkouts/personal/` with my personal code, I can set options
>that are appropriate in each case (e.g., `user.email`, `user.signingkey`,
>`credential.helper`, etc.).
>
>Also, except for reading and writing with `git config`, the configuration is typically
>not used unless you're in a repository.
>There are only a handful of Git commands that don't use a repository at all, so
>usually setting configuration outside of a repository isn't very useful.
>
>Note that if it didn't require a repository, then it would have to work on the current
>working directory. But, it should be noted, the gitdir option specifically does not
>operate on the current working directory.
>While it is customary to have one's working directory be inside the repository, you
>can be elsewhere and use `git -C` to change into the repository (internally, Git does
>indeed change the working directory, but that's an implementation detail).
>
>That's not to say a feature couldn't be added that operated based on the current
>working directory (or some related constraint) instead, but no such feature has
>been added.
I have considered contributing an "includewhere" option that would do that and differentiate from "includeif". I'm not sure it is required, and what would happen with symbolic links.
Just a thought.
--Randall
^ permalink raw reply
* Re: [PATCH v4 0/3] apply with core.filemode=false
From: Junio C Hamano @ 2024-02-19 21:24 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <fd8264c0-3080-c9d9-cac5-51115b9909a5@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > Chandra Pratap (1):
>> > apply: ignore working tree filemode when !core.filemode
>> >
>> > Junio C Hamano (2):
>> > apply: correctly reverse patch's pre- and post-image mode bits
>> > apply: code simplification
>> >
>> > apply.c | 16 +++++++++++++---
>> > t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
>> > 2 files changed, 40 insertions(+), 3 deletions(-)
>>
>> Anybody wants to offer a review on this? I actually am fairly
>> confortable with these without any additional review, but since I am
>> sweeping the "Needs review" topics in the What's cooking report, I
>> thought I would ask for this one, too.
>
> I just had a look over all three of the patches, and to me, they look good
> to go.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 3/9] trailer: prepare to expose functions as part of API
From: Christian Couder @ 2024-02-19 21:31 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Kristoffer Haugsbakk,
Linus Arver
In-Reply-To: <4372af244f02b71cc70f3a8e1b5591b3b9fec93a.1708124951.git.gitgitgadget@gmail.com>
On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> In the next patch, we will move "process_trailers" from trailer.c to
> builtin/interpret-trailers.c. That move will necessitate the growth of
> the trailer.h API, forcing us to expose some additional functions in
> trailer.h.
Nit: actually this patch renames process_trailers() to
interpret_trailers() so the function that will be moved will be
interpret_trailers().
Nit: this patch and the next one will become commits, so perhaps:
s/In the next patch/In a following commit/
> Rename relevant functions so that they include the term "trailer" in
> their name, so that clients of the API will be able to easily identify
> them by their "trailer" moniker, just like all the other functions
> already exposed by trailer.h.
Except that "process_trailers()" already contains "trailer" but will
still be renamed by this patch to "interpret_trailers()". So I think
it might be nice to explain a bit why renaming process_trailers() to
interpret_trailers() makes sense too.
Also I think the subject, "trailer: prepare to expose functions as
part of API" could be more explicit about what the patch is actually
doing, like perhaps "trailer: rename functions to use 'trailer'".
In general, when there is a patch called "prepare to do X", then we
might expect a following patch called something like "actually do X".
But there isn't any patch in the series named like "trailer: expose
functions as part of API".
> Take the opportunity to start putting trailer processing options (opts)
> as the first parameter. This will be the pattern going forward in this
> series.
It's interesting to know that this will be the pattern going forward
in the series, but that doesn't quite tell why it's a good idea to do
it.
So I think it might be nice to repeat an explanation similar to the
one you give in "trailer: start preparing for formatting unification"
for format_trailers_from_commit():
"Reorder parameters for format_trailers_from_commit() to prefer
const struct process_trailer_options *opts
as the first parameter, because these options are intimately tied to
formatting trailers."
And maybe also say that parameters like `FILE *outfile` should be last
because they are some kind of 'out' parameters.
> diff --git a/trailer.c b/trailer.c
> index f74915bd8cd..916175707d8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
> fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
> }
>
> -static void print_all(FILE *outfile, struct list_head *head,
> - const struct process_trailer_options *opts)
> +static void format_trailers(const struct process_trailer_options *opts,
> + struct list_head *trailers, FILE *outfile)
This also renames `struct list_head *head` to `struct list_head
*trailers`. I think it would be nice if the commit message could talk
a bit about these renames too.
^ permalink raw reply
* Re: [PATCH v5 5/9] trailer: start preparing for formatting unification
From: Christian Couder @ 2024-02-19 21:31 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Kristoffer Haugsbakk,
Linus Arver
In-Reply-To: <b2a0f7829a1c5f2822e9a896ffe3744587ff1298.1708124951.git.gitgitgadget@gmail.com>
On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> Currently there are two functions for formatting trailers in
> <trailer.h>:
>
> void format_trailers(const struct process_trailer_options *,
> struct list_head *trailers, FILE *outfile);
>
> void format_trailers_from_commit(struct strbuf *out, const char *msg,
> const struct process_trailer_options *opts);
>
> and although they are similar enough (even taking the same
> process_trailer_options struct pointer) they are used quite differently.
> One might intuitively think that format_trailers_from_commit() builds on
> top of format_trailers(), but this is not the case. Instead
> format_trailers_from_commit() calls format_trailer_info() and
> format_trailers() is never called in that codepath.
>
> This is a preparatory refactor to help us deprecate format_trailers() in
> favor of format_trailer_info() (at which point we can rename the latter
> to the former). When the deprecation is complete, both
> format_trailers_from_commit(), and the interpret-trailers builtin will
> be able to call into the same helper function (instead of
> format_trailers() and format_trailer_info(), respectively). Unifying the
> formatters is desirable because it simplifies the API.
>
> Reorder parameters for format_trailers_from_commit() to prefer
>
> const struct process_trailer_options *opts
>
> as the first parameter, because these options are intimately tied to
> formatting trailers. And take
>
> struct strbuf *out
>
> last, because it's an "out parameter" (something that the caller wants
> to use as the output of this function).
Here also I think the subject could be more specific like for example:
"trailer: reorder format_trailers_from_commit() parameters"
> diff --git a/trailer.c b/trailer.c
> index d23afa0a65c..5025be97899 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info)
> free(info->trailers);
> }
>
> -static void format_trailer_info(struct strbuf *out,
> +static void format_trailer_info(const struct process_trailer_options *opts,
> const struct trailer_info *info,
> const char *msg,
> - const struct process_trailer_options *opts)
> + struct strbuf *out)
Ok, so it's not just format_trailers_from_commit() parameters that are
reordered, but also format_trailer_info() parameters. It would be nice
if the commit message mentioned it.
^ permalink raw reply
* Re: [PATCH v5 9/9] format_trailers_from_commit(): indirectly call trailer_info_get()
From: Christian Couder @ 2024-02-19 21:32 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Kristoffer Haugsbakk,
Linus Arver
In-Reply-To: <7c656b3f77546ae917ff192031c62d4521d9df8c.1708124951.git.gitgitgadget@gmail.com>
On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> This is another preparatory refactor to unify the trailer formatters.
>
> Instead of calling trailer_info_get() directly, call parse_trailers()
> which already calls trailer_info_get(). This change is a NOP because
> format_trailer_info() only looks at the "trailers" string array, not the
> trailer_item objects which parse_trailers() populates.
Is the extra processing done by parse_trailers() compared to
trailer_info_get() impacting performance?
Also when looking only at the patch, it's a bit difficult to
understand that the "trailers" string array is the `char **trailers`
field in `struct trailer_info` and that the trailer_item objects are
the elements of the `struct list_head *head` linked list. It could
also be confusing because the patch is adding a new 'trailers'
variable with `LIST_HEAD(trailers);`. So a few more details could help
understand what's going on.
> In a future patch, we'll change format_trailer_info() to use the parsed
> trailer_item objects instead of the string array.
Ok, so I guess the possible performance issue would disappear then, as
populating the trailer_item objects will be useful.
^ permalink raw reply
* Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range
From: Junio C Hamano @ 2024-02-19 21:35 UTC (permalink / raw)
To: Philip Peterson via GitGitGadget; +Cc: git, Philip Peterson
In-Reply-To: <2c60c4406d4eb1307a32f23604f3ef8e34ad56d6.1708317938.git.gitgitgadget@gmail.com>
"Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> This patchset makes the parse_range function in apply be non-internal
> linkage in order to expose to the unit testing framework. In so doing,
> because there is another function called parse_range, I gave this one a more
> specific name, parse_fragment_range. Other than that, this commit adds
> several test cases (positive and negative) for the function.
We do not write "I did this, I did that" in our proposed log
message. In addition, guidance on the proposed commit log in a
handful of sections in Documentation/SubmittingPatches would be
helpful.
It may probably be a good idea to split this into a preliminary
patch that makes a symbol extern (and doing nothing else), and
the main patch that adds external caller(s) to the function from
a new unit test.
It certainly is better than doing nothing and just make it extern,
but I am not sure "fragment" is specific enough to make the symbol
clearly belong to "apply" API.
> diff --git a/apply.c b/apply.c
> index 7608e3301ca..199a1150df6 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)
> return ptr - line;
> }
>
> -static int parse_range(const char *line, int len, int offset, const char *expect,
> - unsigned long *p1, unsigned long *p2)
> +int parse_fragment_range(const char *line, int len, int offset, const char *expect,
> + unsigned long *p1, unsigned long *p2)
> {
> int digits, ex;
Alternatively we could do something like this to make the blast
radius of this patch smaller.
-static int parse_range(const char *line, int len, int offset, const char *expect,
+#define apply_parse_fragment_range parse_range
+int parse_range(const char *line, int len, int offset, const char *expect,
unsigned long *p1, unsigned long *p2)
If not for unit-test, this function has no reason to be extern with
such a long name, so it is better to allow internal callers to refer
to it with the name that has been good enough for them for the past
19 years since it was introduced in fab2c257 (git-apply: make the
diffstat output happen for "--stat" only., 2005-05-26).
> diff --git a/apply.h b/apply.h
> index 7cd38b1443c..bbc5e3caeb5 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,
> int options);
>
> #endif
> +
> +
> +int parse_fragment_range(const char *line, int len, int offset, const char *expect,
> + unsigned long *p1, unsigned long *p2);
This is wrong. The #endif is about avoiding double inclusion of
this header file and any new declaration must go before it.
> diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
> new file mode 100644
This should go to the next patch.
^ permalink raw reply
* Re: [PATCH v5 0/9] Enrich Trailer API
From: Christian Couder @ 2024-02-19 21:40 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Josh Steadmon, Randall S. Becker, Kristoffer Haugsbakk,
Linus Arver
In-Reply-To: <pull.1632.v5.git.1708124950.gitgitgadget@gmail.com>
On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> In summary this series breaks up "process_trailers()" into smaller pieces,
> exposing many of the parts relevant to trailer-related processing in
> trailer.h. This will force us to eventually introduce unit tests for these
> API functions, but that is a good thing for API stability. We also perform
> some preparatory refactors in order to help us unify the trailer formatting
> machinery toward the end of this series.
I took another look and suggested some improvements to commit messages.
Thanks!
^ permalink raw reply
* Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked
From: Ricardo C @ 2024-02-19 21:41 UTC (permalink / raw)
To: Jean-Noël Avila, git
In-Reply-To: <fc7a8c46-61e4-4b5d-b625-cbc845b81590@gmail.com>
Hello,
On 2/19/24 03:04, Jean-Noël Avila wrote:
> I'm not sure this would be better, but instead of mixing option compatibility
> and actually building your logic, why not use a series of
> die_for_incompatible_opt3 and the like in order to clear the option lists,
> then build your action logic without resorting to special values?
I'm not sure dying would be appropriate here, since the original behavior was
already to clean up buffers and such and then return an errorful value (-1),
and I'd rather keep maximum backwards compatibility. Also, the special value
of -1 for `keep_index` and `include_untracked` is necessary to detect whether
the variable was set by the CLI flags or if it is fine to override with the
config value.
This could be addressed by moving more logic up to `push_stash` and
`save_stash` (where the arguments are parsed), but this would need much more
rewriting and would lead to some code duplication, for what I think is minimal
gain.
Thank you,
Ricardo
^ permalink raw reply
* Re: [PATCH 2/2] apply: rewrite unit tests with structured cases
From: Junio C Hamano @ 2024-02-19 21:49 UTC (permalink / raw)
To: Philip Peterson via GitGitGadget; +Cc: git, Philip Peterson
In-Reply-To: <7dab12ab7b8af3e6a0778fc1a01dd1479990bcff.1708317938.git.gitgitgadget@gmail.com>
"Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> The imperative format was a little hard to read, so I rewrote the test cases
> in a declarative style by defining a common structure for each test case and
> its assertions.
>
> Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
> ---
> t/unit-tests/t-apply.c | 123 ++++++++++++++++++++++++++---------------
> 1 file changed, 78 insertions(+), 45 deletions(-)
In this project, we do not add a version of a known-to-be-bad file
in patch [1/2], only to be immediately improved in patch [2/2].
Unless, of course, there is a good reason to do so. And "to
preserve the true history of what happened in the developer's
working tree" is not a good reason.
We give our developers "rebase -i" and other means to rewrite their
Git history, not just because we want them to be able to pretend
that they are a better developer who never make such a mistake or
misdesign in the first place, but because a polished history is
easier to review in the shorter term and to maintain in the longer
term.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] apply: rewrite unit tests with structured cases
From: Kristoffer Haugsbakk @ 2024-02-19 22:04 UTC (permalink / raw)
To: Josh Soref; +Cc: Philip Peterson, git
In-Reply-To: <7dab12ab7b8af3e6a0778fc1a01dd1479990bcff.1708317938.git.gitgitgadget@gmail.com>
On Mon, Feb 19, 2024, at 05:45, Philip Peterson via GitGitGadget wrote:
> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> The imperative format was a little hard to read, so I rewrote the test cases
> in a declarative style by defining a common structure for each test case and
> its assertions.
>
> Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
IMO in general you can just assert that X and Y in the commit message.
“ The imperative format is hard to read. Rewrite the test cases …
If your patch passes review and is merged then that’s the truth as
determined by you and the reviewers.
More subjective-sounding “This was hard to read” and maybe anecdotes
like “this tripped me up when reading” can go outside the commit message
like the cover letter or the free-form space between the commit message
and the patch (after the three-hyphen/three-dash lines).
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 2/6] dir-iterator: support iteration in sorted order
From: Junio C Hamano @ 2024-02-19 23:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <8a588175dbf23d1938db45507812aad8f3793dbb.1708353264.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The `struct dir_iterator` is a helper that allows us to iterate through
> directory entries. This iterator returns entries in the exact same order
> as readdir(3P) does -- or in other words, it guarantees no specific
> order at all.
>
> This is about to become problematic as we are introducing a new reflog
> subcommand to list reflogs. As the "files" backend uses the directory
> iterator to enumerate reflogs, returning reflog names and exposing them
> to the user would inherit the indeterministic ordering. Naturally, it
> would make for a terrible user interface to show a list with no
> discernible order. While this could be handled at a higher level by the
> new subcommand itself by collecting and ordering the reflogs, this would
> be inefficient and introduce latency when there are many reflogs.
I do not quite understand this argument. Why is sorting at higher
level less (or more, for that matter) efficient than doing so at
lower level? We'd need to sort somewhere no matter what, and I of
course have no problem in listing in a deterministic order.
> Instead, introduce a new option into the directory iterator that asks
> for its entries to be yielded in lexicographical order. If set, the
> iterator will read all directory entries greedily end sort them before
> we start to iterate over them.
"end" -> "and". And of course without such sorting option, this
codepath is allowed to yield entries in any order that is the
easiest to produce? That makes sense.
> While this will of course also incur overhead as we cannot yield the
> directory entries immediately, it should at least be more efficient than
> having to sort the complete list of reflogs as we only need to sort one
> directory at a time.
True. The initial latency before we see the first byte of the
output often matters more in perceived performance the throughput.
As we need to sort to give a reasonable output, that cannot be
avoided.
> This functionality will be used in a follow-up commit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> dir-iterator.c | 87 ++++++++++++++++++++++++++++++++++++++++----------
> dir-iterator.h | 3 ++
> 2 files changed, 73 insertions(+), 17 deletions(-)
>
> diff --git a/dir-iterator.c b/dir-iterator.c
> index f58a97e089..396c28178f 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -2,9 +2,12 @@
> #include "dir.h"
> #include "iterator.h"
> #include "dir-iterator.h"
> +#include "string-list.h"
>
> struct dir_iterator_level {
> DIR *dir;
> + struct string_list entries;
> + size_t entries_idx;
Does it deserve a comment that "dir == NULL" is used as a signal
that we have read the level and sorted its contents into the
"entries" list (and also we have already called closedir(), of
course)?
> @@ -72,6 +75,40 @@ static int push_level(struct dir_iterator_int *iter)
> return -1;
> }
>
> + string_list_init_dup(&level->entries);
> + level->entries_idx = 0;
> +
> + /*
> + * When the iterator is sorted we read and sort all directory entries
> + * directly.
> + */
> + if (iter->flags & DIR_ITERATOR_SORTED) {
> + while (1) {
> + struct dirent *de;
> +
> + errno = 0;
> + de = readdir(level->dir);
> + if (!de) {
> + if (errno && errno != ENOENT) {
> + warning_errno("error reading directory '%s'",
> + iter->base.path.buf);
> + return -1;
> + }
> +
> + break;
> + }
> +
> + if (is_dot_or_dotdot(de->d_name))
> + continue;
The condition to skip an entry currently is simple enough that "."
and ".." are the only ones that are skipped, but it must be kept in
sync with the condition in dir_iterator_advance().
If it becomes more complex than it is now (e.g., we may start to
skip any name that begins with a dot, like ".git" or ".dummy"), it
probably is a good idea *not* to add the same filtering logic here
and in dir_iterator_advance(). Instead, keep the filtering here to
an absolute minumum, and filter the name, whether it came from
readdir() or from the .entries string list, in a single copy of
filtering logic in dir_iterator_advance() function.
We could drop the dot-or-dotdot filter here, too, if we want to
ensure that unified filtering will be correctly done over there.
> + string_list_append(&level->entries, de->d_name);
> + }
> + string_list_sort(&level->entries);
> +
> + closedir(level->dir);
> + level->dir = NULL;
> + }
> +
> return 0;
> }
>
> @@ -88,6 +125,7 @@ static int pop_level(struct dir_iterator_int *iter)
> warning_errno("error closing directory '%s'",
> iter->base.path.buf);
> level->dir = NULL;
> + string_list_clear(&level->entries, 0);
>
> return --iter->levels_nr;
> }
It is somewhat interesting that the original code already has
conditional call to closedir() and prepares .dir to be NULL,
so that we do not have to make it conditional here.
> @@ -136,30 +174,43 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>
> /* Loop until we find an entry that we can give back to the caller. */
> while (1) {
> - struct dirent *de;
> struct dir_iterator_level *level =
> &iter->levels[iter->levels_nr - 1];
> + struct dirent *de;
> + const char *name;
Not a huge deal but this is an unnecessary reordering, right?
> strbuf_setlen(&iter->base.path, level->prefix_len);
> +
> + if (level->dir) {
> + errno = 0;
> + de = readdir(level->dir);
> + if (!de) {
> + if (errno) {
> + warning_errno("error reading directory '%s'",
> + iter->base.path.buf);
> + if (iter->flags & DIR_ITERATOR_PEDANTIC)
> + goto error_out;
> + } else if (pop_level(iter) == 0) {
> + return dir_iterator_abort(dir_iterator);
> + }
> + continue;
> }
>
> + if (is_dot_or_dotdot(de->d_name))
> + continue;
This is the target of the "if we will end up filtering even more in
the future, it would probably be a good idea not to duplicate the
logic to decide what gets filtered in this function and in
push_level()" comment. If we wanted to go that route, we can get
rid of the filtering from push_level(), and move this filter code
outside this if/else before calling prepare_next_entry_data().
The fact that .entries.nr represents the number of entries that are
shown is unusable (because there is an unsorted codepath that does
not even populate .entries), so I am not worried about correctness
gotchas caused by including names in .entries to be filtered out.
But an obvious downside is that the size of the list to be sorted
will become larger.
Or we could introduce a shared helper function that takes a name and
decides if it is to be included, and replace the is_dot_or_dotdot()
call here and in the push_level() with calls to that helper.
In any case, that is primarily a maintainability issue. The code
posted as-is is correct.
> + name = de->d_name;
> + } else {
> + if (level->entries_idx >= level->entries.nr) {
> + if (pop_level(iter) == 0)
> + return dir_iterator_abort(dir_iterator);
> + continue;
> + }
> +
> + name = level->entries.items[level->entries_idx++].string;
> + }
> +
> + if (prepare_next_entry_data(iter, name)) {
> if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
> goto error_out;
> continue;
> @@ -188,6 +239,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
> warning_errno("error closing directory '%s'",
> iter->base.path.buf);
> }
> +
> + string_list_clear(&level->entries, 0);
> }
>
> free(iter->levels);
> diff --git a/dir-iterator.h b/dir-iterator.h
> index 479e1ec784..6d438809b6 100644
> --- a/dir-iterator.h
> +++ b/dir-iterator.h
> @@ -54,8 +54,11 @@
> * and ITER_ERROR is returned immediately. In both cases, a meaningful
> * warning is emitted. Note: ENOENT errors are always ignored so that
> * the API users may remove files during iteration.
> + *
> + * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.
> */
> #define DIR_ITERATOR_PEDANTIC (1 << 0)
> +#define DIR_ITERATOR_SORTED (1 << 1)
>
> struct dir_iterator {
> /* The current path: */
^ permalink raw reply
* Re: [GSoC] Use unsigned integral type for collection of bits
From: eugenio gigante @ 2024-02-19 23:43 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAFJh0PRJkVBr-A=UtmEcAh4cPgC3w_vdTPg6kkjgHVQXHTYRmA@mail.gmail.com>
On Sun, Feb 18, 2024 at 20:09 AM eric sunshine
<sunshine@sunshineco.com> wrote:
> The code in question is not being used as a "bag of bits". Rather,
> it's a tristate binary with values "not-set", "true", and "false".
> Whereas a typical binary could be represented by a single bit, this
> one needs the extra bit to handle the "not-set" case. Moreover, it is
> idiomatic in the Git codebase for -1 to represent "not-set", so I
> think this code is fine as-is since its meaning is clear to those
> familiar with the codebase, thus does not need any changes made to it.
Thank you for the clarification and sorry for the misunderstanding.
> So, refresh_index() is correctly expecting an unsigned value for
> `flags` but refresh() in `builtin/add.c` has undesirably declared
> `flags` as signed.
So, an unsigned type is preferable since we are dealing
with 'bags of bits', and probably only bitwise operators operate
on them. Also the mixing is not ideal.
Yes, I'm interested in fixing the one in `builtin/add.c`.
Il giorno mar 20 feb 2024 alle ore 00:39 eugenio gigante
<giganteeugenio2@gmail.com> ha scritto:
>
> On Sun, Feb 18, 2024 at 20:09 AM eric sunshine
> <sunshine@sunshineco.com> wrote:
> > The code in question is not being used as a "bag of bits". Rather,
> > it's a tristate binary with values "not-set", "true", and "false".
> > Whereas a typical binary could be represented by a single bit, this
> > one needs the extra bit to handle the "not-set" case. Moreover, it is
> > idiomatic in the Git codebase for -1 to represent "not-set", so I
> > think this code is fine as-is since its meaning is clear to those
> > familiar with the codebase, thus does not need any changes made to it.
>
> Thank you for the clarification and sorry for the misunderstanding.
>
> > So, refresh_index() is correctly expecting an unsigned value for
> > `flags` but refresh() in `builtin/add.c` has undesirably declared
> > `flags` as signed.
>
> So, an unsigned type is preferable since we are dealing
> with 'bags of bits', and probably only bitwise operators operate
> on them. Also the mixing is not ideal.
> Yes, I'm interested in fixing the one in `builtin/add.c`.
^ 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