* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-02 23:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <xmqqwmqkwdef.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
>>> specifying that this is the default or not is really useful. If the
>>> configuration was set to true, it is was a no-op. If set to false, no
>>> message will appear.
>>
>> I'm not sure either, and as it's not the topic of this particular patch,
>> I'd like to delegate the decision on the issue.
>
> It is very much spot on the topic of simplifying and clarifying the
> code to unify these remaining two messages into a single one.
I'm inclined to be more against merging than for it, as for me it'd be
confusing to be told that a configuration variable is set to true when I
didn't set it, nor there is any way to figure where it is set, because
in fact it isn't, and it's rather the default that is in use.
Overall, to me the messages are fine as they are (except -n that doesn't
belong there), I don't see compelling reason to hide information from
the user, and thus I won't propose patch that gets rid of one of them.
> And involving the --interactive that allows users a chance to
> rethink and refrain from removing some to the equation would also be
> worth doing in the same topic,
Worth doing what? I'm afraid I lost the plot here, as --interactive
still looks fine to me.
> even though it might not fit your immediate agenda of crusade against
> --dry-run.
I'm hopefully crusading for --dry-run, not against, trying to get rid of
the cause of the original confusion that started -n/-f controversy.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: René Scharfe @ 2024-03-02 22:00 UTC (permalink / raw)
To: Christian Couder, Josh Steadmon, git, Phillip Wood, Achu Luma
In-Reply-To: <CAP8UFD2t1KRo01eenK_RVndyVx5Vp9F4FepTgnR+mwhTGTvXnw@mail.gmail.com>
Am 27.02.24 um 11:04 schrieb Christian Couder:
> On Mon, Feb 26, 2024 at 7:58 PM Josh Steadmon <steadmon@google.com> wrote:
>>
>> On 2024.02.26 18:26, René Scharfe wrote:
>
>>> The output is clean as well, but there's a lot of it. Perhaps too much.
>>> The success messages are boring, though, and if all checks pass then the
>>> only useful information is the status code. A TAP harness like prove
>>> summarizes that nicely:
>>>
>>> $ prove t/unit-tests/bin/t-ctype
>>> t/unit-tests/bin/t-ctype .. ok
>>> All tests successful.
>>> Files=1, Tests=3598, 0 wallclock secs ( 0.08 usr + 0.00 sys = 0.08 CPU)
>>> Result: PASS
>>>
>>> Filtering out passing checks e.g. with "| grep -v ^ok" would help when
>>> debugging a test failure. I vaguely miss the --immediate switch from the
>>> regular test library, however.
>>
>> Yeah, I agree here. It's a lot of output but it's almost always going to
>> be consumed by a test harness rather than a human, and it's easy to
>> filter out the noise if someone does need to do some manual debugging.
>
> Yeah, I know about TAP harnesses like prove, but the most
> straightforward way to run the unit tests is still `make unit-tests`
> in the t/ directory. Also when you add or change some tests, it's a
> good idea to run `make unit-tests` to see what the output is, so you
> still have to see that output quite often when you work on tests and
> going through 3598 of mostly useless output instead of just 14 isn't
> nice.
I was starting the programs from t/unit-tests/bin/ individually because
I didn't know 'make unit-tests' exists. This is much nicer, thank you!
Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to
config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long
ago. It would be even nicer if the former was the default when the
latter is set.
As unit tests are added, their output is surely going to grow to
multiple screens with or without prove, no? So someone writing or
debugging tests will still go back to starting then individually
eventually.
The size of the output in itself is not a problem, I assume, but that
most of it is useless -- details of successful tests are uninteresting.
A test harness can aggregate the output, but prove annoyed me when used
with the regular tests by also aggregating error output and only showing
the numbers of failed tests. Finding their names involved running the
test script again without prove. Turns out it has an option for that.
Added 'GIT_PROVE_OPTS = --failures' to config.mak as well, will see if
it helps.
Is it too much to ask developers to use a test harness? Perhaps: It's
yet another dependency and not enabled by default.
What's the right level of aggregation and how do we achieve it?
Grouping by class is natural and follows the test definition. We
could stop after patch 2. Dunno.
René
^ permalink raw reply
* Re: Modernise .gitconfig Signing Block Name
From: Junio C Hamano @ 2024-03-02 21:08 UTC (permalink / raw)
To: admin; +Cc: git@vger.kernel.org
In-Reply-To: <KfyeAjLLXySd6IEKwEmOvrDqtBIhoCRPon7r8IuUIwPbUhlf8XR0CCmqP3Y5D-8hur57_SFQPqwk0xOA7p0h6xqw48ZMdNbzTBPN2hh9Ph4=@inferencium.net>
admin@inferencium.net writes:
> For backwards compatibility, `[gpg]` can also be allowed as an
> alias for the new name. The default could also be `format = gpg`,
> as it's still the most widely-used signing method.
Sounds good. Patches welcome.
Thanks.
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Junio C Hamano @ 2024-03-02 21:07 UTC (permalink / raw)
To: Sergey Organov; +Cc: Jean-Noël AVILA, git
In-Reply-To: <87r0gs8kgw.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
>> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
>> specifying that this is the default or not is really useful. If the
>> configuration was set to true, it is was a no-op. If set to false, no
>> message will appear.
>
> I'm not sure either, and as it's not the topic of this particular patch,
> I'd like to delegate the decision on the issue.
It is very much spot on the topic of simplifying and clarifying the
code to unify these remaining two messages into a single one.
And involving the --interactive that allows users a chance to
rethink and refrain from removing some to the equation would also be
worth doing in the same topic, even though it might not fit your
immediate agenda of crusade against --dry-run.
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-02 20:09 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: Junio C Hamano, git
In-Reply-To: <6033073.lOV4Wx5bFT@cayenne>
Jean-Noël AVILA <avila.jn@gmail.com> writes:
> On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:
>> 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
>>
>>
>
> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
> specifying that this is the default or not is really useful. If the
> configuration was set to true, it is was a no-op. If set to false, no
> message will appear.
I'm not sure either, and as it's not the topic of this particular patch,
I'd like to delegate the decision on the issue.
^ permalink raw reply
* Modernise .gitconfig Signing Block Name
From: admin @ 2024-03-02 20:01 UTC (permalink / raw)
To: git@vger.kernel.org
As SSH has been supported as a signing method for a while now in Git, the block name `gpg` within `.gitconfig` no longer makes sense as it's not the only method of signing commits and tags. I propose renaming `[gpg]` to something such as `[sig]` or `[sign]`. The current method of achieving this is `format = ssh` within the `[gpg]` block, which would be the same (hypothetically) as redefining SHA-1 to be SHA-256 via `format = sha256` within a `[sha1]` block; it really doesn't make sense in 2024 for it to be this way.
The outcome of this would be using `format = gpg` or `format = ssh` within a `[sig]` or `[sign]` block (or whatever anyone comes up with which could be better).
For backwards compatibility, `[gpg]` can also be allowed as an alias for the new name. The default could also be `format = gpg`, as it's still the most widely-used signing method.
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-02 19:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqv864zjbf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> 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.
>
> There is another thing I noticed.
>
> This part to get rid of "config_set" does make sense.
>
>> git_config(git_clean_config, NULL);
>> - if (force < 0)
>> - force = 0;
>> - else
>> - config_set = 1;
>
> We used to think "force" variable is the master switch to do
> anything , and requireForce configuration was a way to flip its
> default to 0 (so that you need to set it to 1 again from the command
> line). This separates "force" (which can only given via the command
> line) and "require_force" (which controls when the "force" is used)
> and makes the logic simpler.
>
>> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>> 0);
>
> However.
>
>> - 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;
>
> I am not sure if this is making things inconsistent.
I believe things rather got more consistent, see below.
>
> Dry run will be harmless, and we can be lenient and not require
> force. But below, we do not require force when going interactive,
> either.
Except, unlike dry-run, interactive is not harmless, similar to -f.
> So we could instead add
>
> if (dry_run || interactive)
> require_force = 0;
>
> above, drop the "&& !interactive" from the guard for the
> clean.requireForce block.
That'd be less consistent, as dry-run is harmless, whereas neither force
nor interactive are.
> Or we can go the opposite way. We do not have to tweak
> require_force at all based on other conditions. Instead we can
> update the guard below to check "!force && !interactive && !dry_run"
> before entering the clean.requireForce block, no?
No, we do need to tweak require_force, as another if() that is inside
and produces error message does in fact check for require_force being
either negative or positive, i.e., non-zero.
>
> But the code after this patch makes me feel that it is somewhere in
> the middle between these two optimum places.
I believe it's rather right in the spot. I left '-i' to stay with '-f',
as it was before the patch, as both are very distinct (even if in
different manner) when compared to '-n', so now only '-n' is now treated
separately.
The very idea of dry-run is that it is orthogonal to any other behavior,
so if I were designing it, I'd left bailing-out without -f or -i in
place even if -n were given, to show what exactly would happen without
-n. With new code it'd be as simple as removing "if (dry_run)
require_force = 0" line that introduces the original dependency.
>
> Another thing. Stepping back and thinking _why_ the code can treat
> dry_run and interactive the same way (either to make them drop
> require_force above, or neither of them contributes to the value of
> require_force), if we are dropping "you didn't give me --dry-run" in
> the error message below, we should also drop "you didn't give me
> --interactive, either" as well, when complaining about the lack of
> "--force".
In fact, the new code rather keep treating -f and -i somewhat similarly,
rather than -i and -n, intentionally.
That said, if somebody is going to re-consider -f vs -i issue, they now
have more cleaner code that doesn't involve -n anymore.
> One possible objection I can think of against doing so is that it
> might not be so obvious why "interactive" does not have to require
> "force" (even though it is clearly obvious to me). But if that were
> the objection, then to somebody else "dry-run does not have to
> require force" may equally not be so obvious (at least it wasn't so
> obvious to me during the last round of this discussion).
I'm not sure about interactive not requiring force, and I intentionally
avoided this issue in the patch in question, though I think the patch
makes it easier to reason about -i vs -f in the future by removing -n
handling from the picture.
>
> So I can live without the "drop 'nor -i'" part I suggested in the
> above. We would not drop "nor -i" and add "nor --dry-run" back to
> the message instead.
I'm afraid we can't meaningfully keep -n (--dry-run) in the messages. As
it stands, having -n there was a mistake right from the beginning.
Please consider the original message, but without -i and -f, for the
sake of the argument:
"clean.requireForce set to true and -n is not given; refusing to clean"
to me it sounds like nonsense, as it suggests that if were given -n,
we'd perform cleanup, that is simply false as no cleanup is ever
performed once -n is there. Adding -i and -f back to the message
somewhat blurs the problem, yet -n still does not belong there.
> So from that angle, the message after this patch makes me feel that
> it is somewhere in the middle between two more sensible places.
I don't think so, see above. I rather believe that even if everything
else in the patch were denied, the -n should be removed from the error
message, so I did exactly that, and only that (i.e., didn't merge 2
messages into one).
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH 0/2] Support diff.wordDiff config
From: Kristoffer Haugsbakk @ 2024-03-02 19:57 UTC (permalink / raw)
To: Karthik Nayak; +Cc: oliver, git, Junio C Hamano
In-Reply-To: <CAOLa=ZR6tN8eQhByaUobj3kS9wwYegsOQNT8cjZYA-YATJJt7w@mail.gmail.com>
Hi
On Sat, Mar 2, 2024, at 19:02, Karthik Nayak wrote:
> On Sat, Mar 2, 2024 at 6:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>> > This patch series adds the diff.wordDiff config option. This mimics the
>> > '--word-diff' option of `git-diff(1)`.
>>
>> Is it even be sensible to introduce this configuration variable in
>> the first place? What would this do to users who set this variable
>> and use third-party or their own scripts that run "git diff" under
>> the hood?
>
> This is definitely a good question to ask. I'm primarily not a user of this
> option, and this patch series was more of to start this discussion, based
> on the request. I'm comfortable dropping the patch series too if it doesn't
> make much sense.
This looks similar to the discussion from a [stash] topic:
• Proposed introducing config variables which change how `git stash
push` and `git stash save` behave (what they save)
• Concern about how that could break third-party scripts
Like here it would be opt-in. But the user might have no idea what kind
of scripts/programs that they use that happen to use git-stash(1).
(That’s at least how I read the thread)
I guess the concern might be worse for git-stash(1) since it seems very
natural to use that command in scripts in order to deal with a working
tree that might be in a who-knows condition: just get these things out
of the way so I can do what I want.
🔗 [stash]: https://lore.kernel.org/git/xmqq34tnyhhf.fsf@gitster.g/
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 2/2] diff: add 'diff.wordDiff' config option
From: Jean-Noël AVILA @ 2024-03-02 19:57 UTC (permalink / raw)
To: oliver, Karthik Nayak; +Cc: git, Karthik Nayak
In-Reply-To: <20240302095751.123138-3-karthik.188@gmail.com>
Hello,
On Saturday, 2 March 2024 10:57:51 CET Karthik Nayak wrote:
> The git-diff(1) command supports the `--word-diff` which allows the
> users to specify how to delimit word diffs. Provide this option also as
> a config param 'diff.wordDiff'.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/config/diff.txt | 4 +++
> diff.c | 50 +++++++++++++++++++++++++++--------
> t/t4034-diff-words.sh | 11 ++++++--
> 3 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
> index bd5ae0c337..00459beee2 100644
> --- a/Documentation/config/diff.txt
> +++ b/Documentation/config/diff.txt
> @@ -225,3 +225,7 @@ diff.colorMovedWS::
> When moved lines are colored using e.g. the `diff.colorMoved`
setting,
> this option controls the `<mode>` how spaces are treated
> for details of valid modes see '--color-moved-ws' in linkgit:git-
diff[1].
> +
> +diff.wordDiff::
> + Show a word diff, using the `<mode>` to delimit changed words.
> + For details of valid modes see '--word-diff' in linkgit:git-diff[1].
Let's enforce the new formatting rules: placeholders are _emphasized_ and
options are `verbatim`
+
+diff.wordDiff::
+ Show a word diff, using the _<mode>_ to delimit changed words.
+ For details of valid modes see `--word-diff` in linkgit:git-diff[1].
> diff --git a/diff.c b/diff.c
> index e50def4538..050d83ef85 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -69,6 +69,7 @@ static int diff_dirstat_permille_default = 30;
> static struct diff_options default_diff_options;
> static long diff_algorithm;
> static unsigned ws_error_highlight_default = WSEH_NEW;
> +static int diff_word_diff_default;
>
> static char diff_colors[][COLOR_MAXLEN] = {
> GIT_COLOR_RESET,
> @@ -209,6 +210,23 @@ int git_config_rename(const char *var, const char
*value)
> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
> }
>
> +static int parse_word_diff_value(const char *value)
> +{
> + if (!value)
> + return -1;
> + else if (!strcmp(value, "plain"))
> + return DIFF_WORDS_PLAIN;
> + else if (!strcmp(value, "color")) {
> + return DIFF_WORDS_COLOR;
> + }
> + else if (!strcmp(value, "porcelain"))
> + return DIFF_WORDS_PORCELAIN;
> + else if (!strcmp(value, "none"))
> + return DIFF_WORDS_NONE;
> +
> + return -1;
> +}
> +
> long parse_algorithm_value(const char *value)
> {
> if (!value)
> @@ -452,6 +470,18 @@ int git_diff_ui_config(const char *var, const char
*value,
> return 0;
> }
>
> + if (!strcmp(var, "diff.worddiff")) {
> + if (!value)
> + return config_error_nonbool(var);
> + diff_word_diff_default = parse_word_diff_value(value);
> + if (diff_word_diff_default < 0)
> + return error(_("unknown value for config '%s':
%s"),
Thanks for reusing already an already existing translatable string.
> + var, value);
> + if (diff_word_diff_default == DIFF_WORDS_COLOR)
> + diff_use_color_default = 1;
> + return 0;
> + }
> +
> if (git_color_config(var, value, cb) < 0)
> return -1;
>
> @@ -4724,6 +4754,7 @@ void repo_diff_setup(struct repository *r, struct
diff_options *options)
> options->use_color = diff_use_color_default;
> options->detect_rename = diff_detect_rename_default;
> options->xdl_opts |= diff_algorithm;
> + options->word_diff |= diff_word_diff_default;
> if (diff_indent_heuristic)
> DIFF_XDL_SET(options, INDENT_HEURISTIC);
>
> @@ -5504,21 +5535,18 @@ static int diff_opt_word_diff(const struct option
*opt,
> const char *arg, int unset)
> {
> struct diff_options *options = opt->value;
> + int value;
>
> BUG_ON_OPT_NEG(unset);
> +
> if (arg) {
> - if (!strcmp(arg, "plain"))
> - options->word_diff = DIFF_WORDS_PLAIN;
> - else if (!strcmp(arg, "color")) {
> - options->use_color = 1;
> - options->word_diff = DIFF_WORDS_COLOR;
> - }
> - else if (!strcmp(arg, "porcelain"))
> - options->word_diff = DIFF_WORDS_PORCELAIN;
> - else if (!strcmp(arg, "none"))
> - options->word_diff = DIFF_WORDS_NONE;
> - else
> + value = parse_word_diff_value(arg);
> + if (value < 0)
> return error(_("bad --word-diff argument:
%s"), arg);
> + if (value == DIFF_WORDS_COLOR)
> + options->use_color = 1;
> +
> + options->word_diff = value;
> } else {
> if (options->word_diff == DIFF_WORDS_NONE)
> options->word_diff = DIFF_WORDS_PLAIN;
> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
> index 4f70aa6e9f..2cc391c339 100755
> --- a/t/t4034-diff-words.sh
> +++ b/t/t4034-diff-words.sh
> @@ -56,12 +56,19 @@ diff_with_opts () {
> post=$(git rev-parse --short $(git hash-object post)) &&
> test_must_fail git diff --no-index "$@" pre post >output &&
> test_decode_color <output >output.decrypted &&
> - sed -e "2s/index [^ ]*/index $pre..$post/" expect >expected
> + sed -e "2s/index [^ ]*/index $pre..$post/" expect >expected &&
> test_cmp expected output.decrypted
> }
>
> word_diff () {
> - diff_with_opts "--word-diff=$1" $(echo "$@" | cut -d' ' -s -f 2-)
> + # Capture the rest of the arguments to passthrough.
> + rest=$(echo "$@" | cut -d' ' -s -f 2-) &&
> + # Test via the config route.
> + git config diff.wordDiff $1 &&
> + diff_with_opts $rest &&
> + git config --unset diff.wordDiff &&
> + # Test via the command option route.
> + diff_with_opts "--word-diff=$1" $rest
> }
>
> test_language_driver () {
>
Thanks
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Jean-Noël AVILA @ 2024-03-02 19:47 UTC (permalink / raw)
To: Sergey Organov; +Cc: Junio C Hamano, git
In-Reply-To: <87frxam35f.fsf@osv.gnss.ru>
On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:
> 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
>
>
Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that specifying
that this is the default or not is really useful. If the configuration was set
to true, it is was a no-op. If set to false, no message will appear.
^ permalink raw reply
* [PATCH 2/3] sideband: avoid short write(2)
From: Junio C Hamano @ 2024-03-02 19:03 UTC (permalink / raw)
To: git
In-Reply-To: <20240302190348.3946569-1-gitster@pobox.com>
The sideband demultiplexor writes the data it receives on sideband
with xwrite(). We can lose data if the underlying write(2) results
in a short write.
If they are limited to unimportant bytes like eye-candy progress
meter, it may be OK to lose them, but lets be careful and ensure
that we use write_in_full() instead. Note that the original does
not check for errors, and this rewrite does not check for one. At
least not yet.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
sideband.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sideband.c b/sideband.c
index 266a67342b..5d8907151f 100644
--- a/sideband.c
+++ b/sideband.c
@@ -220,7 +220,7 @@ int demultiplex_sideband(const char *me, int status,
}
strbuf_addch(scratch, *brk);
- xwrite(2, scratch->buf, scratch->len);
+ write_in_full(2, scratch->buf, scratch->len);
strbuf_reset(scratch);
b = brk + 1;
@@ -247,7 +247,7 @@ int demultiplex_sideband(const char *me, int status,
die("%s", scratch->buf);
if (scratch->len) {
strbuf_addch(scratch, '\n');
- xwrite(2, scratch->buf, scratch->len);
+ write_in_full(2, scratch->buf, scratch->len);
}
strbuf_release(scratch);
return 1;
--
2.44.0-84-gb387623c12
^ permalink raw reply related
* [PATCH 3/3] repack: check error writing to pack-objects subprocess
From: Junio C Hamano @ 2024-03-02 19:03 UTC (permalink / raw)
To: git
In-Reply-To: <20240302190348.3946569-1-gitster@pobox.com>
When "git repack" repacks promisor objects, it starts a pack-objects
subprocess and uses xwrite() to send object names over the pipe to
it, but without any error checking. An I/O error or short write
(even though a short write is unlikely for such a small amount of
data) can result in a packfile that lacks certain objects we wanted
to put in there, leading to a silent repository corruption.
Use write_in_full(), instead of xwrite(), to mitigate short write
risks, check errors from it, and abort if we see a failure.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/repack.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index ede36328a3..15e4cccc45 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -314,8 +314,9 @@ static int write_oid(const struct object_id *oid,
die(_("could not start pack-objects to repack promisor objects"));
}
- xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz);
- xwrite(cmd->in, "\n", 1);
+ if (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
+ write_in_full(cmd->in, "\n", 1) < 0)
+ die(_("failed to feed promisor objects to pack-objects"));
return 0;
}
--
2.44.0-84-gb387623c12
^ permalink raw reply related
* [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()
From: Junio C Hamano @ 2024-03-02 19:03 UTC (permalink / raw)
To: git
In-Reply-To: <20240302190348.3946569-1-gitster@pobox.com>
We have two packfile stream consumers, index-pack and
unpack-objects, that allow excess payload after the packfile stream
data. Their code to relay excess data hasn't changed significantly
since their original implementation that appeared in 67e5a5ec
(git-unpack-objects: re-write to read from stdin, 2005-06-28) and
9bee2478 (mimic unpack-objects when --stdin is used with index-pack,
2006-10-25).
These code blocks contain hand-rolled loops using xwrite(), written
before our write_in_full() helper existed. This helper now provides
the same functionality.
Replace these loops with write_in_full() for shorter, clearer
code. Update related variables accordingly.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/index-pack.c | 17 +++--------------
builtin/unpack-objects.c | 8 +-------
2 files changed, 4 insertions(+), 21 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a3a37bd215..856428fef9 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1524,14 +1524,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
struct strbuf pack_name = STRBUF_INIT;
struct strbuf index_name = STRBUF_INIT;
struct strbuf rev_index_name = STRBUF_INIT;
- int err;
if (!from_stdin) {
close(input_fd);
} else {
fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);
- err = close(output_fd);
- if (err)
+ if (close(output_fd))
die_errno(_("error while closing pack file"));
}
@@ -1566,17 +1564,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
write_or_die(1, buf.buf, buf.len);
strbuf_release(&buf);
- /*
- * Let's just mimic git-unpack-objects here and write
- * the last part of the input buffer to stdout.
- */
- while (input_len) {
- err = xwrite(1, input_buffer + input_offset, input_len);
- if (err <= 0)
- break;
- input_len -= err;
- input_offset += err;
- }
+ /* Write the last part of the buffer to stdout */
+ write_in_full(1, input_buffer + input_offset, input_len);
}
strbuf_release(&rev_index_name);
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index e0a701f2b3..f1c85a00ae 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -679,13 +679,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix UNUSED)
use(the_hash_algo->rawsz);
/* Write the last part of the buffer to stdout */
- while (len) {
- int ret = xwrite(1, buffer + offset, len);
- if (ret <= 0)
- break;
- len -= ret;
- offset += ret;
- }
+ write_in_full(1, buffer + offset, len);
/* All done */
return has_errors;
--
2.44.0-84-gb387623c12
^ permalink raw reply related
* [PATCH 0/3] Auditing use of xwrite()
From: Junio C Hamano @ 2024-03-02 19:03 UTC (permalink / raw)
To: git
The xwrite() helper relieves us from having to worry about an
interrupted write(2) system call that returns an EINTR error without
writing any, in which case we need to retry the system call. It,
just as the underlying write(2) system call, however can return
without writing the bytes requested to be written fully. A code
that uses xwrite() and wants to write the full contents of the
buffer needs to take care of such a "short write" situation,
typically by calling xwrite() in a loop.
We recently audited all the calls to xwrite(), and have found two
classes of potential "problems". This miniseries is about fixing them.
* There are such loops around xwrite() as described above, which
were written long time ago. The write_in_full() helper does
implement such a loop in a reusable way, and we should just call
it instead.
* There are xwrite() calls whose return value is ignored. We would
not be able to detect (and complete) "short write" in such callers,
of course, but more importantly, we would not notice I/O failures.
Again, use write_in_full() to deal with short writes, and check
the return value from it to detect errors.
Junio C Hamano (3):
unpack: rewrite hand-rolled loop of xwrite()s with write_in_full()
sideband: avoid short write(2)
repack: check error writing to pack-objects subprocess
builtin/index-pack.c | 17 +++--------------
builtin/repack.c | 5 +++--
builtin/unpack-objects.c | 8 +-------
sideband.c | 4 ++--
4 files changed, 9 insertions(+), 25 deletions(-)
--
2.44.0-84-gb387623c12
^ permalink raw reply
* Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message
From: Junio C Hamano @ 2024-03-02 18:08 UTC (permalink / raw)
To: Rubén Justo; +Cc: Eric Sunshine, git, Aryan Gupta, Eric Sunshine
In-Reply-To: <cf978790-4885-4103-946d-10f807048441@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> To improve the accuracy of the message, I wonder if it is worth doing
> ...
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index b5eaf7fdc1..5b5ee0dc1d 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -989,9 +989,10 @@ test_dir_is_empty () {
> # Check if the file exists and has a size greater than zero
> test_file_not_empty () {
> test "$#" = 2 && BUG "2 param"
> + test_path_is_file "$1" &&
> if ! test -s "$1"
> then
> - echo "'$1' is not a non-empty file."
> + echo "'$1' is empty but should not be"
> false
> fi
> }
Simple and effective to remove the need to have to worry about the
"missing" case. The "but should not be" part may still be subject
to discussion, but I do not have a strong opinion there.
^ permalink raw reply
* Re: [PATCH 2/2] diff: add 'diff.wordDiff' config option
From: Karthik Nayak @ 2024-03-02 18:02 UTC (permalink / raw)
To: Eric Sunshine; +Cc: oliver, git
In-Reply-To: <CAPig+cRXtk0f=gMSkRka8mCFfT2=PKSBXMQGGhA3J_t=O_rpAA@mail.gmail.com>
On Sat, Mar 2, 2024 at 11:25 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Mar 2, 2024 at 4:58 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> > The git-diff(1) command supports the `--word-diff` which allows the
> > users to specify how to delimit word diffs. Provide this option also as
> > a config param 'diff.wordDiff'.
> >
> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > ---
> > diff --git a/diff.c b/diff.c
> > @@ -209,6 +210,23 @@ int git_config_rename(const char *var, const char *value)
> > +static int parse_word_diff_value(const char *value)
> > +{
> > + if (!value)
> > + return -1;
> > + else if (!strcmp(value, "plain"))
> > + return DIFF_WORDS_PLAIN;
> > + else if (!strcmp(value, "color")) {
> > + return DIFF_WORDS_COLOR;
> > + }
> > + else if (!strcmp(value, "porcelain"))
> > + return DIFF_WORDS_PORCELAIN;
> > + else if (!strcmp(value, "none"))
> > + return DIFF_WORDS_NONE;
> > +
> > + return -1;
> > +}
>
> Why is one arm surrounded by curly braces when none of the others are?
Seems like I missed that one, if the patch series is being taken
forward, I will fix
it. Thanks
^ permalink raw reply
* Re: [PATCH 0/2] Support diff.wordDiff config
From: Karthik Nayak @ 2024-03-02 18:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: oliver, git
In-Reply-To: <xmqqedcszhty.fsf@gitster.g>
On Sat, Mar 2, 2024 at 6:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > This patch series adds the diff.wordDiff config option. This mimics the
> > '--word-diff' option of `git-diff(1)`.
>
> Is it even be sensible to introduce this configuration variable in
> the first place? What would this do to users who set this variable
> and use third-party or their own scripts that run "git diff" under
> the hood?
This is definitely a good question to ask. I'm primarily not a user of this
option, and this patch series was more of to start this discussion, based
on the request. I'm comfortable dropping the patch series too if it doesn't
make much sense.
>
> The usual answer is "these tools should be using the low-level
> plumbing commands like diff-files, diff-index, and diff-tree", so I
> am not worried about it too much myself, and the above is purely the
> devil's advocate comment.
>
> Having said that, running
>
> $ git grep -e 'git diff '
>
> in the collection of scripts I use [*] to work on this project, I am
> reminded that I may have to be a bit more conservative than I
> currently am about the risk of breaking scripts with the changes
> like the one being proposed.
>
> The proposed feature also may break those who use the git-prompt and
> diff-highlight available in conrib/, even though I am not sure how
> badly they would break, because I only looked at the lines given by
> this command:
>
> $ git grep -e 'git diff ' -- \*.sh ':!t/'
>
> and didn't check how the output from 'git diff' is used.
>
>
> [Footnote]
>
> * They can be seen in the 'todo' branch, if anybody is interested.
Having said that, wouldn't this cause a problem only if the config is set up?
Meaning the user must explicitly set `diff.wordDiff` for their scripts
to potentially
break. In that sense, is it a breaking feature?
^ permalink raw reply
* Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message
From: Junio C Hamano @ 2024-03-02 17:44 UTC (permalink / raw)
To: Dirk Gouders; +Cc: Eric Sunshine, Eric Sunshine, git, Aryan Gupta
In-Reply-To: <ghr0gtglhd.fsf@gouders.net>
Dirk Gouders <dirk@gouders.net> writes:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Fri, Mar 1, 2024 at 5:11 PM Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <ericsunshine@charter.net> writes:
>>> > A more accurate message might be "'foo' is empty but
>>> > should not be (or doesn't exist)", but that's unnecessarily long-winded
>>> > and adds little information that the test author couldn't discover by
>>> > noticing the file's absence.
>>>
>>> The "adds little information" version may be
>>>
>>> echo "'$1' is either missing or empty, but should not be"
>>> ...
>> I find "'$1' is either missing or empty, but should not be" suggestion
>> clear and easily understood. I'll reroll with that.
>
> This is a view from a position with more distance:
>
> I find that not so easily understood -- the "but should not
> be" part is rather unexpected and I feel, it doesn't provide necessary
> information, e.g.:
>
> test_path_is_executable () {
> ...
> echo "$1 is not executable"
> ...
>
> also doesn't state what is wanted and I doubt that message doesn't
> clearly describe the problem.
I cannot tell if you really meant the double negative involving
"doubt", but assuming you did, you are saying that
With "X is not Y", it is clear enough that we expect X to be Y
(if it were not clear to somebody who read "X is not Y" that we
want X to be Y, then "X is not Y, but it should be" may needed,
but "X is not Y" is clear enough).
So you think "$1 is either missing or empty" is better without "but
should not be" added to the end? Am I reading you correctly?
I think this takes us back pretty much to square one ;-) but that is
also fine.
But the above argument depends on an untold assumption. The message
"X is not Y" must be clearly understood as a complaint, not a mere
statement of a fact. I am not sure if that is the case.
Instead of "X is not Y, but it should be", the way to clarify these
messages may be to say "error: X is not Y", perhaps?
> While I looked at it: there is another `test -s` in test_grep () that
> perhaps could be fixed the same way:
>
> if test -s "$last_arg"
> then
> cat >&4 "$last_arg"
> else
> echo >&4 "<File '$last_arg' is empty>"
> fi
If you are worried about "test -s" failing because "$last_arg" does
not exist, then you are worried too much. We upfront guard the
test_grep() helper with "test -f" of the same file and diagnoses the
lack of the file as a bug in the test. And we do not assume gremlins
removing random files while we are running tests.
^ permalink raw reply
* Re: [GSOC][PATCH] userdiff: Add JavaScript function patterns
From: Junio C Hamano @ 2024-03-02 17:13 UTC (permalink / raw)
To: Christian Couder; +Cc: Sergius Nyah, git, pk, shyamthakkar001
In-Reply-To: <CAP8UFD1sAYORc_q20t4Y28mSjNa0BZ69XQ-hGFsNTzbThsG1KQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
>> + # 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
>
> I think we try to use just test_grep instead of test_i18ngrep these days.
Thanks for reminding. I am tempted to suggest doing this.
------ >8 ----------- >8 ----------- >8 ----------- >8 ------
Subject: test_i18ngrep: hard deprecate and forbid its use
Since v2.44.0-rc0~109 (Merge branch 'sp/test-i18ngrep', 2023-12-27)
none of the tests we have, either in 'master' or in flight and
collected in 'seen', use test_i18ngrep.
Perhaps it is good time to update test_i18ngrep to BUG to avoid
people adding new calls to it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/test-lib-functions.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git c/t/test-lib-functions.sh w/t/test-lib-functions.sh
index b5eaf7fdc1..6eaf116346 100644
--- c/t/test-lib-functions.sh
+++ w/t/test-lib-functions.sh
@@ -1263,9 +1263,8 @@ test_cmp_bin () {
cmp "$@"
}
-# Deprecated - do not use this in new code
test_i18ngrep () {
- test_grep "$@"
+ BUG "do not use test_i18ngrep---use test_grep instead"
}
test_grep () {
^ permalink raw reply related
* Re: [PATCH 0/2] Support diff.wordDiff config
From: Junio C Hamano @ 2024-03-02 17:03 UTC (permalink / raw)
To: Karthik Nayak; +Cc: oliver, git
In-Reply-To: <20240302095751.123138-1-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> This patch series adds the diff.wordDiff config option. This mimics the
> '--word-diff' option of `git-diff(1)`.
Is it even be sensible to introduce this configuration variable in
the first place? What would this do to users who set this variable
and use third-party or their own scripts that run "git diff" under
the hood?
The usual answer is "these tools should be using the low-level
plumbing commands like diff-files, diff-index, and diff-tree", so I
am not worried about it too much myself, and the above is purely the
devil's advocate comment.
Having said that, running
$ git grep -e 'git diff '
in the collection of scripts I use [*] to work on this project, I am
reminded that I may have to be a bit more conservative than I
currently am about the risk of breaking scripts with the changes
like the one being proposed.
The proposed feature also may break those who use the git-prompt and
diff-highlight available in conrib/, even though I am not sure how
badly they would break, because I only looked at the lines given by
this command:
$ git grep -e 'git diff ' -- \*.sh ':!t/'
and didn't check how the output from 'git diff' is used.
[Footnote]
* They can be seen in the 'todo' branch, if anybody is interested.
^ permalink raw reply
* Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message
From: Rubén Justo @ 2024-03-02 16:38 UTC (permalink / raw)
To: Eric Sunshine, git; +Cc: Junio C Hamano, Aryan Gupta, Eric Sunshine
In-Reply-To: <20240301204922.40304-1-ericsunshine@charter.net>
On Fri, Mar 01, 2024 at 03:49:22PM -0500, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> The function `test_file_not_empty` asserts that a file exists and is not
> empty. When the assertion fails, it complains:
>
> 'foo' is not a non-empty file.
>
> which is difficult to interpret due to the double-negative. To make it
> easier to understand the problem, simplify the message by dropping the
> double-negative and stating the problem more directly:
>
> 'foo' is empty but should not be
>
> (The full-stop is also dropped from the message to reflect the style of
> messages issued by other `test_path_*` functions.)
>
> Note: Technically, the revised message is slightly less accurate since
> the function asserts both that the file exists and that it is non-empty,
> but the new message talks only about the emptiness of the file, not
> whether it exists. A more accurate message might be "'foo' is empty but
> should not be (or doesn't exist)", but that's unnecessarily long-winded
> and adds little information that the test author couldn't discover by
> noticing the file's absence.
To improve the accuracy of the message, I wonder if it is worth doing
what we do in test_must_be_empty:
test_must_be_empty () {
test "$#" -ne 1 && BUG "1 param"
test_path_is_file "$1" &&
if test -s "$1"
then
echo "'$1' is not empty, it contains:"
cat "$1"
return 1
fi
}
Perhaps:
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b5eaf7fdc1..5b5ee0dc1d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -989,9 +989,10 @@ test_dir_is_empty () {
# Check if the file exists and has a size greater than zero
test_file_not_empty () {
test "$#" = 2 && BUG "2 param"
+ test_path_is_file "$1" &&
if ! test -s "$1"
then
- echo "'$1' is not a non-empty file."
+ echo "'$1' is empty but should not be"
false
fi
}
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> This is a tangential follow-up to the discussion at [1].
>
> [1]: https://lore.kernel.org/git/CAPig+cQ+JNBwydUq0CsTZGs8mHs3L3fJDuSosd+-WdKwWWw=gg@mail.gmail.com/
>
> t/test-lib-functions.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index b5eaf7fdc1..9e97b324c5 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -991,7 +991,7 @@ test_file_not_empty () {
> test "$#" = 2 && BUG "2 param"
> if ! test -s "$1"
> then
> - echo "'$1' is not a non-empty file."
> + echo "'$1' is empty but should not be"
> false
> fi
> }
> --
> 2.44.0
>
^ permalink raw reply related
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Philippe Blain @ 2024-03-02 16:32 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget, git
Cc: Johannes Schindelin, Elijah Newren, Phillip Wood, ZheNing Hu
In-Reply-To: <pull.1682.git.1709396291693.gitgitgadget@gmail.com>
Hi,
Le 2024-03-02 à 11:18, Philippe Blain via GitGitGadget a écrit :
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Allow disabling the advice shown when a squencer operation results in a
> merge conflict through a new config 'advice.sequencerConflict'.
>
> Update the tests accordingly. Note that the body of the second test in
> t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must
> escape them in the added line.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
I meant to CC your addresses in https://lore.kernel.org/git/pull.1682.git.1709396291693.gitgitgadget@gmail.com/
which I'm responding to, but the CC's did not get through somehow.
Cheers,
Philippe.
^ permalink raw reply
* Re: [PATCH] clean: improve -n and -f implementation and documentation
From: Junio C Hamano @ 2024-03-02 16:31 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <875xy76qe1.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> 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.
There is another thing I noticed.
This part to get rid of "config_set" does make sense.
> git_config(git_clean_config, NULL);
> - if (force < 0)
> - force = 0;
> - else
> - config_set = 1;
We used to think "force" variable is the master switch to do
anything , and requireForce configuration was a way to flip its
default to 0 (so that you need to set it to 1 again from the command
line). This separates "force" (which can only given via the command
line) and "require_force" (which controls when the "force" is used)
and makes the logic simpler.
> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
> 0);
However.
> - 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;
I am not sure if this is making things inconsistent.
Dry run will be harmless, and we can be lenient and not require
force. But below, we do not require force when going interactive,
either. So we could instead add
if (dry_run || interactive)
require_force = 0;
above, drop the "&& !interactive" from the guard for the
clean.requireForce block.
Or we can go the opposite way. We do not have to tweak
require_force at all based on other conditions. Instead we can
update the guard below to check "!force && !interactive && !dry_run"
before entering the clean.requireForce block, no?
But the code after this patch makes me feel that it is somewhere in
the middle between these two optimum places.
Another thing. Stepping back and thinking _why_ the code can treat
dry_run and interactive the same way (either to make them drop
require_force above, or neither of them contributes to the value of
require_force), if we are dropping "you didn't give me --dry-run" in
the error message below, we should also drop "you didn't give me
--interactive, either" as well, when complaining about the lack of
"--force".
One possible objection I can think of against doing so is that it
might not be so obvious why "interactive" does not have to require
"force" (even though it is clearly obvious to me). But if that were
the objection, then to somebody else "dry-run does not have to
require force" may equally not be so obvious (at least it wasn't so
obvious to me during the last round of this discussion).
So I can live without the "drop 'nor -i'" part I suggested in the
above. We would not drop "nor -i" and add "nor --dry-run" back to
the message instead.
So from that angle, the message after this patch makes me feel that
it is somewhere in the middle between two more sensible places.
> + 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"));
> }
>
> if (force > 1)
>
> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Rubén Justo @ 2024-03-02 16:18 UTC (permalink / raw)
To: Dragan Simic; +Cc: Junio C Hamano, git
In-Reply-To: <253d3c827f6e389d533c60113b08f94f@manjaro.org>
On Thu, Feb 29, 2024 at 09:09:25PM +0100, Dragan Simic wrote:
> On 2024-02-29 21:02, Rubén Justo wrote:
> > On Thu, Feb 29, 2024 at 11:33:24AM -0800, Junio C Hamano wrote:
> >
> > > Do not forget that the objective of the larger-picture-revamping of
> > > this page is to make the description of each option self-contained.
> > > Similarity between -m's description and -c's description does not
> > > count as being uselessly repetitive.
> >
> > OK. I was not considering this.
>
> I'd also like to avoid repetition, but if we want to have self-contained
> command descriptions (which is good), some repetition is unavoidable.
I don't see much of a problem with repetitions. Within the
self-containmnent aspect it makes a lot of sense. And, after all, my
messages have been mainly about repeating ... placeholders.
> In fact, we can see what avoiding the repetition has lead us to in the
> current state of the git-branch(1) man page.
Indeed. I agree that the lack of repetition has brought us here. But I
think we're not 100% aligned in what repetition is :-), yet. We're
working on it!
Thanks.
^ permalink raw reply
* [PATCH] sequencer: allow disabling conflict advice
From: Philippe Blain via GitGitGadget @ 2024-03-02 16:18 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
Allow disabling the advice shown when a squencer operation results in a
merge conflict through a new config 'advice.sequencerConflict'.
Update the tests accordingly. Note that the body of the second test in
t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must
escape them in the added line.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
sequencer: allow disabling conflict advice
CC: Elijah Newren newren@gmail.com CC: Phillip Wood
phillip.wood@dunelm.org.uk CC: Johannes Schindelin
Johannes.Schindelin@gmx.de CC: ZheNing Hu adlternative@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1682
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 1 +
sequencer.c | 33 ++++++++++++++++++---------------
t/t3501-revert-cherry-pick.sh | 1 +
t/t3507-cherry-pick-conflict.sh | 2 ++
6 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..736b88407a4 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -104,6 +104,9 @@ advice.*::
rmHints::
In case of failure in the output of linkgit:git-rm[1],
show directions on how to proceed from the current state.
+ sequencerConflict::
+ Advice shown when a sequencer operation stops because
+ of conflicts.
sequencerInUse::
Advice shown when a sequencer command is already in progress.
skippedCherryPicks::
diff --git a/advice.c b/advice.c
index 6e9098ff089..23e48194e74 100644
--- a/advice.c
+++ b/advice.c
@@ -71,6 +71,7 @@ static struct {
[ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
[ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
[ADVICE_RM_HINTS] = { "rmHints" },
+ [ADVICE_SEQUENCER_CONFLICT] = { "sequencerConflict" },
[ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" },
[ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
[ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..98966f8991d 100644
--- a/advice.h
+++ b/advice.h
@@ -40,6 +40,7 @@ enum advice_type {
ADVICE_RESOLVE_CONFLICT,
ADVICE_RM_HINTS,
ADVICE_SEQUENCER_IN_USE,
+ ADVICE_SEQUENCER_CONFLICT,
ADVICE_SET_UPSTREAM_FAILURE,
ADVICE_SKIPPED_CHERRY_PICKS,
ADVICE_STATUS_AHEAD_BEHIND_WARNING,
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..3e2f028ce2d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -467,7 +467,7 @@ static void print_advice(struct repository *r, int show_hint,
char *msg = getenv("GIT_CHERRY_PICK_HELP");
if (msg) {
- advise("%s\n", msg);
+ advise_if_enabled(ADVICE_SEQUENCER_CONFLICT, "%s\n", msg);
/*
* A conflict has occurred but the porcelain
* (typically rebase --interactive) wants to take care
@@ -480,22 +480,25 @@ static void print_advice(struct repository *r, int show_hint,
if (show_hint) {
if (opts->no_commit)
- advise(_("after resolving the conflicts, mark the corrected paths\n"
- "with 'git add <paths>' or 'git rm <paths>'"));
+ advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
+ _("after resolving the conflicts, mark the corrected paths\n"
+ "with 'git add <paths>' or 'git rm <paths>'"));
else if (opts->action == REPLAY_PICK)
- advise(_("After resolving the conflicts, mark them with\n"
- "\"git add/rm <pathspec>\", then run\n"
- "\"git cherry-pick --continue\".\n"
- "You can instead skip this commit with \"git cherry-pick --skip\".\n"
- "To abort and get back to the state before \"git cherry-pick\",\n"
- "run \"git cherry-pick --abort\"."));
+ advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
+ _("After resolving the conflicts, mark them with\n"
+ "\"git add/rm <pathspec>\", then run\n"
+ "\"git cherry-pick --continue\".\n"
+ "You can instead skip this commit with \"git cherry-pick --skip\".\n"
+ "To abort and get back to the state before \"git cherry-pick\",\n"
+ "run \"git cherry-pick --abort\"."));
else if (opts->action == REPLAY_REVERT)
- advise(_("After resolving the conflicts, mark them with\n"
- "\"git add/rm <pathspec>\", then run\n"
- "\"git revert --continue\".\n"
- "You can instead skip this commit with \"git revert --skip\".\n"
- "To abort and get back to the state before \"git revert\",\n"
- "run \"git revert --abort\"."));
+ advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
+ _("After resolving the conflicts, mark them with\n"
+ "\"git add/rm <pathspec>\", then run\n"
+ "\"git revert --continue\".\n"
+ "You can instead skip this commit with \"git revert --skip\".\n"
+ "To abort and get back to the state before \"git revert\",\n"
+ "run \"git revert --abort\"."));
else
BUG("unexpected pick action in print_advice()");
}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index aeab689a98d..bc7c878b236 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -170,6 +170,7 @@ test_expect_success 'advice from failed revert' '
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".
+ hint: Disable this message with "git config advice.sequencerConflict false"
EOF
test_commit --append --no-tag "double-add dream" dream dream &&
test_must_fail git revert HEAD^ 2>actual &&
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index c88d597b126..a643893dcbd 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -60,6 +60,7 @@ test_expect_success 'advice from failed cherry-pick' '
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
+ hint: Disable this message with "git config advice.sequencerConflict false"
EOF
test_must_fail git cherry-pick picked 2>actual &&
@@ -74,6 +75,7 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
error: could not apply \$picked... picked
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
+ hint: Disable this message with \"git config advice.sequencerConflict false\"
EOF
test_must_fail git cherry-pick --no-commit picked 2>actual &&
base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox