* RE: [PATCH v3] build: support z/OS (OS/390).
From: rsbecker @ 2024-03-01 18:25 UTC (permalink / raw)
To: 'Junio C Hamano', 'Haritha D'
Cc: git, 'Kristoffer Haugsbakk'
In-Reply-To: <xmqqbk7x6cox.fsf@gitster.g>
On Friday, March 1, 2024 1:15 PM, Junio C Hamano wrote:
>Haritha D <Harithamma.D@ibm.com> writes:
>
>> The win test(7) test case failed stating the reason as "The Operation
>> cancelled". I saw that it failed after 5 hours 59 minutes of running
>> the test cases(build). How do I handle this?
>
>Those "win test (n)", at least some of them, seem to have been somewhat
flaky [*1*]. If you are certain you did not break them with
>your change, you do not have to fix them yourself.
>
>I am a wrong person to ask how the test failure that may [*2*] block GGG
submission can be circumvented, as I am not involved in that
>machinery at all.
>
>Thanks.
>
>
>[Footnotes]
>
>*1* Also I've seen osx-clang job time-out from time to time, without
> failing any specific test. Re-running failed jobs from the menu
> often make them pass, which is why I said "somewhat flaky".
>
>*2* I do not even know if GGG refuses to submit a series with a test
> failure, let alone if it allows to override such a safety if
> exists.
Which tests have been hanging on S390? We have occasional hangs on NonStop
that end up being attributed to our CI build system not supplying pipes
properly to git. It would be interesting if the same tests are having issues
on different platforms.
--Randall
^ permalink raw reply
* Re: [PATCH v3] build: support z/OS (OS/390).
From: Junio C Hamano @ 2024-03-01 18:15 UTC (permalink / raw)
To: Haritha D; +Cc: git@vger.kernel.org, Kristoffer Haugsbakk
In-Reply-To: <B3775704-6FCE-4994-8682-309D521FD2CB@ibm.com>
Haritha D <Harithamma.D@ibm.com> writes:
> The win test(7) test case failed stating the reason as "The
> Operation cancelled". I saw that it failed after 5 hours 59
> minutes of running the test cases(build). How do I handle this?
Those "win test (n)", at least some of them, seem to have been
somewhat flaky [*1*]. If you are certain you did not break them
with your change, you do not have to fix them yourself.
I am a wrong person to ask how the test failure that may [*2*] block
GGG submission can be circumvented, as I am not involved in that
machinery at all.
Thanks.
[Footnotes]
*1* Also I've seen osx-clang job time-out from time to time, without
failing any specific test. Re-running failed jobs from the menu
often make them pass, which is why I said "somewhat flaky".
*2* I do not even know if GGG refuses to submit a series with a test
failure, let alone if it allows to override such a safety if
exists.
^ permalink raw reply
* Re: [PATCH] branch: advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-01 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1q8t7roc.fsf@gitster.g>
Hi
> This will give the message with "hint:" prefix, which is a good
> starting point.
>
> The message is given unconditionally, without any way to turn it
> off. For those who ...
>
>> git-branch(1) will error out if you give it a bad ref name. But the user
>> might not understand why or what part of the name is illegal.
>
> ... do not understand why, it is helpful, but once they learned, it
> is one extra line of unwanted text. If you want to give it a way to
> squelch, see the comment before where enum advice_type is declared
> in advice.h header file.
I thought of doing that, but I reckoned that people who have a good
intuition for the ref syntax would not get this error enough to want to
turn if off.
I’ll add a squelch option in the next version.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Junio C Hamano @ 2024-03-01 18:07 UTC (permalink / raw)
To: Jean-Noël Avila; +Cc: Sergey Organov, git
In-Reply-To: <51a196c0-ea57-4ec5-99ea-c3f09cd90962@gmail.com>
Jean-Noël Avila <avila.jn@gmail.com> writes:
>> + /* Dry run won't remove anything, so requiring force makes no sense */
>> + if(dry_run)
>> + require_force = 0;
Style. "if (dry_run)".
Getting rid of "config_set", which was an extra variable that kept
track of where "force" came from, does make the logic cleaner, I
guess. What we want to happen is that one of -i/-n/-f is required
when clean.requireForce is *not* unset (i.e. 0 <= require_force).
>> + if (!force && !interactive) {
The require-force takes effect only when neither force or
interactive is given, so the new code structure puts the above
obvious conditional around "do we complain due to requireForce?"
logic. Sensible.
>> + if (require_force > 0)
>> + die(_("clean.requireForce set to true and neither -f, nor -i given; "
>> + "refusing to clean"));
If it is explicitly set, we get this message. And ...
>> + else if (require_force < 0)
>> + die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
>> "refusing to clean"));
... if it is set due to default (in other words, if it is not unset), we
get this message.
As you said, I do not think it matters too much either way to the
end-users where the truth setting of clean.requireForce came from,
either due to the default or the user explicitly configuring. So
unifying to a single message may be helpful to both readers and
translators.
clean.requireForce is true; unless interactive, -f is required
might be a bit shorter and more to the point.
> The last two cases can be coalesced into a single case (the last one),
> because the difference in the messages does not bring more information
> to the user.
Yeah.
Thanks.
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Junio C Hamano @ 2024-03-01 18:07 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Sergey Organov, git, Jean-Noël Avila
In-Reply-To: <86ce3c89-4a58-42bf-a31a-96fa6b74e937@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote:
>>> Please use backticks for options, configuration and environment names:
>>> `clean.requireForce`
>>
>> I did consider this. However, existing text already has exactly this one
>> unquoted, so I just did the same. Hopefully it will be fixed altogether
>> later, or are you positive I better resend the patch with quotes?
>
> Sometimes I see widespread changes (like formatting many files) get
> rejected because it is considered _churn_. Not fixing this in this
> series and then maybe someone else fixing it later seems like churn as
> well. Isn’t it better to fix it while you are changing the text?
Any one of these is fine:
(1) add the new paragraph with mark-up consistent with existing
text (which is what Sergey did).
(2) add the new paragraph with correct mark-up, making the document
less consistent overall.
(3) have one patch to fix broken mark-up of existing text, followed
by another patch to add the new paragraph with correct mark-up.
If you take one of the first two, it would be a very good idea to
have a comment in the proposed log message to note the need for
later clean-up. Without being written down anywhere, your discovery
and the brain cycles you spent while deciding what to do will be
wasted, which is not what you want.
Thanks, both.
^ permalink raw reply
* Re: [PATCH] branch: advise about ref syntax rules
From: Junio C Hamano @ 2024-03-01 18:06 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <d275d1d179b90592ddd7b5da2ae4573b3f7a37b7.1709307442.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> Notes (series):
> Hopefully I am using `advice.h` correctly here.
Let's see.
> - if (strbuf_check_branch_ref(ref, name))
> - die(_("'%s' is not a valid branch name"), name);
> + if (strbuf_check_branch_ref(ref, name)) {
> + error(_("'%s' is not a valid branch name"), name);
> + advise(_("See `man git check-ref-format`"));
> + exit(1);
> + }
This will give the message with "hint:" prefix, which is a good
starting point.
The message is given unconditionally, without any way to turn it
off. For those who ...
> git-branch(1) will error out if you give it a bad ref name. But the user
> might not understand why or what part of the name is illegal.
... do not understand why, it is helpful, but once they learned, it
is one extra line of unwanted text. If you want to give it a way to
squelch, see the comment before where enum advice_type is declared
in advice.h header file. The callsites would become something like
advise_if_enabled(ADVICE_VALID_REF_NAME,
_("See `man git check-ref-format` for valid refname syntax."));
Another thing is that rewriting die() into error() + advice() +
manual exit() is an anti-pattern these days.
int code = die_message(_("'%s' is not a valid branch name"), name);
advice_if_enabled(...); /* see above */
exit(code);
In the same source file, you will find an existing example to mimic.
Thanks.
^ permalink raw reply
* [PATCH] gitcli: drop mention of “non-dashed form”
From: Kristoffer Haugsbakk @ 2024-03-01 18:05 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Johannes Schindelin
Git builtins used to be called like e.g. `git-commit`, not `git
commit` (*dashed form* and *non-dashed form*, respectively). The dashed
form was deprecated in version 1.5.4 (2006). Now only a few commands
have an alternative dashed form when `SKIP_DASHED_BUILT_INS` is
active.[1]
The mention here is from 2f7ee089dff (parse-options: Add a gitcli(5) man
page., 2007-12-13), back when the deprecation was relatively
recent. These days though it seems like an irrelevant point to make to
budding CLI scripters—you don’t have to warn against a style that
probably doesn’t even work on their git(1) installation.
† 1: 179227d6e21 (Optionally skip linking/copying the built-ins,
2020-09-21)
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
Now:
```
$ git grep --files-with-matches 'dashed form'
Documentation/RelNotes/1.5.4.txt
Documentation/RelNotes/2.16.0.txt
Makefile
t/README
```
For 179227d6e21 (Optionally skip linking/copying the built-ins,
2020-09-21):
(sorry for the resend; I sent to my test address first)
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Documentation/gitcli.txt | 3 ---
1 file changed, 3 deletions(-)
diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
index e5fac943227..7c709324ba9 100644
--- a/Documentation/gitcli.txt
+++ b/Documentation/gitcli.txt
@@ -81,9 +81,6 @@ you will.
Here are the rules regarding the "flags" that you should follow when you are
scripting Git:
- * It's preferred to use the non-dashed form of Git commands, which means that
- you should prefer `git foo` to `git-foo`.
-
* Splitting short options to separate words (prefer `git foo -a -b`
to `git foo -ab`, the latter may not even work).
--
2.44.0.106.g650c15c891b
^ permalink raw reply related
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Junio C Hamano @ 2024-03-01 16:21 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZQAxGFDrBmq0+Q77YcfT4hBivRu9oOGZF-r81F49P+00Q@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>> * kn/for-all-refs (2024-02-23) 6 commits
>> (merged to 'next' on 2024-02-27 at aef2406cca)
>> + for-each-ref: add new option to include root refs
>> + ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
>> + refs: introduce `refs_for_each_include_root_refs()`
>> + refs: extract out `loose_fill_ref_dir_regular_file()`
>> + refs: introduce `is_pseudoref()` and `is_headref()`
>> + Merge branch 'ps/reftable-backend' into kn/for-all-refs
>>
>> "git for-each-ref" filters its output with prefixes given from the
>> command line, but it did not honor an empty string to mean "pass
>> everything", which has been corrected.
>>
>> Will merge to 'master'.
>> source: <20240223100112.44127-1-karthik.188@gmail.com>
>
> Thanks, the description here needs to be modified since we moved from
> the `an empty string to mean "pass everything"` to providing the
> `--include-root-refs` option.
Good point. I just updated the draft for the next issue of
'whats-cooking.txt' to read like this:
"git for-each-ref" learned "--include-root-refs" option to show
even the stuff outside the 'refs/' hierarchy.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 03/11] commit-reach(repo_in_merge_bases_many): report missing commits
From: Junio C Hamano @ 2024-03-01 16:18 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git, Patrick Steinhardt,
Dirk Gouders, Johannes Schindelin
In-Reply-To: <20240301065805.GB2680308@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 28, 2024 at 09:44:09AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> @@ -1402,6 +1436,8 @@ static int merge_mode_and_contents(struct merge_options *opt,
>> &o->oid,
>> &a->oid,
>> &b->oid);
>> + if (result->clean < 0)
>> + return -1;
>
> Sorry, I accidentally commented on v2 of your series a moment ago,
> rather than the most recent version. But this hunk was untouched between
> the two, so the comment still applies:
>
> https://lore.kernel.org/git/20240301065647.GA2680308@coredump.intra.peff.net/
>
> -Peff
Thanks for spotting.
The topic now is in 'next' so let's fix it incrementally while I'll
hold it there. If all of us thought it has seen enough eyeballs and
is good enough for 'next', yet we later find there was something we
all missed, that is worth a separate explanation, e.g., "The primary
motivation behind the series is still good, but for such and such
reasons we missed this case we are fixing."
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Junio C Hamano @ 2024-03-01 16:16 UTC (permalink / raw)
To: Linus Arver; +Cc: git
In-Reply-To: <xmqq1q8ubtg6.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> Doh, please wait for my v6 reroll (will send to the list in the next
>> half hour) to clean up the commit messages. Thanks.
>
> If we had communication gap and the topic was prematurely merged and
> was caught within a day or two, I would be a bit more sympathetic,
> but given that this was merged more than a week ago, that's totally
> unacceptable.
>
> If you have improvements, please do so as incremental patches on
> top. I'll hold the topic in 'next' until we are ready.
Well, I changed my mind. As we haven't rewound the tip of 'next'
post release, let's eject what is in 'next' and queue the latest,
pretending that the earlier round weren't in 'next' at all.
Thanks.
^ permalink raw reply
* [PATCH] branch: advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-01 15:38 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
git-branch(1) will error out if you give it a bad ref name. But the user
might not understand why or what part of the name is illegal. The man
page for git-check-ref-format(1) contains these rules. Let’s advise
about it since that is not a command that you just happen upon.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
Hopefully I am using `advice.h` correctly here.
§ git-replace(1)
I did not add a hint for a similar message in `builtin/replace.c`.
`builtin/replace.c` has an error message in `check_ref_valid` for an
invalid ref name:
```
return error(_("'%s' is not a valid ref name"), ref->buf);
```
But there doesn’t seem to be a point to placing a hint here.
The preceding calls to `repo_get_oid` will catch both missing refs and
existing refs with invalid names:
```
if (repo_get_oid(r, refname, &object))
return error(_("failed to resolve '%s' as a valid ref"), refname);
```
Like for example this:
```
$ printf $(git rev-parse @~) > .git/refs/heads/hello..goodbye
$ git replace @ hello..goodbye
error: failed to resolve 'hello..goodbye' as a valid ref
[…]
$ git replace @ non-existing
error: failed to resolve 'non-existing' as a valid ref
```
§ Alternatives (to this change)
While working on this I also thought that it might be nice to have a
man page `gitrefsyntax`. That one could use a lot of the content from
`man git check-ref-format` verbatim. Then the hint could point towards
that man page. And it seems that AsciiDoc supports _includes_ which
means that the rules don’t have to be duplicated between the two man
pages.
branch.c | 7 +++++--
builtin/branch.c | 7 +++++--
t/t3200-branch.sh | 10 ++++++++++
3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/branch.c b/branch.c
index 6719a181bd1..1386918c60e 100644
--- a/branch.c
+++ b/branch.c
@@ -370,8 +370,11 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
*/
int validate_branchname(const char *name, struct strbuf *ref)
{
- if (strbuf_check_branch_ref(ref, name))
- die(_("'%s' is not a valid branch name"), name);
+ if (strbuf_check_branch_ref(ref, name)) {
+ error(_("'%s' is not a valid branch name"), name);
+ advise(_("See `man git check-ref-format`"));
+ exit(1);
+ }
return ref_exists(ref->buf);
}
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5fb..fa81e359157 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,8 +576,11 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
*/
if (ref_exists(oldref.buf))
recovery = 1;
- else
- die(_("invalid branch name: '%s'"), oldname);
+ else {
+ error(_("invalid branch name: '%s'"), oldname);
+ advise(_("See `man git check-ref-format`"));
+ exit(1);
+ }
}
for (int i = 0; worktrees[i]; i++) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..9400a8baa35 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1725,4 +1725,14 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
test_cmp_config "" --default "" branch.foo5.merge
'
+cat <<\EOF >expect
+error: 'foo..bar' is not a valid branch name
+hint: See `man git check-ref-format`
+EOF
+
+test_expect_success 'errors if given a bad branch name' '
+ test_must_fail git branch foo..bar >actual 2>&1 &&
+ test_cmp expect actual
+'
+
test_done
--
2.44.0.106.g650c15c891b
^ permalink raw reply related
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Kristoffer Haugsbakk @ 2024-03-01 15:29 UTC (permalink / raw)
To: Sergey Organov; +Cc: Junio C Hamano, git, Jean-Noël Avila
In-Reply-To: <87frxam35f.fsf@osv.gnss.ru>
On Fri, Mar 1, 2024, at 15:34, Sergey Organov wrote:
>> Please use backticks for options, configuration and environment names:
>> `clean.requireForce`
>
> I did consider this. However, existing text already has exactly this one
> unquoted, so I just did the same. Hopefully it will be fixed altogether
> later, or are you positive I better resend the patch with quotes?
Sometimes I see widespread changes (like formatting many files) get
rejected because it is considered _churn_. Not fixing this in this
series and then maybe someone else fixing it later seems like churn as
well. Isn’t it better to fix it while you are changing the text?
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3] build: support z/OS (OS/390).
From: Ghanshyam Thakkar @ 2024-03-01 14:39 UTC (permalink / raw)
To: Haritha D, git@vger.kernel.org; +Cc: Kristoffer Haugsbakk, Junio C Hamano
In-Reply-To: <B3775704-6FCE-4994-8682-309D521FD2CB@ibm.com>
On Fri Mar 1, 2024 at 2:39 PM IST, Haritha D wrote:
> Hi Team,
>
> The win test(7) test case failed stating the reason as "The Operation cancelled". I saw that it failed after 5 hours 59 minutes of running the test cases(build). How do I handle this?
>
> Please suggest.
Hey,
This seems to be a recurring issue, as this happens to me majority
of the time. Seems as though the vm just sits idle after finishing tests
as 360 minutes is the timeout for github acitons.
Usually, rerunning the failed tests from the actions tab works for me.
Thanks.
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-01 14:34 UTC (permalink / raw)
To: Jean-Noël Avila; +Cc: Junio C Hamano, git
In-Reply-To: <51a196c0-ea57-4ec5-99ea-c3f09cd90962@gmail.com>
Jean-Noël Avila <avila.jn@gmail.com> writes:
> Putting my documentation/translator hat:
>
> Le 29/02/2024 à 20:07, Sergey Organov a écrit :
>> What -n actually does in addition to its documented behavior is
>> ignoring of configuration variable clean.requireForce, that makes
>> sense provided -n prevents files removal anyway.
>>
>> So, first, document this in the manual, and then modify implementation
>> to make this more explicit in the code.
>>
>> Improved implementation also stops to share single internal variable
>> 'force' between command-line -f option and configuration variable
>> clean.requireForce, resulting in more clear logic.
>>
>> The error messages now do not mention -n as well, as it seems
>> unnecessary and does not reflect clarified implementation.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> Documentation/git-clean.txt | 2 ++
>> builtin/clean.c | 26 +++++++++++++-------------
>> 2 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
>> index 69331e3f05a1..662eebb85207 100644
>> --- a/Documentation/git-clean.txt
>> +++ b/Documentation/git-clean.txt
>> @@ -49,6 +49,8 @@ OPTIONS
>> -n::
>> --dry-run::
>> Don't actually remove anything, just show what would be done.
>> + Configuration variable clean.requireForce is ignored, as
>> + nothing will be deleted anyway.
>
> Please use backticks for options, configuration and environment names:
> `clean.requireForce`
I did consider this. However, existing text already has exactly this one
unquoted, so I just did the same. Hopefully it will be fixed altogether
later, or are you positive I better resend the patch with quotes?
>>
>> -q::
>> --quiet::
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d90766cad3a0..fcc50d08ee9b 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -25,7 +25,7 @@
>> #include "help.h"
>> #include "prompt.h"
>>
>> -static int force = -1; /* unset */
>> +static int require_force = -1; /* unset */
>> static int interactive;
>> static struct string_list del_list = STRING_LIST_INIT_DUP;
>> static unsigned int colopts;
>> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
>> }
>>
>> if (!strcmp(var, "clean.requireforce")) {
>> - force = !git_config_bool(var, value);
>> + require_force = git_config_bool(var, value);
>> return 0;
>> }
>>
>> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>> {
>> int i, res;
>> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
>> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
>> + int ignored_only = 0, force = 0, errors = 0, gone = 1;
>> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>> struct strbuf abs_path = STRBUF_INIT;
>> struct dir_struct dir = DIR_INIT;
>> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>> };
>>
>> git_config(git_clean_config, NULL);
>> - if (force < 0)
>> - force = 0;
>> - else
>> - config_set = 1;
>>
>> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>> 0);
>>
>> - if (!interactive && !dry_run && !force) {
>> - if (config_set)
>> - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
>> + /* Dry run won't remove anything, so requiring force makes no sense */
>> + if(dry_run)
>> + require_force = 0;
>> +
>> + if (!force && !interactive) {
>> + if (require_force > 0)
>> + die(_("clean.requireForce set to true and neither -f, nor -i given; "
>> + "refusing to clean"));
>> + else if (require_force < 0)
>> + die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
>> "refusing to clean"));
>> - else
>> - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
>> - " refusing to clean"));
>> }
>>
>
> The last two cases can be coalesced into a single case (the last one),
> because the difference in the messages does not bring more information
> to the user.
Did you misread the patch? There are only 2 cases here, the last (third)
one is marked with '-' (removed). Too easy to misread this, I'd say. New
code is:
if (require_force > 0)
die(_("clean.requireForce set to true and neither -f, nor -i given; "
"refusing to clean"));
else if (require_force < 0)
die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
and is basically unchanged from the original, except reference to '-n' has been
removed. Btw, is now comma needed after -f, and isn't it better to
substitute ':' for ';'?
Thank you for review!
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Jean-Noël Avila @ 2024-03-01 13:20 UTC (permalink / raw)
To: Sergey Organov, Junio C Hamano; +Cc: git
In-Reply-To: <875xy76qe1.fsf@osv.gnss.ru>
Putting my documentation/translator hat:
Le 29/02/2024 à 20:07, Sergey Organov a écrit :
> What -n actually does in addition to its documented behavior is
> ignoring of configuration variable clean.requireForce, that makes
> sense provided -n prevents files removal anyway.
>
> So, first, document this in the manual, and then modify implementation
> to make this more explicit in the code.
>
> Improved implementation also stops to share single internal variable
> 'force' between command-line -f option and configuration variable
> clean.requireForce, resulting in more clear logic.
>
> The error messages now do not mention -n as well, as it seems
> unnecessary and does not reflect clarified implementation.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> Documentation/git-clean.txt | 2 ++
> builtin/clean.c | 26 +++++++++++++-------------
> 2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
> index 69331e3f05a1..662eebb85207 100644
> --- a/Documentation/git-clean.txt
> +++ b/Documentation/git-clean.txt
> @@ -49,6 +49,8 @@ OPTIONS
> -n::
> --dry-run::
> Don't actually remove anything, just show what would be done.
> + Configuration variable clean.requireForce is ignored, as
> + nothing will be deleted anyway.
Please use backticks for options, configuration and environment names:
`clean.requireForce`
>
> -q::
> --quiet::
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d90766cad3a0..fcc50d08ee9b 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -25,7 +25,7 @@
> #include "help.h"
> #include "prompt.h"
>
> -static int force = -1; /* unset */
> +static int require_force = -1; /* unset */
> static int interactive;
> static struct string_list del_list = STRING_LIST_INIT_DUP;
> static unsigned int colopts;
> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "clean.requireforce")) {
> - force = !git_config_bool(var, value);
> + require_force = git_config_bool(var, value);
> return 0;
> }
>
> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> {
> int i, res;
> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
> + int ignored_only = 0, force = 0, errors = 0, gone = 1;
> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
> struct strbuf abs_path = STRBUF_INIT;
> struct dir_struct dir = DIR_INIT;
> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> };
>
> git_config(git_clean_config, NULL);
> - if (force < 0)
> - force = 0;
> - else
> - config_set = 1;
>
> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
> 0);
>
> - if (!interactive && !dry_run && !force) {
> - if (config_set)
> - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
> + /* Dry run won't remove anything, so requiring force makes no sense */
> + if(dry_run)
> + require_force = 0;
> +
> + if (!force && !interactive) {
> + if (require_force > 0)
> + die(_("clean.requireForce set to true and neither -f, nor -i given; "
> + "refusing to clean"));
> + else if (require_force < 0)
> + die(_("clean.requireForce defaults to true and neither -f, nor -i given; "
> "refusing to clean"));
> - else
> - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
> - " refusing to clean"));
> }
>
The last two cases can be coalesced into a single case (the last one),
because the difference in the messages does not bring more information
to the user.
> if (force > 1)
>
> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
^ permalink raw reply
* [PATCH v3 1/1] [PATCH] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-01 13:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, shejialuo
In-Reply-To: <20240301130334.135773-1-shejialuo@gmail.com>
test -(e|f) does not provide a nice error message when we hit test
failures, so use test_path_exists, test_path_is_dir instead.
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 62de819a44..3b038c338f 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
test_expect_success 'basic clone' '
test ! -d trunk &&
git svn clone "$svnrepo"/project/trunk &&
- test -d trunk/.git/svn &&
- test -e trunk/foo &&
+ test_path_is_dir trunk/.git/svn &&
+ test_path_exists trunk/foo &&
rm -rf trunk
'
test_expect_success 'clone to target directory' '
test ! -d target &&
git svn clone "$svnrepo"/project/trunk target &&
- test -d target/.git/svn &&
- test -e target/foo &&
+ test_path_is_dir target/.git/svn &&
+ test_path_exists target/foo &&
rm -rf target
'
test_expect_success 'clone with --stdlayout' '
test ! -d project &&
git svn clone -s "$svnrepo"/project &&
- test -d project/.git/svn &&
- test -e project/foo &&
+ test_path_is_dir project/.git/svn &&
+ test_path_exists project/foo &&
rm -rf project
'
test_expect_success 'clone to target directory with --stdlayout' '
test ! -d target &&
git svn clone -s "$svnrepo"/project target &&
- test -d target/.git/svn &&
- test -e target/foo &&
+ test_path_is_dir target/.git/svn &&
+ test_path_exists target/foo &&
rm -rf target
'
--
2.44.0
^ permalink raw reply related
* [PATCH v3 0/1] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-01 13:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Junio C Hamano, shejialuo
In-Reply-To: <20240301034606.69673-1-shejialuo@gmail.com>
As discussed in v2, it is improper to use ! test_path_is_dir to replace
the test ! -f. This patch reverts the code.
shejialuo (1):
t9117: prefer test_path_* helper functions
t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
2.44.0
^ permalink raw reply
* Re: [PATCH 1/1] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-01 11:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine
In-Reply-To: <xmqqwmqm8rmr.fsf@gitster.g>
Thanks for your wonderful comments. I have known that semantics is
important not only the functionality. I will send a new patch at now.
^ permalink raw reply
* Re: [PATCH 1/1] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-01 11:29 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano
In-Reply-To: <CAPig+cRfO8t1tdCL6MB4b9XopF3HkZ==hU83AFZ38b-2zsXDjQ@mail.gmail.com>
Thanks for your comment.
> Although you make the test pass by using `!` to invert the result of
> `test_path_is_dir`, the complaint will nevertheless get lodged, and
> may very well be confusing for anyone scrutinizing the output of the
> tests when running the script with `-v` or `-x`.
I have run the script with `-v`, I have got the following result:
Directory trunk doesn't exist
I come to realisize the fault with your dedicated comments. An assertion
is an assertion.
And I am impressed by the following idea:
> Rather than asserting some initial condition at the start of the test,
> it is more common and more robust simply to _ensure_ that the desired
> initial condition holds. So, for instance, instead of asserting `test
> ! -d target`, modern practice is to ensure that `target` doesn't
> exist. Thus:
>
> test_expect_success 'clone to target directory' '
> rm -rf target &&
> git svn clone "$svnrepo"/project/trunk target &&
> ...
>
> is a more robust implementation. This also addresses the problem that
> the `rm -rf target` at the very end of each test won't be executed if
> any command earlier in the test fails (due to the short-circuiting
> behavior of the &&-operator).
The command `rm -rf target` ensures an exit status of 0 regardless of
whether the `target` exists. Thus the code will elegant make sure the
initial condition holds. I think I could add a patch to clean the code.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #09; Tue, 27)
From: Karthik Nayak @ 2024-03-01 10:54 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqjzmpm9b8.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 928 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> * kn/for-all-refs (2024-02-23) 6 commits
> (merged to 'next' on 2024-02-27 at aef2406cca)
> + for-each-ref: add new option to include root refs
> + ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
> + refs: introduce `refs_for_each_include_root_refs()`
> + refs: extract out `loose_fill_ref_dir_regular_file()`
> + refs: introduce `is_pseudoref()` and `is_headref()`
> + Merge branch 'ps/reftable-backend' into kn/for-all-refs
>
> "git for-each-ref" filters its output with prefixes given from the
> command line, but it did not honor an empty string to mean "pass
> everything", which has been corrected.
>
> Will merge to 'master'.
> source: <20240223100112.44127-1-karthik.188@gmail.com>
Thanks, the description here needs to be modified since we moved from
the `an empty string to mean "pass everything"` to providing the
`--include-root-refs` option.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* RE: [PATCH v3] build: support z/OS (OS/390).
From: Haritha D @ 2024-03-01 9:09 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Kristoffer Haugsbakk, Junio C Hamano
In-Reply-To: <xmqq7circevo.fsf@gitster.g>
Hi Team,
The win test(7) test case failed stating the reason as "The Operation cancelled". I saw that it failed after 5 hours 59 minutes of running the test cases(build). How do I handle this?
Please suggest.
Best regards
Haritha
On 26/02/24, 11:15 PM, "Junio C Hamano" <gitster@pobox.com <mailto:gitster@pobox.com>> wrote:
"Haritha via GitGitGadget" <gitgitgadget@gmail.com <mailto:gitgitgadget@gmail.com>> writes:
> From: Haritha D <harithamma.d@ibm.com <mailto:harithamma.d@ibm.com>>
>
> Since the z/OS linker does not support searching dynamic libraries,
> and the current setting of CC_LD_DYNPATH results in a directory
> to be supplied to the link step with no option as the suffix,
> it causes a linker error because the z/OS LD linker
> does not accept directories as input.
> Therefore, -L option is added.
Shouldn't all of the above removed by now, with 07bbe4ca (Merge
branch 'jc/make-libpath-template', 2024-02-06) that merged the
support for platforms whose linker does not have a way to specify
the path used at runtime to locate dynamic libraries and is already
in the Git 2.44 release? The autoconf-generated config.mak.autogen
would leave CC_LD_DYNPATH to empty on such a platform, which then
adds only one "-L $where" (instead of the usual "-L $where -R
$where") on the linker command line, making your earlier workaround
to use "-L $where -L $where" unnecessary?
> Also introduced z/OS (OS/390) as a platform in config.mak.uname
"Introduce z/OS ..." (cf. Documentation/SubmittingPatches)
Or perhaps
Add platform specific settings in config.mak.uname for zOS
(OS/390).
> diff --git a/config.mak.uname b/config.mak.uname
> index dacc95172dc..03ee2b74525 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -638,6 +638,18 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
> SHELL_PATH = /usr/coreutils/bin/bash
> endif
> +ifeq ($(uname_S),OS/390)
> + NO_SYS_POLL_H = YesPlease
> + NO_STRCASESTR = YesPlease
> + NO_REGEX = YesPlease
> + NO_MMAP = YesPlease
> + NO_NSEC = YesPlease
> + NO_STRLCPY = YesPlease
> + NO_MEMMEM = YesPlease
> + NO_GECOS_IN_PWENT = YesPlease
> + HAVE_STRINGS_H = YesPlease
> + NEEDS_MODE_TRANSLATION = YesPlease
All of the above should be indented with TAB to imitate the support
for other platforms, I think.
Thanks.
> +endif
> ifeq ($(uname_S),MINGW)
> ifeq ($(shell expr "$(uname_R)" : '1\.'),2)
> $(error "Building with MSys is no longer supported")
>
> base-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11
^ permalink raw reply
* [GSOC][PATCH] userdiff: Add JavaScript function patterns
From: Sergius Nyah @ 2024-03-01 7:40 UTC (permalink / raw)
To: git, christian.couder, gitster; +Cc: pk, shyamthakkar001, Sergius Nyah
In-Reply-To: <xmqqttlsjvsi.fsf@gitster.g>
This commit adds a patterns used to match JavaScript functions.
It now correctly identifies function declarations, function expressions,
and functions defined inside blocks. Add test for corresponding change in userdiff.
Signed-off-by: Sergius Nyah <sergiusnyah@gmail.com>
---
t/t4018-diff-funcname.sh | 22 ++++++++++++++++++++++
userdiff.c | 12 ++++++++++++
2 files changed, 34 insertions(+)
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index e026fac1f4..d35cce18a0 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -120,3 +120,25 @@ do
done
test_done
+
+test_expect_success 'identify builtin patterns in JavaScript' '
+ # setup
+ echo "function myFunction() { return true; }" > test.js &&
+ echo "var myVar = function() { return false; }" >> test.js &&
+ git add test.js &&
+ git commit -m "add test.js" &&
+
+ # modify the file
+ echo "function myFunction() { return false; }" > test.js &&
+ echo "var myVar = function() { return true; }" >> test.js &&
+
+ # command under test
+ git diff >output &&
+
+ # check results
+ test_i18ngrep "function myFunction() { return true; }" output &&
+ test_i18ngrep "function myFunction() { return false; }" output &&
+ test_i18ngrep "var myVar = function() { return false; }" output &&
+ test_i18ngrep "var myVar = function() { return true; }" output
+'
+test_done
\ No newline at end of file
diff --git a/userdiff.c b/userdiff.c
index 2b1dab2649..bbe2bcb9a3 100644
--- a/userdiff.c
+++ b/userdiff.c
+PATTERNS("javascript",
+ /* Looks for lines that start with optional whitespace, followed
+ * by 'function'* and any characters (for function declarations),
+ * or valid JavaScript identifiers, equals sign '=', 'function' keyword
+ * and any characters (for function expressions).
+ * Also considers functions defined inside blocks with '{...}'.
+ */
+ "^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
+ /* This pattern matches JavaScript identifiers */
+ "[a-zA-Z_$][0-9a-zA-Z_$]*"
+ "|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
+ "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
--
2.43.2
^ permalink raw reply related
* Accuracy of "git blame -C" - are there configuration options?
From: Baumann, Moritz @ 2024-03-01 7:21 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
A colleague recently experimented with "git blame -C" and noticed that
the accuracy with which git detects lines moved from other files varies
a lot depending on the sizes of the files involved.
Example repository: https://github.com/Boddlnagg/git-blame-problem
The latest commit on all branches moves some lines from Decl.cpp to
line 93 of DeclBase.cpp. "git blame -C" doesn't detect this on main, but
on the "works-as-expected" branch, in which the size of Decl.cpp was
artificially reduced, it does.
Further experiments show that the following work as well:
git blame -L 93,95 -C main -- DeclBase.cpp
git blame -L 80,108 -C main -- DeclBase.cpp
However, with a sufficiently large range, it stops working again:
git blame -L 80,109 -C main -- DeclBase.cpp
How does the pickaxe search of git blame work under the hood? Are there
any configuration options I can use if I want to trade performance for
accuracy?
Best regards,
Moritz Baumann
^ permalink raw reply
* Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it
From: Jeff King @ 2024-03-01 7:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqmsrji5zr.fsf@gitster.g>
On Thu, Feb 29, 2024 at 08:34:48AM -0800, Junio C Hamano wrote:
> > Otherwise the server complains that the other side did not respect its
> > advertised object-format (I sure am glad to have included the final
> > "hey, this input works, right?" test there, as that is what caught it).
>
> Ah, good finding. Will use it to amend.
>
> I wonder if it is still worth testing if the command is happy with
> an input that lacks object-format capability under SHA-1 mode. This
> test piece is primarily about packfile-uris, so in that sense, we
> are not all that interested in that unspecified client object-format
> defaults to the initial value of serve.c:client_hash_algo (which is
> SHA-1), and not testing that here is perfectly fine, I guess.
Yeah, if we want to test it, I'd prefer to do so separately as its own
test rather than keeping it as a subtle side effect of this one. I
looked around to see if it might exist already, but I didn't find one
(OTOH, it is hard to grep for since we are looking for the _absence_ of
an object-format line in hand-crafted input).
But taking a step back, why do we care about this case? It is because
older clients that speak v2 but do not yet know about object-format will
make a request without it, and they should still work in sha1 mode. So
the more general thing to test here is whether those older versions can
successfully fetch from a newer server.
There are tests in t/interop for cloning and fetching from a different
version. I kind of doubt anybody runs them regularly, though (and
picking the interesting versions to find this case isn't entirely
trivial). So I dunno.
-Peff
^ permalink raw reply
* Re: [PATCH v4 03/11] commit-reach(repo_in_merge_bases_many): report missing commits
From: Jeff King @ 2024-03-01 6:58 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Dirk Gouders, Johannes Schindelin
In-Reply-To: <1938b317a49f4d688bfefd7e5a74ad750a55a91a.1709113458.git.gitgitgadget@gmail.com>
On Wed, Feb 28, 2024 at 09:44:09AM +0000, Johannes Schindelin via GitGitGadget wrote:
> @@ -1402,6 +1436,8 @@ static int merge_mode_and_contents(struct merge_options *opt,
> &o->oid,
> &a->oid,
> &b->oid);
> + if (result->clean < 0)
> + return -1;
Sorry, I accidentally commented on v2 of your series a moment ago,
rather than the most recent version. But this hunk was untouched between
the two, so the comment still applies:
https://lore.kernel.org/git/20240301065647.GA2680308@coredump.intra.peff.net/
-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