* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Johannes Schindelin @ 2024-01-12 13:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Stan Hu
In-Reply-To: <ZaEZar9OTVgfkD9r@framework>
Hi Patrick,
On Fri, 12 Jan 2024, Patrick Steinhardt wrote:
> On Fri, Jan 12, 2024 at 11:08:21AM +0100, Johannes Schindelin wrote:
>
> > On Thu, 11 Jan 2024, Patrick Steinhardt wrote:
> >
> > > The Bash completion script must not print anything to either stdout or
> > > stderr. Instead, it is only expected to populate certain variables.
> > > Tighten our `test_completion ()` test helper to verify this requirement.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > > t/t9902-completion.sh | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > > index aa9a614de3..78cb93bea7 100755
> > > --- a/t/t9902-completion.sh
> > > +++ b/t/t9902-completion.sh
> > > @@ -87,9 +87,11 @@ test_completion ()
> > > else
> > > sed -e 's/Z$//' |sort >expected
> > > fi &&
> > > - run_completion "$1" &&
> > > + run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
> > > sort out >out_sorted &&
> > > - test_cmp expected out_sorted
> > > + test_cmp expected out_sorted &&
> > > + test_must_be_empty "$TRASH_DIRECTORY"/output &&
> >
> > It seems that this fails CI on macOS, most likely because we're running
> > with `set -x` and that output somehow ends up in `output`, see e.g. here:
> > https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880
> >
> > [...]
> > ++ test_completion 'git switch '
> > ++ test 1 -gt 1
> > ++ sed -e 's/Z$//'
> > ++ sort
> > ++ run_completion 'git switch '
> > ++ sort out
> > ++ test_cmp expected out_sorted
> > ++ test 2 -ne 2
> > ++ eval 'diff -u' '"$@"'
> > +++ diff -u expected out_sorted
> > ++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ test 1 -ne 1
> > ++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ test 1 -ne 1
> > ++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:'
> > '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
> > ++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> > ++ local -a COMPREPLY _words
> > ++ local _cword
> > [...]
> >
> > Maybe this is running in Dash and therefore `BASH_XTRACEFD=4` in
> > `test-lib.sh` has not the intended effect?
>
> Meh, thanks for the heads up. Another test gap in GitLab CI which I'm
> going to address soon via a new macOS job.
>
> In any case, Dash indeed does not honor the above envvar.
Hmm. I had a closer look and now I am thoroughly confused. In
`t/lib-bash.exe`, we make sure that this test is run in Bash. And indeed,
when I call
BASH=1 POSIXLY_CORRECT=1 TEST_SHELL_PATH=/bin/dash dash t9902-completion.sh -iVx
I am greeted by this error message:
git-completion.bash: Syntax error: "(" unexpected (expecting "fi")
So something else must be going on here.
> I also could not find any alternate solutions to redirect the tracing
> output. So for all I can see there are a few ways to handle this:
>
> - `set -x` and then restore the previous value after having called
> `run_completion`.
>
> - Filter the output so that any line starting with "${PS4}" gets
> removed.
>
> - Don't test for this bug.
>
> Not sure which way to go, but the first alternative feels a bit more
> sensible to me. It does remove the ability to see what's going on in the
> completion script though in case one wants to debug it.
Personally, I would go for option 2, filtering out the xtrace output. This
here seems to work:
-- snip --
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b14ae4de14e..23cd1cd9508 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -87,11 +87,14 @@ test_completion ()
else
sed -e 's/Z$//' |sort >expected
fi &&
+ PS4=+ &&
run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
+ sed "/^+/{:1;s/'[^']*'//g;/'/{N;b1};d}" \
+ <"$TRASH_DIRECTORY"/output >"$TRASH_DIRECTORY"/output.x &&
+ test_must_be_empty "$TRASH_DIRECTORY"/output.x &&
+ rm "$TRASH_DIRECTORY"/output "$TRASH_DIRECTORY"/output.x &&
sort out >out_sorted &&
- test_cmp expected out_sorted &&
- test_must_be_empty "$TRASH_DIRECTORY"/output &&
- rm "$TRASH_DIRECTORY"/output
+ test_cmp expected out_sorted
}
# Test __gitcomp.
-- snap --
It is a bit ugly, in particular the `sed` expression (which is a bit
complex because the `output` file can contain multi-line strings enclosed
in single-quotes), and we could probably lose the `"$TRASH_DIRECTORY"/`
part to improve the reading experience somewhat.
But my main concern is: Why does this happen in the first place? If we are
running with Bash, why does `BASH_XTRACEFD` to work as intended here and
makes it necessary to filter out the traced commands?
Ciao,
Johannes
^ permalink raw reply related
* Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
From: Mohit Marathe @ 2024-01-12 13:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, git@vger.kernel.org, britton.kerin@gmail.com,
peff@peff.net
In-Reply-To: <xmqqa5pdav04.fsf@gitster.g>
> > I took a closer look at `builtin/patch-id.c` and it seems replacing
> > `atoi()` (which is used to parse numbers in the hunk header) wouldn't
> > improve anything, unless I'm missing something.
>
>
> You can of course notice an invalid patch that places non-digit to
> the hunk header and error out with such a change. If we are reading
> output from Git that we are invoking, hopefully we will not see such
> an invalid patch, but the command is designed to read arbitrary
> input like a patch e-mail you received over the network, so I do not
> think it is fair to say there is no merit to such a change, even
> though I agree that it may not matter all that much.
>
> A corrupt patch may be getting a nonsense patch-ID with the current
> code and hopefully is not matching other patches that are not
> corrupt, but with such a change, a corrupt patch may not be getting
> any patch-ID and a loop that computes patch-ID for many files and
> try to match them up might need to be rewritten to take the new
> failure case into account.
Oh, I didn't know corrupt patches were a thing. I will start working
on the patch for this.
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: phillip.wood123 @ 2024-01-12 11:01 UTC (permalink / raw)
To: Michael Lohmann, git
In-Reply-To: <20240111233311.64734-1-mi.al.lohmann@gmail.com>
Hi Michael
On 11/01/2024 23:33, Michael Lohmann wrote:
> This extends the functionality of `git log --merge` to also work with
> conflicts for rebase, cherry pick and revert.
>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
> Hi everybody!
>
> When Phillip Wood gave me some nice hints on his workflow concerning
> conflicts [1] (we discussed if `--show-current-patch` would make sense
> for cherry pick/revert). When I learned about `git log --merge` I was a
> bit sad to discover that those do not exist for rebase/revert/cherry
> pick since they look really valuable for getting an understanding on
> what was changed. It is basically the counterpart to
> `git show ${ACTION}_HEAD` for understanding the source of the conflict.
>
> I am curious if also other people would be interested in having an easy
> way to get a log of only the relevant commits touching conflicting files
> for said actions.
>
> With this patch I think the functionality is there, just hijacking the
> `--merge` code - maybe an alias like `git log --conflict` or similar
> might be more descriptive now?
>
> What do you think about this idea? (Or am I maybe missing an easy way to
> achieve it with the existing code as well?)
I should start by saying that I didn't know "git log --merge" existed
before I saw this message so please correct me if I've misunderstood
what this patch is doing. If I understand correctly it shows the commits
from each side of the merge and is equivalent to
git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^
[*] so I'm not sure what
git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a common
ancestor. As I say I've not used "git log --merge" so maybe I'm missing
the reason why it would be useful when cherry-picking or rebasing.
Best Wishes
Phillip
[*] assuming we're not picking a merge commit
> Michael
>
>
> [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/
>
> revision.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..2e5c00dabd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,
> }
> }
>
> +static char* get_action_head_name(struct object_id* oid)
> +{
> + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) {
> + return "MERGE_HEAD";
> + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) {
> + return "REBASE_HEAD";
> + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) {
> + return "CHERRY_PICK_HEAD";
> + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) {
> + return "REVERT_HEAD";
> + } else
> + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
> +}
> +
> static void prepare_show_merge(struct rev_info *revs)
> {
> struct commit_list *bases;
> struct commit *head, *other;
> struct object_id oid;
> const char **prune = NULL;
> + const char *action_head_name;
> int i, prune_num = 1; /* counting terminating NULL */
> struct index_state *istate = revs->repo->index;
>
> if (repo_get_oid(the_repository, "HEAD", &oid))
> die("--merge without HEAD?");
> head = lookup_commit_or_die(&oid, "HEAD");
> - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> - die("--merge without MERGE_HEAD?");
> - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> + action_head_name = get_action_head_name(&oid);
> + other = lookup_commit_or_die(&oid, action_head_name);
> add_pending_object(revs, &head->object, "HEAD");
> - add_pending_object(revs, &other->object, "MERGE_HEAD");
> + add_pending_object(revs, &other->object, action_head_name);
> bases = repo_get_merge_bases(the_repository, head, other);
> add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
> add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
^ permalink raw reply
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Patrick Steinhardt @ 2024-01-12 10:50 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Stan Hu
In-Reply-To: <d978494d-6c48-5923-4745-c42a39e1187a@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]
On Fri, Jan 12, 2024 at 11:08:21AM +0100, Johannes Schindelin wrote:
> Hi Patrick,
>
> On Thu, 11 Jan 2024, Patrick Steinhardt wrote:
>
> > The Bash completion script must not print anything to either stdout or
> > stderr. Instead, it is only expected to populate certain variables.
> > Tighten our `test_completion ()` test helper to verify this requirement.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/t9902-completion.sh | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index aa9a614de3..78cb93bea7 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -87,9 +87,11 @@ test_completion ()
> > else
> > sed -e 's/Z$//' |sort >expected
> > fi &&
> > - run_completion "$1" &&
> > + run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
> > sort out >out_sorted &&
> > - test_cmp expected out_sorted
> > + test_cmp expected out_sorted &&
> > + test_must_be_empty "$TRASH_DIRECTORY"/output &&
>
> It seems that this fails CI on macOS, most likely because we're running
> with `set -x` and that output somehow ends up in `output`, see e.g. here:
> https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880
>
> [...]
> ++ test_completion 'git switch '
> ++ test 1 -gt 1
> ++ sed -e 's/Z$//'
> ++ sort
> ++ run_completion 'git switch '
> ++ sort out
> ++ test_cmp expected out_sorted
> ++ test 2 -ne 2
> ++ eval 'diff -u' '"$@"'
> +++ diff -u expected out_sorted
> ++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> ++ test 1 -ne 1
> ++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> ++ test 1 -ne 1
> ++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> ++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> ++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:'
> '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
> ++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> ++ local -a COMPREPLY _words
> ++ local _cword
> [...]
>
> Maybe this is running in Dash and therefore `BASH_XTRACEFD=4` in
> `test-lib.sh` has not the intended effect?
Meh, thanks for the heads up. Another test gap in GitLab CI which I'm
going to address soon via a new macOS job.
In any case, Dash indeed does not honor the above envvar. I also could
not find any alternate solutions to redirect the tracing output. So for
all I can see there are a few ways to handle this:
- `set -x` and then restore the previous value after having called
`run_completion`.
- Filter the output so that any line starting with "${PS4}" gets
removed.
- Don't test for this bug.
Not sure which way to go, but the first alternative feels a bit more
sensible to me. It does remove the ability to see what's going on in the
completion script though in case one wants to debug it.
I'll reroll this patch series early next week.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Achu Luma @ 2024-01-12 10:27 UTC (permalink / raw)
To: git
Cc: chriscool, christian.couder, gitster, l.s.r, me, phillip.wood123,
phillip.wood, steadmon, Achu Luma
In-Reply-To: <20240105161413.10422-1-ach.lumap@gmail.com>
In the recent codebase update (8bf6fbd00d (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.
This commit migrates the unit tests for C character classification
functions (isdigit(), isspace(), etc) from the legacy approach
using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
to the new unit testing framework (t/unit-tests/test-lib.h).
The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: René Scharfe <l.s.r@web.de>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
The change between version 4 and version 5 is:
- Added tests to handle EOF.
Thanks to Phillip for noticing the missing tests..
Here is a diff between v4 and v5:
+ if (!check(!func(EOF))) \
+ test_msg(" i: 0x%02x (EOF)", EOF); \
Thanks also to René, Phillip, Junio and Taylor who helped with
previous versions.
Makefile | 2 +-
t/helper/test-ctype.c | 70 ------------------------------------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0070-fundamental.sh | 4 ---
t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 81 insertions(+), 77 deletions(-)
delete mode 100644 t/helper/test-ctype.c
create mode 100644 t/unit-tests/t-ctype.c
diff --git a/Makefile b/Makefile
index 15990ff312..1a62e48759 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
TEST_BUILTINS_OBJS += test-config.o
TEST_BUILTINS_OBJS += test-crontab.o
TEST_BUILTINS_OBJS += test-csprng.o
-TEST_BUILTINS_OBJS += test-ctype.o
TEST_BUILTINS_OBJS += test-date.o
TEST_BUILTINS_OBJS += test-delta.o
TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-mem-pool
UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
- printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
- rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
- /*
- * We can't find NUL using strchr. Accept it as the first
- * character in the spec -- there are no empty classes.
- */
- if (ch == '\0')
- return ch == *s;
- if (*s == '\0')
- s++;
- return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) { \
- int i; \
- for (i = 0; i < 256; i++) { \
- if (is_in(s, i) != t(i)) \
- report_error(#t, i); \
- } \
- if (t(EOF)) \
- report_error(#t, EOF); \
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
- "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
- "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
- "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
- "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
- "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
- "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
- "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
- "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
- "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
- "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
- "\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
- TEST_CLASS(isdigit, DIGIT);
- TEST_CLASS(isspace, " \n\r\t");
- TEST_CLASS(isalpha, LOWER UPPER);
- TEST_CLASS(isalnum, LOWER UPPER DIGIT);
- TEST_CLASS(is_glob_special, "*?[\\");
- TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
- TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
- TEST_CLASS(isascii, ASCII);
- TEST_CLASS(islower, LOWER);
- TEST_CLASS(isupper, UPPER);
- TEST_CLASS(iscntrl, CNTRL);
- TEST_CLASS(ispunct, PUNCT);
- TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
- TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
- return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
{ "config", cmd__config },
{ "crontab", cmd__crontab },
{ "csprng", cmd__csprng },
- { "ctype", cmd__ctype },
{ "date", cmd__date },
{ "delta", cmd__delta },
{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
int cmd__config(int argc, const char **argv);
int cmd__crontab(int argc, const char **argv);
int cmd__csprng(int argc, const char **argv);
-int cmd__ctype(int argc, const char **argv);
int cmd__date(int argc, const char **argv);
int cmd__delta(int argc, const char **argv);
int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
-test_expect_success 'character classes (isspace, isalpha etc.)' '
- test-tool ctype
-'
-
test_expect_success 'mktemp to nonexistent directory prints filename' '
test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..f315489984
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,80 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+ /*
+ * We can't find NUL using strchr. Accept it as the first
+ * character in the spec -- there are no empty classes.
+ */
+ if (ch == '\0')
+ return ch == *s;
+ if (*s == '\0')
+ s++;
+ return !!strchr(s, ch);
+}
+
+/* Macro to test a character type */
+#define TEST_CTYPE_FUNC(func, string) \
+static void test_ctype_##func(void) { \
+ for (int i = 0; i < 256; i++) { \
+ if (!check_int(func(i), ==, is_in(string, i))) \
+ test_msg(" i: 0x%02x", i); \
+ } \
+ if (!check(!func(EOF))) \
+ test_msg(" i: 0x%02x (EOF)", EOF); \
+}
+
+#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+ "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+ "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+ "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+ "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+ "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+ "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+ "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+ "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+ "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+ "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+ "\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+int cmd_main(int argc, const char **argv) {
+ /* Run all character type tests */
+ TEST_CHAR_CLASS(isspace);
+ TEST_CHAR_CLASS(isdigit);
+ TEST_CHAR_CLASS(isalpha);
+ TEST_CHAR_CLASS(isalnum);
+ TEST_CHAR_CLASS(is_glob_special);
+ TEST_CHAR_CLASS(is_regex_special);
+ TEST_CHAR_CLASS(is_pathspec_magic);
+ TEST_CHAR_CLASS(isascii);
+ TEST_CHAR_CLASS(islower);
+ TEST_CHAR_CLASS(isupper);
+ TEST_CHAR_CLASS(iscntrl);
+ TEST_CHAR_CLASS(ispunct);
+ TEST_CHAR_CLASS(isxdigit);
+ TEST_CHAR_CLASS(isprint);
+
+ return test_done();
+}
--
2.42.0.windows.2
^ permalink raw reply related
* [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Achu Luma @ 2024-01-12 10:21 UTC (permalink / raw)
To: git; +Cc: christian.couder, Achu Luma, Christian Couder
In the recent codebase update (8bf6fbd00d (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.
This commit migrates the unit tests for advise_if_enabled functionality
from the legacy approach using the test-tool command `test-tool advise`
in t/helper/test-advise.c to the new unit testing framework
(t/unit-tests/test-lib.h).
The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
Makefile | 2 +-
t/helper/test-advise.c | 22 -----------------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0018-advice.sh | 33 -------------------------
t/unit-tests/t-advise.c | 53 +++++++++++++++++++++++++++++++++++++++++
6 files changed, 54 insertions(+), 58 deletions(-)
delete mode 100644 t/helper/test-advise.c
delete mode 100755 t/t0018-advice.sh
create mode 100644 t/unit-tests/t-advise.c
diff --git a/Makefile b/Makefile
index 15990ff312..27916e4341 100644
--- a/Makefile
+++ b/Makefile
@@ -783,7 +783,6 @@ X =
PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
-TEST_BUILTINS_OBJS += test-advise.o
TEST_BUILTINS_OBJS += test-bitmap.o
TEST_BUILTINS_OBJS += test-bloom.o
TEST_BUILTINS_OBJS += test-bundle-uri.o
@@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-mem-pool
UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-advise
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
deleted file mode 100644
index 8a3fd0009a..0000000000
--- a/t/helper/test-advise.c
+++ /dev/null
@@ -1,22 +0,0 @@
-#include "test-tool.h"
-#include "advice.h"
-#include "config.h"
-#include "setup.h"
-
-int cmd__advise_if_enabled(int argc, const char **argv)
-{
- if (argc != 2)
- die("usage: %s <advice>", argv[0]);
-
- setup_git_directory();
- git_config(git_default_config, NULL);
-
- /*
- * Any advice type can be used for testing, but NESTED_TAG was
- * selected here and in t0018 where this command is being
- * executed.
- */
- advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
-
- return 0;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..e7f7326ce6 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -10,7 +10,6 @@ static const char * const test_tool_usage[] = {
};
static struct test_cmd cmds[] = {
- { "advise", cmd__advise_if_enabled },
{ "bitmap", cmd__bitmap },
{ "bloom", cmd__bloom },
{ "bundle-uri", cmd__bundle_uri },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..68751dda66 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -3,7 +3,6 @@
#include "git-compat-util.h"
-int cmd__advise_if_enabled(int argc, const char **argv);
int cmd__bitmap(int argc, const char **argv);
int cmd__bloom(int argc, const char **argv);
int cmd__bundle_uri(int argc, const char **argv);
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
deleted file mode 100755
index c13057a4ca..0000000000
--- a/t/t0018-advice.sh
+++ /dev/null
@@ -1,33 +0,0 @@
-#!/bin/sh
-
-test_description='Test advise_if_enabled functionality'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'advice should be printed when config variable is unset' '
- cat >expect <<-\EOF &&
- hint: This is a piece of advice
- hint: Disable this message with "git config advice.nestedTag false"
- EOF
- test-tool advise "This is a piece of advice" 2>actual &&
- test_cmp expect actual
-'
-
-test_expect_success 'advice should be printed when config variable is set to true' '
- cat >expect <<-\EOF &&
- hint: This is a piece of advice
- hint: Disable this message with "git config advice.nestedTag false"
- EOF
- test_config advice.nestedTag true &&
- test-tool advise "This is a piece of advice" 2>actual &&
- test_cmp expect actual
-'
-
-test_expect_success 'advice should not be printed when config variable is set to false' '
- test_config advice.nestedTag false &&
- test-tool advise "This is a piece of advice" 2>actual &&
- test_must_be_empty actual
-'
-
-test_done
diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
new file mode 100644
index 0000000000..15df29c955
--- /dev/null
+++ b/t/unit-tests/t-advise.c
@@ -0,0 +1,53 @@
+#include "test-lib.h"
+#include "advice.h"
+#include "config.h"
+#include "setup.h"
+#include "strbuf.h"
+
+static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
+ "hint: Disable this message with \"git config advice.nestedTag false\"\n";
+static const char advice_msg[] = "This is a piece of advice";
+static const char out_file[] = "./output.txt";
+
+
+static void check_advise_if_enabled(const char *argv, const char *conf_val, const char *expect) {
+ FILE *file;
+ struct strbuf actual = STRBUF_INIT;
+
+ if (conf_val)
+ git_default_advice_config("advice.nestedTag", conf_val);
+
+ file = freopen(out_file, "w", stderr);
+ if (!check(!!file)) {
+ test_msg("Error opening file %s", out_file);
+ return;
+ }
+
+ advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv);
+ fclose(file);
+
+ if (!check(strbuf_read_file(&actual, out_file, 0) >= 0)) {
+ test_msg("Error reading file %s", out_file);
+ strbuf_release(&actual);
+ return;
+ }
+
+ check_str(actual.buf, expect);
+ strbuf_release(&actual);
+
+ if (!check(remove(out_file) == 0))
+ test_msg("Error deleting %s", out_file);
+}
+
+int cmd_main(int argc, const char **argv) {
+ setenv("TERM", "dumb", 1);
+
+ TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
+ "advice should be printed when config variable is unset");
+ TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
+ "advice should be printed when config variable is set to true");
+ TEST(check_advise_if_enabled(advice_msg, "false", ""),
+ "advice should not be printed when config variable is set to false");
+
+ return test_done();
+}
--
2.42.0.windows.2
^ permalink raw reply related
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Johannes Schindelin @ 2024-01-12 10:08 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Stan Hu
In-Reply-To: <73406ca9c8f38ac2ad8f0e32d6d81f1566a6b4d1.1704969119.git.ps@pks.im>
Hi Patrick,
On Thu, 11 Jan 2024, Patrick Steinhardt wrote:
> The Bash completion script must not print anything to either stdout or
> stderr. Instead, it is only expected to populate certain variables.
> Tighten our `test_completion ()` test helper to verify this requirement.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t9902-completion.sh | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index aa9a614de3..78cb93bea7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -87,9 +87,11 @@ test_completion ()
> else
> sed -e 's/Z$//' |sort >expected
> fi &&
> - run_completion "$1" &&
> + run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
> sort out >out_sorted &&
> - test_cmp expected out_sorted
> + test_cmp expected out_sorted &&
> + test_must_be_empty "$TRASH_DIRECTORY"/output &&
It seems that this fails CI on macOS, most likely because we're running
with `set -x` and that output somehow ends up in `output`, see e.g. here:
https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880
[...]
++ test_completion 'git switch '
++ test 1 -gt 1
++ sed -e 's/Z$//'
++ sort
++ run_completion 'git switch '
++ sort out
++ test_cmp expected out_sorted
++ test 2 -ne 2
++ eval 'diff -u' '"$@"'
+++ diff -u expected out_sorted
++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ test 1 -ne 1
++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ test 1 -ne 1
++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:'
'/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ local -a COMPREPLY _words
++ local _cword
[...]
Maybe this is running in Dash and therefore `BASH_XTRACEFD=4` in
`test-lib.sh` has not the intended effect?
Ciao,
Johannes
^ permalink raw reply
* [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-12 10:05 UTC (permalink / raw)
To: Git List, Junio C Hamano, Taylor Blau, Jeff King, Dragan Simic
In-Reply-To: <7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com>
Using advise_if_enabled() to display an advice will automatically
include instructions on how to disable the advice, alongside the main
advice:
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
To do so, we provide a knob which can be used to disable the advice.
But also to tell us the opposite: to show the advice.
Let's not include the deactivation instructions for an advice if the
user explicitly sets its visibility.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
I hope this patch captures most of the comments from the previous
iteration, mostly:
- Allow to disable the disabling instructions, per advice.
- No new test is needed, therefore the first two commits from the
previous iteration are not needed here.
Thanks.
advice.c | 109 +++++++++++++++++++++++++---------------------
t/t0018-advice.sh | 1 -
2 files changed, 59 insertions(+), 51 deletions(-)
diff --git a/advice.c b/advice.c
index f6e4c2f302..8474966ce1 100644
--- a/advice.c
+++ b/advice.c
@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
return "";
}
+enum advice_level {
+ ADVICE_LEVEL_DEFAULT = 0,
+ ADVICE_LEVEL_DISABLED,
+ ADVICE_LEVEL_ENABLED,
+};
+
static struct {
const char *key;
- int enabled;
+ enum advice_level level;
} advice_setting[] = {
- [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 },
- [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec", 1 },
- [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile", 1 },
- [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec", 1 },
- [ADVICE_AM_WORK_DIR] = { "amWorkDir", 1 },
- [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 },
- [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 },
- [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 },
- [ADVICE_DIVERGING] = { "diverging", 1 },
- [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 },
- [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 },
- [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 },
- [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 },
- [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 },
- [ADVICE_NESTED_TAG] = { "nestedTag", 1 },
- [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning", 1 },
- [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists", 1 },
- [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst", 1 },
- [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce", 1 },
- [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent", 1 },
- [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching", 1 },
- [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate", 1 },
- [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName", 1 },
- [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected", 1 },
- [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward", 1 }, /* backwards compatibility */
- [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh", 1 },
- [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict", 1 },
- [ADVICE_RM_HINTS] = { "rmHints", 1 },
- [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse", 1 },
- [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 },
- [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1 },
- [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 },
- [ADVICE_STATUS_HINTS] = { "statusHints", 1 },
- [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 },
- [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 },
- [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
- [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 },
- [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
- [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
- [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
+ [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" },
+ [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec" },
+ [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile" },
+ [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec" },
+ [ADVICE_AM_WORK_DIR] = { "amWorkDir" },
+ [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName" },
+ [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge" },
+ [ADVICE_DETACHED_HEAD] = { "detachedHead" },
+ [ADVICE_DIVERGING] = { "diverging" },
+ [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates" },
+ [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch" },
+ [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" },
+ [ADVICE_IGNORED_HOOK] = { "ignoredHook" },
+ [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" },
+ [ADVICE_NESTED_TAG] = { "nestedTag" },
+ [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning" },
+ [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists" },
+ [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst" },
+ [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce" },
+ [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent" },
+ [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching" },
+ [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate" },
+ [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
+ [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
+ [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
+ [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
+ [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
+ [ADVICE_RM_HINTS] = { "rmHints" },
+ [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" },
+ [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
+ [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
+ [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning" },
+ [ADVICE_STATUS_HINTS] = { "statusHints" },
+ [ADVICE_STATUS_U_OPTION] = { "statusUoption" },
+ [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated" },
+ [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
+ [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead" },
+ [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath" },
+ [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" },
+ [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" },
};
static const char turn_off_instructions[] =
@@ -116,13 +122,13 @@ void advise(const char *advice, ...)
int advice_enabled(enum advice_type type)
{
- switch(type) {
- case ADVICE_PUSH_UPDATE_REJECTED:
- return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
- advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
- default:
- return advice_setting[type].enabled;
- }
+ int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+
+ if (type == ADVICE_PUSH_UPDATE_REJECTED)
+ return enabled &&
+ advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
+
+ return enabled;
}
void advise_if_enabled(enum advice_type type, const char *advice, ...)
@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
return;
va_start(params, advice);
- vadvise(advice, 1, advice_setting[type].key, params);
+ vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
+ advice_setting[type].key, params);
va_end(params);
}
@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
if (strcasecmp(k, advice_setting[i].key))
continue;
- advice_setting[i].enabled = git_config_bool(var, value);
+ advice_setting[i].level = git_config_bool(var, value)
+ ? ADVICE_LEVEL_ENABLED
+ : ADVICE_LEVEL_DISABLED;
return 0;
}
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index c13057a4ca..0dcfb760a2 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
test_expect_success 'advice should be printed when config variable is set to true' '
cat >expect <<-\EOF &&
hint: This is a piece of advice
- hint: Disable this message with "git config advice.nestedTag false"
EOF
test_config advice.nestedTag true &&
test-tool advise "This is a piece of advice" 2>actual &&
base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v2 2/3] advice: fix an unexpected leading space
From: Rubén Justo @ 2024-01-12 9:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqa5pbcpnx.fsf@gitster.g>
On 11-ene-2024 17:21:22, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > [ ... ]
> > diff --git a/advice.h b/advice.h
> > index 0f584163f5..2affbe1426 100644
> > --- a/advice.h
> > +++ b/advice.h
> > @@ -49,6 +49,7 @@ struct string_list;
> > ADVICE_UPDATE_SPARSE_PATH,
> > ADVICE_WAITING_FOR_EDITOR,
> > ADVICE_SKIPPED_CHERRY_PICKS,
> > + ADVICE_WORKTREE_ADD_ORPHAN,
> > };
> >
> > Note the hunk header, instead of a much more expected:
> >
> > @@ -49,6 +49,7 @@ enum advice_type
>
> Next time, don't include "diff" output in the proposed log message
> without indenting. It makes it hard to read and parse.
My fault. Sorry.
Is there any way to make git-format-patch issue a warning or refuse to
continue when the resulting patch is not going to be accepted by git-am?
>
> The attached is what I queued in my tree.
Thanks.
^ permalink raw reply
* Re: [DISCUSS] Introducing Rust into the Git project
From: Sam James @ 2024-01-12 8:24 UTC (permalink / raw)
To: brian m. carlson
Cc: Sam James, me, git, John Paul Adrian Glaubitz, Helge Deller,
John David Anglin, arsen, Antoni Boucher
In-Reply-To: <ZaB-ayQuGqrS-mL0@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> [[PGP Signed Part:Undecided]]
> On 2024-01-11 at 11:45:07, Sam James wrote:
>> Something I'm a bit concerned about is that right now, neither
>> rustc_codegen_gcc nor gccrs are ready for use here.
>>
>> We've had trouble getting things wired up for rustc_codegen_gcc
>> - which is not to speak against their wonderful efforts - because
>> the Rust community hasn't yet figured out how to handle things which
>> pure rustc supports yet. See
>> e.g. https://github.com/rust-lang/libc/pull/3032.
>
> Is this simply library support in the libc crate? That's very easy to add.
[CC'd the rustc_codegen_gcc maintainer as well as some folks who have
tried using rustc_codegen_gcc for their distributions.]
Evidently not on the last point? ;)
Even just patching it in downstream isn't easy because you then have to
do it for many many packages. But after that PR stalling because of the
policy issue, there wasn't really anywhere to go, because of the
chicken-and-egg situation.
Let alone then, once the libc crate has it, going around and wiring up
in other crates.
The discussion on the PR seems clear that the intention is to not add
it until some policy is revised/formulated? I also don't want to have
to have that debate with every crate just because rustc doesn't support
it.
>
>> I think care should be taken in citing rustc_codegen_gcc and gccrs
>> as options for alternative platforms for now. They will hopefully
>> be great options in the future, but they aren't today, and they probably
>> won't be in the next 6 months at the least.
>
> What specifically is missing for rust_codegen_gcc? I know gccrs is not
> ready at the moment, but I was under the impression that
> rust_codegen_gcc was at least usable. I'm aware it requires some
> patches to GCC, but distros should be able to carry those.
>
> If rust_codegen_gcc isn't viable, then I agree we should avoid making
> Rust mandatory, but I'd like to learn more.
It's in a general state of instability. There's still *very* active work
ongoing in libgccjit (by the rust_codegen_gcc maintainer).
I'd say "you need to patch your GCC" is probably not a good state of
affairs for using something critical like git anyway, but even then,
I'm not aware of anyone having used it to build real-world common
applications using Rust for a non-rustc-supported platform, at least
not then using those builds day-to-day.
So, even if we were willing to chase the active flurry of libgccjit
patches (which is wonderful to see!), it's a significant moving
target. In Gentoo, we're probably better-placed than most people
to be able to do that, but it's still a lot of work and it doesn't
sound very robust for us to be doing for core infrastructure.
We have a lot of packages in Gentoo - partly actually stuff in the
Python ecosystem - where we're very excited to be able to use
rust_codegen_gcc (or gccrs, whichever comes first inreadiness, surely
rust_codegen_gcc) for alt platforms, but it's just not there yet.
^ permalink raw reply
* Re: [PATCH v4 4/4] archive: support remote archive from stateless transport
From: Linus Arver @ 2024-01-12 8:12 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin, Eric Sunshine
In-Reply-To: <18b9a11d3be9d804e8d22d054ea881b8336d170c.1702562879.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Even though we can establish a stateless connection, we still cannot
> archive the remote repository using a stateless HTTP protocol. Try the
> following steps to make it work.
As Yoda once said, "Do or do not, there is no try". Here I think you
meant "Do".
> 1. Add support for "git-upload-archive" service in "http-backend".
>
> 2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
> protocol version, instead of use the "git-upload-archive" service.
>
> 3. "git-archive" does not expect to see protocol version and
> capabilities when connecting to remote-helper, so do not send them
> in "remote-curl.c" for the "git-upload-archive" service.
It would be great if you could break up this patch into 3 smaller
patches. Or 4 patches if you decide to move the new test cases into their
own patch.
> @@ -723,7 +729,8 @@ static struct service_cmd {
> {"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
>
> {"POST", "/git-upload-pack$", service_rpc},
> - {"POST", "/git-receive-pack$", service_rpc}
> + {"POST", "/git-receive-pack$", service_rpc},
> + {"POST", "/git-upload-archive$", service_rpc}
> };
Style nit: it might be cleaner to put the new "git-upload-archive" just
above "git-upload-pack" because the two have a special relationship now.
^ permalink raw reply
* Re: [PATCH 1/2] t1401: generalize reference locking
From: Jeff King @ 2024-01-12 8:03 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Justin Tobler via GitGitGadget, git, Justin Tobler
In-Reply-To: <ZaDuEufXOnwH7hT4@framework>
On Fri, Jan 12, 2024 at 08:45:22AM +0100, Patrick Steinhardt wrote:
> > The obvious quick fix is to sprinkle more error() into the reftable
> > code. But in the longer term, I think the right direction is that the
> > ref code should accept an error strbuf or similar mechanism to propagate
> > human-readable error test to the caller.
>
> Agreed, I think it's good that the reftable library itself does not
> print error messages. In this particular case we are already able to
> provide a proper error message due to the error code that the library
> returns. But there are certainly going to be other cases where it might
> make sense to pass in an error strbuf.
Oh, if there is an error code you can use already, that is even better. :)
Thanks for taking care of this case.
-Peff
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Johannes Sixt @ 2024-01-12 7:59 UTC (permalink / raw)
To: Michael Lohmann; +Cc: phillip.wood123, git
In-Reply-To: <ce46229d-8964-4445-9a17-cff40aca1cb4@kdbg.org>
Am 12.01.24 um 08:35 schrieb Johannes Sixt:
> ... (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
The Github UI is a bit unhelpful here. The author date is Nov 11, 2014.
-- Hannes
^ permalink raw reply
* Re: [PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper
From: Linus Arver @ 2024-01-12 7:56 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <af8fd2a4eb8783be4a62973bfd2135da4568570e.1702562879.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> After successfully connecting to the smart transport by calling
> process_connect_service() in connect_helper(), run do_take_over() to
> replace the old vtable with a new one which has methods ready for the
> smart transport connection.
>
>
> The connect_helper() function is used as the connect method of the
> vtable in "transport-helper.c", and it is called by transport_connect()
> in "transport.c" to setup a connection. The only place that we call
> transport_connect() so far is in "builtin/archive.c". Without running
> do_take_over(), it may fail to call transport_disconnect() in
> run_remote_archiver() of "builtin/archive.c". This is because for a
> stateless connection or a service like "git-upload-pack-archive", the
There is "git-upload-pack" and "git-upload-archive". Which one did you
mean here? Or did you mean both?
> remote helper may receive a SIGPIPE signal and exit early. To have a
> graceful disconnect method by calling do_take_over() will solve this
> issue.
Are you saying that this patch fixes an existing bug? That is, is this
patch independent of the first patch (transport-helper: no connection
restriction in connect_helper) in this series?
> The subsequent commit will introduce remote archive over a stateless-rpc
> connection.
Does the next commit depend on this patch? If not, I think you can drop
this paragraph.
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> transport-helper.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 51088cc03a..3b036ae1ca 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,
>
> fd[0] = data->helper->out;
> fd[1] = data->helper->in;
> +
> + do_take_over(transport);
> return 0;
> }
>
> --
> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply
* Re: [PATCH 1/2] t1401: generalize reference locking
From: Patrick Steinhardt @ 2024-01-12 7:45 UTC (permalink / raw)
To: Jeff King; +Cc: Justin Tobler via GitGitGadget, git, Justin Tobler
In-Reply-To: <20240112070142.GD618729@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]
On Fri, Jan 12, 2024 at 02:01:42AM -0500, Jeff King wrote:
> On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:
>
> > > (note that you get a different error message if the refs are packed,
> > > since there we can notice the d/f conflict manually).
> >
> > If all we care for is the exit code then this would work for the
> > reftable backend, too:
> >
> > ```
> > $ git init --ref-format=reftable repo
> > Initialized empty Git repository in /tmp/repo/.git/
> > $ cd repo/
> > $ git commit --allow-empty --message message
> > [main (root-commit) c2512d3] x
> > $ git symbolic-ref refs/heads refs/heads/foo
> > $ echo $?
> > 1
> > ```
>
> Yep, exactly. That should work for both and cover what the test was
> originally trying to do.
>
> > A bit unfortunate that there is no proper error message in that case,
> > but that is a different topic.
>
> Yeah, I would call that an outright bug. It does not have to be part of
> this patch, but is worth fixing (and testing). I suspect it's not going
> to be the only place with this problem. Most of the files-backend ref
> code is very happy to spew to stdout using error(), but the reftable
> code, having been written from a more lib-conscious perspective,
> probably doesn't.
Yup, I've already fixed this bug in the reftable backend.
> The obvious quick fix is to sprinkle more error() into the reftable
> code. But in the longer term, I think the right direction is that the
> ref code should accept an error strbuf or similar mechanism to propagate
> human-readable error test to the caller.
Agreed, I think it's good that the reftable library itself does not
print error messages. In this particular case we are already able to
provide a proper error message due to the error code that the library
returns. But there are certainly going to be other cases where it might
make sense to pass in an error strbuf.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper
From: Linus Arver @ 2024-01-12 7:42 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <d343585cb5e696f521c2ee1dd6c0f0c2d86de113.1702562879.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> When commit b236752a (Support remote archive from all smart transports,
> 2009-12-09) added "remote archive" support for "smart transports", it
> was for transport that supports the ".connect" method. The
> "connect_helper()" function protected itself from getting called for a
> transport without the method before calling process_connect_service(),
OK.
> which did not work with such a transport.
How about 'which only worked with the ".connect" method.' ?
>
> Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
> 2018-03-15) added a way for a transport without the ".connect" method
> to establish a "stateless" connection in protocol-v2, which
s/which/where
> process_connect_service() was taught to handle the "stateless"
> connection,
I think using 'the ".stateless_connect" method' is more consistent with
the rest of this text.
> making the old safety valve in its caller that insisted
> that ".connect" method must be defined too strict, and forgot to loosen
> it.
I think just "...making the old protection too strict. But edc9caf7
forgot to adjust this protection accordingly." is simpler to read.
> Remove the restriction in the "connect_helper()" function and give the
> function "process_connect_service()" the opportunity to establish a
> connection using ".connect" or ".stateless_connect" for protocol v2. So
> we can connect with a stateless-rpc and do something useful. E.g., in a
> later commit, implements remote archive for a repository over HTTP
> protocol.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> transport-helper.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>
> /* Get_helper so connect is inited. */
> get_helper(transport);
> - if (!data->connect)
> - die(_("operation not supported by protocol"));
Should we still terminate early here if both data->connect and
data->stateless_connect are not truthy? It would save a few CPU cycles,
but even better, remain true to the the original intent of the code.
Maybe there was a really good reason to terminate early here that we're
not aware of?
But also, what about the case where both are enabled? Should we print an
error message? (Maybe this concern is outside the scope of this series?)
> if (!process_connect_service(transport, name, exec))
> die(_("can't connect to subservice %s"), name);
> --
> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
^ permalink raw reply
* Re: [PATCH] strvec: use correct member name in comments
From: Jeff King @ 2024-01-12 7:41 UTC (permalink / raw)
To: Linus Arver via GitGitGadget; +Cc: git, Linus Arver
In-Reply-To: <pull.1640.git.1705043195997.gitgitgadget@gmail.com>
On Fri, Jan 12, 2024 at 07:06:35AM +0000, Linus Arver via GitGitGadget wrote:
> From: Linus Arver <linusa@google.com>
>
> In d70a9eb611 (strvec: rename struct fields, 2020-07-28), we renamed the
> "argv" member to "v". In the same patch we also did the following rename
> in strvec.c:
>
> -void strvec_pushv(struct strvec *array, const char **argv)
> +void strvec_pushv(struct strvec *array, const char **items)
>
> and it appears that this s/argv/items operation was erroneously applied
> to strvec.h.
>
> Rename "items" to "v".
Good catch. The source of the problem is that the patch originally used
"items" in the struct, too, but after review we settled on the more
concise "v". I'd almost certainly have then flipped the name in the
struct definition and relied on the compiler to help find the fallout.
But of course it doesn't look in comments. :)
As you note, we still call use "items" for the vector passed in to
pushv. I think that is OK, and there is no real need to use the terse
"v" there (it is also purely internal; the declaration in strvec.h does
not name it at all).
So this patch looks great to me. Thanks!
-Peff
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Johannes Sixt @ 2024-01-12 7:35 UTC (permalink / raw)
To: Michael Lohmann; +Cc: phillip.wood123, git
In-Reply-To: <20240111233311.64734-1-mi.al.lohmann@gmail.com>
Am 12.01.24 um 00:33 schrieb Michael Lohmann:
> This extends the functionality of `git log --merge` to also work with
> conflicts for rebase, cherry pick and revert.
>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
> Hi everybody!
>
> When Phillip Wood gave me some nice hints on his workflow concerning
> conflicts [1] (we discussed if `--show-current-patch` would make sense
> for cherry pick/revert). When I learned about `git log --merge` I was a
> bit sad to discover that those do not exist for rebase/revert/cherry
> pick since they look really valuable for getting an understanding on
> what was changed. It is basically the counterpart to
> `git show ${ACTION}_HEAD` for understanding the source of the conflict.
>
> I am curious if also other people would be interested in having an easy
> way to get a log of only the relevant commits touching conflicting files
> for said actions.
>
> With this patch I think the functionality is there, just hijacking the
> `--merge` code - maybe an alias like `git log --conflict` or similar
> might be more descriptive now?
>
> What do you think about this idea? (Or am I maybe missing an easy way to
> achieve it with the existing code as well?)
Ha! Very nice patch. For comparison, have a look at my patch to achieve
the same that I never submitted (in particular, the author date):
https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
This implementation is more complete than mine. I like it.
>
> Michael
>
>
> [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/
>
> revision.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..2e5c00dabd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,
> }
> }
>
> +static char* get_action_head_name(struct object_id* oid)
> +{
> + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) {
> + return "MERGE_HEAD";
> + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) {
> + return "REBASE_HEAD";
> + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) {
> + return "CHERRY_PICK_HEAD";
> + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) {
> + return "REVERT_HEAD";
> + } else
> + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
> +}
> +
> static void prepare_show_merge(struct rev_info *revs)
> {
> struct commit_list *bases;
> struct commit *head, *other;
> struct object_id oid;
> const char **prune = NULL;
> + const char *action_head_name;
> int i, prune_num = 1; /* counting terminating NULL */
> struct index_state *istate = revs->repo->index;
>
> if (repo_get_oid(the_repository, "HEAD", &oid))
> die("--merge without HEAD?");
> head = lookup_commit_or_die(&oid, "HEAD");
> - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> - die("--merge without MERGE_HEAD?");
> - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> + action_head_name = get_action_head_name(&oid);
> + other = lookup_commit_or_die(&oid, action_head_name);
> add_pending_object(revs, &head->object, "HEAD");
> - add_pending_object(revs, &other->object, "MERGE_HEAD");
> + add_pending_object(revs, &other->object, action_head_name);
> bases = repo_get_merge_bases(the_repository, head, other);
> add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
> add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
^ permalink raw reply
* Re: Help: Trying to setup triangular workflow
From: Jeff King @ 2024-01-12 7:31 UTC (permalink / raw)
To: Matthew B. Gray; +Cc: git
In-Reply-To: <b59c59f6-29e1-4d67-bace-13adcc108454@app.fastmail.com>
On Fri, Jan 12, 2024 at 03:23:58PM +1300, Matthew B. Gray wrote:
> Here's what I get from running the documented example:
>
> λ git config push.default current
> λ git config remote.pushdefault myfork
> λ git switch -c mybranch origin/main
> λ git push
> * [new branch] mybranch -> mybranch
> branch 'mybranch' set up to track 'myfork/mybranch'.
This push step is rewriting your upstream config. Do you have
push.autoSetupRemote configured? In general you wouldn't want that for a
triangular flow.
Though I think it also is only supposed to kick in if there is no
tracking configured already. Why did the "git switch" invocation not set
up tracking itself? When I run those commands it does. Do you have
branch.autoSetupMerge turned off in your config?
-Peff
^ permalink raw reply
* Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action
From: Jeff King @ 2024-01-12 7:24 UTC (permalink / raw)
To: Michael Lohmann; +Cc: gitster, git, phillip.wood123, phillip.wood, wanja.hentze
In-Reply-To: <20240111200627.64199-1-mi.al.lohmann@gmail.com>
On Thu, Jan 11, 2024 at 09:06:27PM +0100, Michael Lohmann wrote:
> > I agree with you why NONE is called as such. If "revert" does not
> > take "--start" (I do not remember offhand), I would think it would
> > be better to follow suit.
> My point was that yes, it might be in sync with what the user passes in
> as arguments, but when I followed the code and saw lots of references to
> ACTION_NONE I was puzzled, since my intuition of that name was that
> _no action_ should be taken (which did not make sense to me).
Just my two cents as an observer who is very familiar with the idioms of
Git's codebase: it's common for us to use NONE here to mean "an action
has not been selected", which the code then translates to a default
action. So that's what I would have chosen.
But your way of seeing it also makes sense to me. I think I just find
the "START" name jarring because we do not use that word elsewhere to
describe the action. What if you called it ACTION_DEFAULT? Then it is
both the "default" value we give it, and also the default action (which
is not otherwise named in the code).
As far as the enum vs char thing, I do not have a strong opinion (though
I do tend to like enums myself). Here are a few minor style comments
(again that are idiomatic for our code base):
> +enum action {
> + ACTION_START = 0,
> + ACTION_CONTINUE,
> + ACTION_SKIP,
> + ACTION_ABORT,
> + ACTION_QUIT,
> +};
Explicitly setting ACTION_START to 0 is good (even though it is not
strictly necessary) because it makes it clear that we expect to use its
truthiness later. But later...
> @@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> "--edit", opts->edit > 0,
> NULL);
>
> - if (cmd) {
> - opts->revs = NULL;
> - } else {
> + if (cmd == ACTION_START) {
> struct setup_revision_opt s_r_opt;
> opts->revs = xmalloc(sizeof(*opts->revs));
> repo_init_revisions(the_repository, opts->revs, NULL);
> @@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> memset(&s_r_opt, 0, sizeof(s_r_opt));
> s_r_opt.assume_dashdash = 1;
> argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
> + } else {
> + opts->revs = NULL;
We do not take advantage of that. It is still OK to do "if (cmd)" with
the enum, and that's what I'd usually expect in our code base. There is
no need for this hunk at all (which also switches the order of the
conditional, which just seems like churn to me).
> @@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = {
> NULL
> };
>
> +static char* get_cmd_optionname(enum action cmd)
From CodingGuidelines:
When declaring pointers, the star sides with the variable name, i.e.
"char *string", not "char* string" or "char * string". This makes it
easier to understand code like "char *string, c;".
(Yes, I know there are arguments for the other way, too; but consistency
is the most important thing, I think).
> +{
> + switch (cmd) {
> + case ACTION_CONTINUE: return "--continue";
> + case ACTION_SKIP: return "--skip";
> + case ACTION_ABORT: return "--abort";
> + case ACTION_QUIT: return "--quit";
> + case ACTION_START: BUG("no commandline flag for ACTION_START");
> + }
> +}
I find this perfectly readable, and is likely the way I'd write it in a
personal project. But in this project I find we tend to stick to more
conventional formatting, like:
switch (cmd) {
case ACTION_CONTINUE:
return "--continue";
case ACTION_SKIP:
return "--skip";
...and so on...
> @@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> }
>
> /* Check for incompatible command line arguments */
> - if (cmd) {
> - char *this_operation;
> - if (cmd == 'q')
> - this_operation = "--quit";
> - else if (cmd == 'c')
> - this_operation = "--continue";
> - else if (cmd == 's')
> - this_operation = "--skip";
> - else {
> - assert(cmd == 'a');
> - this_operation = "--abort";
> - }
> -
> - verify_opt_compatible(me, this_operation,
> + if (cmd != ACTION_START)
Likewise here I'd probably leave this as "if (cmd)".
> [...]
Everything else looked pretty good to me.
-Peff
^ permalink raw reply
* [PATCH] strvec: use correct member name in comments
From: Linus Arver via GitGitGadget @ 2024-01-12 7:06 UTC (permalink / raw)
To: git; +Cc: Linus Arver, Linus Arver
From: Linus Arver <linusa@google.com>
In d70a9eb611 (strvec: rename struct fields, 2020-07-28), we renamed the
"argv" member to "v". In the same patch we also did the following rename
in strvec.c:
-void strvec_pushv(struct strvec *array, const char **argv)
+void strvec_pushv(struct strvec *array, const char **items)
and it appears that this s/argv/items operation was erroneously applied
to strvec.h.
Rename "items" to "v".
Signed-off-by: Linus Arver <linusa@google.com>
---
strvec: use correct member name in comments
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1640%2Flistx%2Ffix-strvec-typos-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1640/listx/fix-strvec-typos-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1640
strvec.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/strvec.h b/strvec.h
index 9f55c8766ba..4715d3e51f8 100644
--- a/strvec.h
+++ b/strvec.h
@@ -4,8 +4,8 @@
/**
* The strvec API allows one to dynamically build and store
* NULL-terminated arrays of strings. A strvec maintains the invariant that the
- * `items` member always points to a non-NULL array, and that the array is
- * always NULL-terminated at the element pointed to by `items[nr]`. This
+ * `v` member always points to a non-NULL array, and that the array is
+ * always NULL-terminated at the element pointed to by `v[nr]`. This
* makes the result suitable for passing to functions expecting to receive
* argv from main().
*
@@ -22,7 +22,7 @@ extern const char *empty_strvec[];
/**
* A single array. This should be initialized by assignment from
- * `STRVEC_INIT`, or by calling `strvec_init`. The `items`
+ * `STRVEC_INIT`, or by calling `strvec_init`. The `v`
* member contains the actual array; the `nr` member contains the
* number of elements in the array, not including the terminating
* NULL.
@@ -80,7 +80,7 @@ void strvec_split(struct strvec *, const char *);
void strvec_clear(struct strvec *);
/**
- * Disconnect the `items` member from the `strvec` struct and
+ * Disconnect the `v` member from the `strvec` struct and
* return it. The caller is responsible for freeing the memory used
* by the array, and by the strings it references. After detaching,
* the `strvec` is in a reinitialized state and can be pushed
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 2/2] t5541: remove lockfile creation
From: Jeff King @ 2024-01-12 7:03 UTC (permalink / raw)
To: Justin Tobler via GitGitGadget; +Cc: git, Patrick Steinhardt, Justin Tobler
In-Reply-To: <f953a668c6a7e0a57adcee77ceee2d578970065e.1705004670.git.gitgitgadget@gmail.com>
On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
> - # the new branch should not have been created upstream
> - test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> -
> - # upstream should still reflect atomic2, the last thing we pushed
> - # successfully
> - git rev-parse atomic2 >expected &&
> - # ...to other.
> - git -C "$d" rev-parse refs/heads/other >actual &&
> - test_cmp expected actual &&
> -
> - # the new branch should not have been created upstream
> + # The atomic and other branches should be created upstream.
> test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> + test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
This last comment should say "should not be created", I think?
Other than that, both patches look good to me.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] t1401: generalize reference locking
From: Jeff King @ 2024-01-12 7:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Justin Tobler via GitGitGadget, git, Justin Tobler
In-Reply-To: <ZZ_MPK2huH2j6CGd@tanuki>
On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:
> > (note that you get a different error message if the refs are packed,
> > since there we can notice the d/f conflict manually).
>
> If all we care for is the exit code then this would work for the
> reftable backend, too:
>
> ```
> $ git init --ref-format=reftable repo
> Initialized empty Git repository in /tmp/repo/.git/
> $ cd repo/
> $ git commit --allow-empty --message message
> [main (root-commit) c2512d3] x
> $ git symbolic-ref refs/heads refs/heads/foo
> $ echo $?
> 1
> ```
Yep, exactly. That should work for both and cover what the test was
originally trying to do.
> A bit unfortunate that there is no proper error message in that case,
> but that is a different topic.
Yeah, I would call that an outright bug. It does not have to be part of
this patch, but is worth fixing (and testing). I suspect it's not going
to be the only place with this problem. Most of the files-backend ref
code is very happy to spew to stdout using error(), but the reftable
code, having been written from a more lib-conscious perspective,
probably doesn't.
The obvious quick fix is to sprinkle more error() into the reftable
code. But in the longer term, I think the right direction is that the
ref code should accept an error strbuf or similar mechanism to propagate
human-readable error test to the caller.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] t7450: test submodule urls
From: Jeff King @ 2024-01-12 6:57 UTC (permalink / raw)
To: Victoria Dye; +Cc: Victoria Dye via GitGitGadget, git
In-Reply-To: <d852ad72-32c4-4b0f-8f34-e8b38b7f71ad@github.com>
On Thu, Jan 11, 2024 at 08:54:47AM -0800, Victoria Dye wrote:
> > All of this is inherited from the existing check_name() code, which I
> > think has all of the same bugs. The test scripts all just use the stdin
> > mode, so they don't notice. It's not too hard to fix, but maybe it's
> > worth just ripping out the unreachable code.
>
> Thanks for pointing out those issues, I think removing the command line
> input mode is the way to go. The description of the 'check_name()' mentions
> that the stdin mode was "primarily intended for testing". But as 85321a346b5
> (submodule--helper: move "check-name" to a test-tool, 2022-09-01) pointed
> out, 'check_name()' was never used outside of tests anyway, so whatever use
> case was imagined for the command line mode never seemed to have existed.
>
> Combine that with the fact that the command line mode is so different from
> the stdin mode (non-zero exit code for invalid names, prints nothing vs.
> zero exit code, prints valid names), there don't seem to be any real
> downsides to removing the unused code.
That sounds like a good plan to me. :)
-Peff
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Jeff King @ 2024-01-12 6:56 UTC (permalink / raw)
To: Stefan Haller; +Cc: Junio C Hamano, git
In-Reply-To: <c8ad96bc-0180-42f4-b559-20b475098eca@haller-berlin.de>
On Thu, Jan 11, 2024 at 02:28:51PM +0100, Stefan Haller wrote:
> On 10.01.24 12:08, Jeff King wrote:
> > On Mon, Jan 08, 2024 at 10:20:00AM -0800, Junio C Hamano wrote:
> >
> >> An obvious alternative is to have .lazygit directory next to .git directory
> >> which would give you a bigger separation, which can cut both ways.
> >
> > Just to spell out one of those ways: unlike ".git", we will happily
> > check out ".lazygit" from an untrusted remote repository. That may be a
> > feature if you want to be able to share project-specific config, or it
> > might be a terrible security vulnerability if lazygit config files can
> > trigger arbitrary code execution.
>
> Unless you don't version it and add it to .gitignore instead, which (I
> suppose) is what most people do with their .vscode/settings.json, for
> example.
A .gitignore will help with people accidentally adding their .lazygit
directory. What I meant, though, was somebody _intentionally_ creating a
malicious repository that would then execute arbitrary code when the
victim cloned it. We prevent that from happening with .git/config
because there's special handling that refuses to check out the name
".git" (or other filesystem-equivalent names). But ".lazygit" would not
have that same protection.
-Peff
^ 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