* RE: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
From: rsbecker @ 2024-01-10 17:55 UTC (permalink / raw)
To: 'Mohit Marathe', 'Christian Couder'
Cc: git, gitster, britton.kerin, peff
In-Reply-To: <F6ejgAfr2IMRNR3Tq0CDTHeT9xMWzJ9ley8M_fnSX97ayRNRp_CEgA62WdtOooi9bha1WJPGB53ptJYQFII2lCbIflwgNvbIaefw7nK8w7M=@proton.me>
On Wednesday, January 10, 2024 12:38 PM, Mohit Marathe wrote:
>>In https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/ Junio says:
>>
>>"Some places use atoi() immediately followed by strspn() to skip over
>>digits, which means they are parsing an integer and want to continue
>>reading after the integer, which is incompatible with what
>>strtol_i() wants to do. They need either a separate helper or an
>>updated strtol_i() that optionally allows you to parse the prefix and
>>report where the integer ended, e.g., something like:"
>>
>>and then he suggests the above helper.
>>
>>So it seems that the two instances you found look like good places
>>where Junio says the new helper could be useful.
>>
>>Now if you want to continue further on this, I think you would need to
>>take a closer look at those two instances to see if replacing atoi()
>>there with the new helper would improve something there or not. If you
>>find it would improve something, be sure to explain what would be
>>improved in the commit message.
>
>I took a closer look at `builtin/patch-id.c` and it seems replacing `atoi()` (which is
>used to parse numbers in the hunk header) wouldn't improve anything, unless I'm
>missing something.
>
>So then I tried finding other places where `atoi()` can be replaced but I find it
>difficult to find any reason that would justify the change.
>So far I've only looked at few of the MANY occurrences of `atoi()`.
>As far as I understand, the only advantage of `strtol_i()` over `atoi()` is better error
>handling. And most of places I've seen either already takes care of that or does not
>need that at all.
I am not sure this is a good idea. The error detection inside strtol_i() reports a -1 if the supplied text value is invalid. This does not differentiate between an invalid value and a valid "-1" supplied. Replacing all instances of atoi() with strtol_i() will likely cause breakages as the error semantics are different between the two.
--Randall
^ permalink raw reply
* [PATCH 5/5] t5325: run expected-to-fail `index-pack`s with `--threads=1`
From: Taylor Blau @ 2024-01-10 17:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <ZZ7VEVXSg1T8ZkIK@nand.local>
For identical reasons as in the previous commit, apply the same
treatment to expected-to-fail `index-pack` invocations in t5325 with
`--threads=1`.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/t5325-reverse-index.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh
index 431a603ca0..dc3d2235e8 100755
--- a/t/t5325-reverse-index.sh
+++ b/t/t5325-reverse-index.sh
@@ -64,7 +64,7 @@ test_expect_success 'index-pack can verify reverse indexes' '
chmod u+w $rev &&
printf "xxxx" | dd of=$rev bs=1 count=4 conv=notrunc &&
- test_must_fail git index-pack --rev-index --verify \
+ test_must_fail git index-pack --threads=1 --rev-index --verify \
$packdir/pack-$pack.pack 2>err &&
grep "validation error" err
'
--
2.43.0.288.g906e6a084d
^ permalink raw reply related
* [PATCH 4/5] t5313: run expected-to-fail `index-pack`s with `--threads=1`
From: Taylor Blau @ 2024-01-10 17:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <ZZ7VEVXSg1T8ZkIK@nand.local>
For identical reasons as in the previous commit, apply the same
treatment to expected-to-fail `index-pack` invocations in t5313 with
`--threads=1`.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/t5313-pack-bounds-checks.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
index ceaa6700a2..e9829b6ef3 100755
--- a/t/t5313-pack-bounds-checks.sh
+++ b/t/t5313-pack-bounds-checks.sh
@@ -91,7 +91,7 @@ test_expect_success 'pack/index object count mismatch' '
# ...and make sure that index-pack --verify, which has its
# own reading routines, does not segfault.
- test_must_fail git index-pack --verify $pack
+ test_must_fail git index-pack --threads=1 --verify $pack
'
test_expect_success 'matched bogus object count' '
@@ -111,7 +111,7 @@ test_expect_success 'matched bogus object count' '
git cat-file blob $object >actual &&
test_cmp file actual &&
- test_must_fail git index-pack --verify $pack
+ test_must_fail git index-pack --threads=1 --verify $pack
'
# Note that we cannot check the fallback case for these
@@ -128,7 +128,7 @@ test_expect_success 'bogus object offset (v1)' '
munge $idx $((4 * 256)) "\377\0\0\0" &&
clear_base &&
test_must_fail git cat-file blob $object &&
- test_must_fail git index-pack --verify $pack
+ test_must_fail git index-pack --threads=1 --verify $pack
'
test_expect_success 'bogus object offset (v2, no msb)' '
@@ -136,7 +136,7 @@ test_expect_success 'bogus object offset (v2, no msb)' '
munge $idx $(ofs_table 1) "\0\377\0\0" &&
clear_base &&
test_must_fail git cat-file blob $object &&
- test_must_fail git index-pack --verify $pack
+ test_must_fail git index-pack --threads=1 --verify $pack
'
test_expect_success 'bogus offset into v2 extended table' '
@@ -144,7 +144,7 @@ test_expect_success 'bogus offset into v2 extended table' '
munge $idx $(ofs_table 1) "\377\0\0\0" &&
clear_base &&
test_must_fail git cat-file blob $object &&
- test_must_fail git index-pack --verify $pack
+ test_must_fail git index-pack --threads=1 --verify $pack
'
test_expect_success 'bogus offset inside v2 extended table' '
@@ -169,7 +169,7 @@ test_expect_success 'bogus offset inside v2 extended table' '
mv tmp "$idx" &&
clear_base &&
test_must_fail git cat-file blob $object &&
- test_must_fail git index-pack --verify $pack
+ test_must_fail git index-pack --threads=1 --verify $pack
'
test_expect_success 'bogus OFS_DELTA in packfile' '
--
2.43.0.288.g906e6a084d
^ permalink raw reply related
* [PATCH 3/5] t5308: run expected-to-fail `index-pack`s with `--threads=1`
From: Taylor Blau @ 2024-01-10 17:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <ZZ7VEVXSg1T8ZkIK@nand.local>
For identical reasons as in the previous commit, apply the same
treatment to expected-to-fail `index-pack` invocations in t5308 with
`--threads=1`.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/t5308-pack-detect-duplicates.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5308-pack-detect-duplicates.sh b/t/t5308-pack-detect-duplicates.sh
index 655cafa054..5df552a42e 100755
--- a/t/t5308-pack-detect-duplicates.sh
+++ b/t/t5308-pack-detect-duplicates.sh
@@ -76,7 +76,7 @@ test_expect_success 'lookup in duplicated pack' '
test_expect_success 'index-pack can reject packs with duplicates' '
clear_packs &&
create_pack dups.pack 2 &&
- test_must_fail git index-pack --strict --stdin <dups.pack &&
+ test_must_fail git index-pack --threads=1 --strict --stdin <dups.pack &&
test_expect_code 1 git cat-file -e $LO_SHA1
'
--
2.43.0.288.g906e6a084d
^ permalink raw reply related
* [PATCH 2/5] t5302: run expected-to-fail `index-pack`s with `--threads=1`
From: Taylor Blau @ 2024-01-10 17:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <ZZ7VEVXSg1T8ZkIK@nand.local>
For identical reasons as in the previous commit, apply the same
treatment to expected-to-fail `index-pack` invocations in t5302 with
`--threads=1`.
(Note that this treatment only needs to be applied in tests which are
expected to pass when built with the leak-checker enabled).
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/t5302-pack-index.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index d88e6f1691..fdbcd80f89 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -290,7 +290,7 @@ test_expect_success 'too-large packs report the breach' '
pack=$(git pack-objects --all pack </dev/null) &&
sz="$(test_file_size pack-$pack.pack)" &&
test "$sz" -gt 20 &&
- test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
+ test_must_fail git index-pack --threads=1 --max-input-size=20 pack-$pack.pack 2>err &&
grep "maximum allowed size (20 bytes)" err
'
--
2.43.0.288.g906e6a084d
^ permalink raw reply related
* [PATCH 1/5] t5309: run expected-to-fail `index-pack`s with `--threads=1`
From: Taylor Blau @ 2024-01-10 17:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <ZZ7VEVXSg1T8ZkIK@nand.local>
The t5309 script triggers a racy false positive with SANITIZE=leak on a
multi-core system. Running with "--stress --run=6" usually fails within
10 seconds or so for me, complaining with something like:
+ git index-pack --fix-thin --stdin
fatal: REF_DELTA at offset 46 already resolved (duplicate base 01d7713666f4de822776c7622c10f1b07de280dc?)
=================================================================
==3904583==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x7fa790d01986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
#1 0x7fa790add769 in __pthread_getattr_np nptl/pthread_getattr_np.c:180
#2 0x7fa790d117c5 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
#3 0x7fa790d11957 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:598
#4 0x7fa790d03fe8 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:51
#5 0x7fa790d013fd in __lsan_thread_start_func ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:440
#6 0x7fa790adc3eb in start_thread nptl/pthread_create.c:444
#7 0x7fa790b5ca5b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).
Aborted
What happens is this:
0. We construct a bogus pack with a duplicate object in it and trigger
index-pack.
1. We spawn a bunch of worker threads to resolve deltas (on my system
it is 16 threads).
2. One of the threads sees the duplicate object and bails by calling
exit(), taking down all of the threads. This is expected and is the
point of the test.
3. At the time exit() is called, we may still be spawning threads from
the main process via pthread_create(). LSan hooks thread creation
to update its book-keeping; it has to know where each thread's
stack is (so it can find entry points for reachable memory). So it
calls pthread_getattr_np() to get information about the new thread.
That may allocate memory that must be freed with a matching call to
pthread_attr_destroy(). Probably LSan does that immediately, but
if you're unlucky enough, the exit() will happen while it's between
those two calls, and the allocated pthread_attr_t appears as a
leak.
This isn't a real leak. It's not even in our code, but rather in the
LSan instrumentation code. So we could just ignore it. But the false
positive can cause people to waste time tracking it down.
It's possibly something that LSan could protect against (e.g., cover the
getattr/destroy pair with a mutex, and then in the final post-exit()
check for leaks try to take the same mutex). But I don't know enough
about LSan to say if that's a reasonable approach or not (or if my
analysis is even completely correct).
One approach to papering over this issue (short of LSan fixing it
upstream) is to make the creation of work threads "atomic", i.e. by
spawning all of them before letting any of them start to work. This
shouldn't make any practical difference for non-LSan runs. The thread
spawning is quick, and could happen before any worker thread gets
scheduled anyway.
But that requires us to tweak production code (albeit at a negligible
cost) in order to appease LSan in this narrow circumstance. Another
approach is to simply run these expected-to-fail `index-pack`
invocations with `--threads=1` so that we bypass the above issue
entirely.
The downside of that approach is that the test doesn't match our
production code as well as it did before (where we might have run those
same `index-pack` invocations with >1 thread, depending on how many CPUs
the testing machine has). The risk there is that we might miss a
regression that would leave us in an inconsistent state. But that feels
rather unlikely in practice, and there are many other tests related to
`index-pack` in the suite.
Original-patch-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/t5309-pack-delta-cycles.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
index 4e910c5b9d..4100595c89 100755
--- a/t/t5309-pack-delta-cycles.sh
+++ b/t/t5309-pack-delta-cycles.sh
@@ -44,7 +44,7 @@ test_expect_success 'index-pack detects missing base objects' '
pack_obj $A $B
} >missing.pack &&
pack_trailer missing.pack &&
- test_must_fail git index-pack --fix-thin --stdin <missing.pack
+ test_must_fail git index-pack --threads=1 --fix-thin --stdin <missing.pack
'
test_expect_success 'index-pack detects REF_DELTA cycles' '
@@ -55,13 +55,13 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '
pack_obj $B $A
} >cycle.pack &&
pack_trailer cycle.pack &&
- test_must_fail git index-pack --fix-thin --stdin <cycle.pack
+ test_must_fail git index-pack --threads=1 --fix-thin --stdin <cycle.pack
'
test_expect_success 'failover to an object in another pack' '
clear_packs &&
git index-pack --stdin <ab.pack &&
- test_must_fail git index-pack --stdin --fix-thin <cycle.pack
+ test_must_fail git index-pack --threads=1 --stdin --fix-thin <cycle.pack
'
test_expect_success 'failover to a duplicate object in the same pack' '
@@ -73,7 +73,7 @@ test_expect_success 'failover to a duplicate object in the same pack' '
pack_obj $A
} >recoverable.pack &&
pack_trailer recoverable.pack &&
- test_must_fail git index-pack --fix-thin --stdin <recoverable.pack
+ test_must_fail git index-pack --threads=1 --fix-thin --stdin <recoverable.pack
'
test_done
--
2.43.0.288.g906e6a084d
^ permalink raw reply related
* Re: [PATCH] branch: error description when deleting a not fully merged branch
From: Junio C Hamano @ 2024-01-10 17:48 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <04c3556f-0242-4ac3-b94a-be824cd2004a@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> The error message we show when the user tries to delete a not fully
> merged branch describes the error and gives a hint to the user:
>
> error: the branch 'foo' is not fully merged.
> If you are sure you want to delete it, run 'git branch -D foo'.
>
> Let's move the hint part so that it takes advantage of the advice
> machinery:
>
> error: the branch 'foo' is not fully merged
> hint: If you are sure you want to delete it, run 'git branch -D foo'
> hint: Disable this message with "git config advice.forceDeleteBranch false"
This is probably one sensible step forward, so let's queue it as-is.
But with reservations for longer-term future direction. Stepping
back a bit, when 'foo' is not fully merged and the user used "branch
-d" on it, is it sensible for us to suggest use of "branch -D"?
Especially now this is a "hint" to help less experienced folks, it
may be helpful to suggest how the user can answer "If you are sure
you want to delete" part. As this knows what unique commits on the
branch being deleted are about to be lost, one way to do so may be
to tell the user about them ("you are about to lose 'branch: error
description when deleting a not fully merged branch' and other 47
commits that are not merged the target branch 'main'", for example).
Another possibility is to suggest merging the branch into the
target, instead of suggesting a destructive "deletion", but I
suspect that it goes too far second-guessing the end-user intention.
Thanks.
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>
> This change is a pending NEEDSWORK from a recent series about adjusting
> the error messages in branch.c
>
> Unfortunately the full message now becomes a three line message.
>
> Hopefully we can find a way in the near future to keep it at two.
>
> Documentation/config/advice.txt | 3 +++
> advice.c | 1 +
> advice.h | 3 ++-
> builtin/branch.c | 9 ++++++---
> 4 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index 4d7e5d8759..5814d659b9 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -142,4 +142,7 @@ advice.*::
> Advice shown when a user tries to create a worktree from an
> invalid reference, to instruct how to create a new unborn
> branch instead.
> + forceDeleteBranch::
> + Advice shown when a user tries to delete a not fully merged
> + branch without the force option set.
> --
> diff --git a/advice.c b/advice.c
> index 50c79443ba..e91f5d7ab8 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -79,6 +79,7 @@ static struct {
> [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
> [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
> [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
> + [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 },
> };
>
> static const char turn_off_instructions[] =
> diff --git a/advice.h b/advice.h
> index 2affbe1426..5bef900142 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -10,7 +10,7 @@ struct string_list;
> * Add the new config variable to Documentation/config/advice.txt.
> * Call advise_if_enabled to print your advice.
> */
> - enum advice_type {
> +enum advice_type {
> ADVICE_ADD_EMBEDDED_REPO,
> ADVICE_ADD_EMPTY_PATHSPEC,
> ADVICE_ADD_IGNORED_FILE,
> @@ -50,6 +50,7 @@ struct string_list;
> ADVICE_WAITING_FOR_EDITOR,
> ADVICE_SKIPPED_CHERRY_PICKS,
> ADVICE_WORKTREE_ADD_ORPHAN,
> + ADVICE_FORCE_DELETE_BRANCH,
> };
>
> int git_default_advice_config(const char *var, const char *value);
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0a32d1b6c8..2240433bc8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -24,6 +24,7 @@
> #include "ref-filter.h"
> #include "worktree.h"
> #include "help.h"
> +#include "advice.h"
> #include "commit-reach.h"
>
> static const char * const builtin_branch_usage[] = {
> @@ -190,9 +191,11 @@ static int check_branch_commit(const char *branchname, const char *refname,
> return -1;
> }
> if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
> - error(_("the branch '%s' is not fully merged.\n"
> - "If you are sure you want to delete it, "
> - "run 'git branch -D %s'"), branchname, branchname);
> + error(_("the branch '%s' is not fully merged"),
> + branchname);
> + advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
> + _("If you are sure you want to delete it, "
> + "run 'git branch -D %s'"), branchname);
> return -1;
> }
> return 0;
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Dragan Simic @ 2024-01-10 17:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Rubén Justo, Jeff King, Git List
In-Reply-To: <xmqqil41duov.fsf@gitster.g>
On 2024-01-10 17:22, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> No worries. Regarding disabling the advices for disabling the advice
>> messages, maybe it would be better to have only one configuration knob
>> for that purpose, e.g. "core.verboseAdvice", as a boolean knob.
>
> I am not sure if you understood Peff's example that illustrates why
> it is a bad thing, as ...
>
>> That
>> way, fishing for the right knob for some advice message wouldn't turn
>> itself into an issue,
>
> ... this is exactly what a single core.verboseAdvice knob that
> squelches the "how to disable this particular advice message" part
> from the message is problematic. Before you see and familialize
> yourself with all advice messages, you may see one piece of advice X
> and find it useful to keep, feeling no need to turn it off. If you
> use that single knob to squelch the part to tell you how to turn
> advice X off. But what happens when you hit another unrelated
> advice Y then? Because your core.verboseAdvice is a single big red
> button, the message does not say which advice.* variable to tweak
> for this particular advice message Y.
Makes sense, but please allow me to explain how I envisioned the whole
thing
with the single, big core.verboseAdvice configuration knob:
1) You use git and find some advice useful, so you decide to keep it
displayed.
However, the additional advice about turning the advice off becomes
annoying
a bit, or better said, it becomes redundant because the main advice
stays.
2) As a result, you simply set core.verboseAdvice to false and voila,
the
redundant additional advice disappears. Seems perfect! Of course,
it
isn't perfect, as the next point will clearly show.
3) You keep using git, and some advice becomes no longer needed, maybe
even
one of the advices that you previously used to find useful, or you
find
some newly added advice a bit annoying and, as a result, not needed.
But,
what do you do, where's that helpful additional advice?
4) As a careful git user that remembers important things, you go back to
your
git configuration file and set core.verboseAdvice to true, and the
additional
advices are back, telling you how to disable the unwanted advice.
5) After you disable the unwanted advice, you set core.verboseAdvice
back to
false and keep it that way until the next redundant advice pops up.
However, I do see that this approach has its downsides, for example the
need
for the unwanted advice to be displayed again together with the
additional advice,
by executing the appropriate git commands, after the above-described
point #4.
Let's see what it would look like with the granular, per-advice on/off
knobs:
1) You use git and find some advice useful, so you decide to keep it
displayed.
However, the additional advice about turning the advice off becomes
annoying
a bit, or better said, it becomes redundant because the main advice
stays.
2) As a result, you follow the additional advice and set the specific
knob to
false, and voila, the redundant additional advice disappears. Of
course,
it once again isn't perfect, as the next point will clearly show.
3) You keep using git, and one of the advices that you previously used
to find
useful becomes no longer needed. But, what do you do, where's that
helpful
additional advice telling you how to turn the advice off?
4) Now you need to dig though the manual pages, or to re-enable the
additional
advices in your git configuration file, perhaps all of them at once,
while
keeping a backup of your original settings, to restore it later.
Then, you
again need to wait until the original advice gets displayed.
5) The additional advice is finally back, after some time passes or
after
specifically reproducing the exact git commands, telling you how to
disable
the unwanted advice. Of course, you follow the advice and set the
right
knob in your git configuration file.
5) After you disable the unwanted advice, you restore the git
configuration
backup that you made earlier (you did that, right?), taking care not
to
override the newly made changes that disabled the unwanted advice.
Quite frankly, the second approach, although more granular, seems much
more
complicated and more error-prone to me.
Of course, please let me know if I'm missing something obvious.
^ permalink raw reply
* Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
From: Mohit Marathe @ 2024-01-10 17:38 UTC (permalink / raw)
To: Christian Couder
Cc: git@vger.kernel.org, gitster@pobox.com, britton.kerin@gmail.com,
peff@peff.net
In-Reply-To: <CAP8UFD1d7FSa=mUzzUA5e3eSEcCVfaymxWewo5GjdDBi4GyE-g@mail.gmail.com>
>In https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/ Junio says:
>
>"Some places use atoi() immediately followed by strspn() to skip over
>digits, which means they are parsing an integer and want to continue
>reading after the integer, which is incompatible with what
>strtol_i() wants to do. They need either a separate helper or an
>updated strtol_i() that optionally allows you to parse the prefix
>and report where the integer ended, e.g., something like:"
>
>and then he suggests the above helper.
>
>So it seems that the two instances you found look like good places
>where Junio says the new helper could be useful.
>
>Now if you want to continue further on this, I think you would need to
>take a closer look at those two instances to see if replacing atoi()
>there with the new helper would improve something there or not. If you
>find it would improve something, be sure to explain what would be
>improved in the commit message.
I took a closer look at `builtin/patch-id.c` and it seems replacing
`atoi()` (which is used to parse numbers in the hunk header) wouldn't
improve anything, unless I'm missing something.
So then I tried finding other places where `atoi()` can be replaced
but I find it difficult to find any reason that would justify the change.
So far I've only looked at few of the MANY occurrences of `atoi()`.
As far as I understand, the only advantage of `strtol_i()`
over `atoi()` is better error handling. And most of places I've seen either
already takes care of that or does not need that at all.
Thanks!
^ permalink raw reply
* Re: [PATCH] index-pack: spawn threads atomically
From: Taylor Blau @ 2024-01-10 17:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240110114456.GF16674@coredump.intra.peff.net>
On Wed, Jan 10, 2024 at 06:44:56AM -0500, Jeff King wrote:
> On Fri, Jan 05, 2024 at 11:33:23AM -0500, Taylor Blau wrote:
>
> > - test_must_fail git index-pack --fix-thin --stdin <recoverable.pack
> > + test_must_fail git index-pack --threads=1 --fix-thin --stdin <recoverable.pack
> > [...]
> > For what it's worth, I'm fine with either approach, mostly to avoid
> > tying up more of the list's time discussing the options. But I have a
> > vague preference towards `--threads=1` since it doesn't require us to
> > touch production code.
>
> That's quite tempting, actually. The flip side, though, is that the test
> no longer reflects the production code as well. That is, in the real
> world we'd still call exit() from a thread. That obviously works OK now
> (modulo LSan), but if we ever had a regression where that left us in an
> inconsistent state, we'd be less likely to notice it. Feels kind of
> unlikely in practice, though.
>
> I dunno. I guess the real least-bad thing is seeing if LSan can be
> fixed to handle this atomically. I haven't even reported it there.
In the meantime, I think that the `--threads=1` approach feels less
invasive. I tend to agree that neither option is ideal, but that
`--threads=1` is probably the least bad, and that failing to catch a
regression there feels rather unlikely.
> If do go with "--threads=1", I suspect several tests in that file need
> it.
Yeah, there are a couple of others. I think the ones that need modifying
are at the intersection of "expected to fail" and "in a test which is
expected to pass leak-free":
$ grep -l 'TEST_PASSES_SANITIZE_LEAK=true' t????-*.sh |
xargs grep -l 'test_must_fail git index-pack'
t5302-pack-index.sh
t5308-pack-detect-duplicates.sh
t5309-pack-delta-cycles.sh
t5313-pack-bounds-checks.sh
t5325-reverse-index.sh
I'll send a series shortly to tweak those test scripts to avoid this
issue if you want to notify the LSan folks of this issue more generally.
> -Peff
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/4] reftable/stack: refactor stack reloading to have common exit path
From: Junio C Hamano @ 2024-01-10 17:30 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <01ece2626dd4cb494829e146d99c172fa8428478.1704714575.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The `reftable_stack_reload_maybe_reuse()` function is responsible for
> reloading the reftable list from disk. The function is quite hard to
> follow though because it has a bunch of different exit paths, many of
> which have to free the same set of resources.
>
> Refactor the function to have a common exit path. While at it, touch up
> the style of this function a bit to match our usual coding style better.
> ---
> reftable/stack.c | 86 +++++++++++++++++++++++-------------------------
> 1 file changed, 42 insertions(+), 44 deletions(-)
Missing sign-off.
Other than that, I did not find anything questionable in the
conversion. By sticking to the two simple invariants:
- we use "err" as our return value when we jump to "out:"
- we always keep "names" and "names_after" freeable, and free them
when we jump to "out:".
the exit status and leak prevention are both very clear (and the
behaviour is not changed---it is not like there are any existing
leaks that are plugged by this restructuring of the loop).
^ permalink raw reply
* Re: [PATCH] t4129: prevent loss of exit code due to the use of pipes
From: Junio C Hamano @ 2024-01-10 17:25 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1636.git.1704891257544.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> t/t4129-apply-samemode.sh | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Looks good. Will queue with two instances of a minor style fix (see
below).
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index e7a7295f1b6..ffabeafa213 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -41,7 +41,8 @@ test_expect_success FILEMODE 'same mode (index only)' '
> chmod +x file &&
> git add file &&
> git apply --cached patch-0.txt &&
> - git ls-files -s file | grep "^100755"
> + git ls-files -s file > ls-files-output &&
Redirection operator ">" and "<" sticks to the file in question
(look for "> " and "< " in this file and you'd find none), i.e.
git ls-files -s file >ls-files-output &&
Thanks.
^ permalink raw reply
* Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph
From: Junio C Hamano @ 2024-01-10 16:38 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git, Scott Leggett
In-Reply-To: <20240110113914.GE16674@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think a simpler solution would be that upon clearing the slab, we
> either "finish" each commit (filling in the maybe_tree field) or
> "unparse" it (unsetting the parsed flag, and then doing a regular and/or
> graph lookup if it is accessed again later).
Wow, that's clever.
> It should be easy-ish to iterate through the slab and look at the
> commits that are mentioned in it. Though maybe not? Each commit knows
> its slab-id, but I'm not sure if we have a master list of commits to go
> the other way.
We have table of all in-core objects, don't we?
> + * This will throw away the parents list, which is potentially sketchy.
> + * A better version of this would just unset commit->object.parsed
> + * and then do a minimal version of parse_commit() that _just_ loads
> + * the tree data (and/or graph position if available).
Yeah, it is a concern that we may be working with an in-core commit
object whose parent list has already been rewritten during revision
traversal. Well thought out.
> + *
> + * Naturally we'd need to drop the "const" from our commit above, too.
> + */
> + unparse_commit(r, &commit->object.oid);
> + repo_parse_commit(r, commit);
> +
> return NULL;
> }
>
> I dunno. I do feel like this is a lurking maintenance headache, and
> might even be a triggerable bug. But without knowing of a way that it
> happens in the current code base, it feels like it would be easy to make
> things worse rather than better.
Unfortunately I share the feeling X-<.
Thanks.
^ permalink raw reply
* [PATCH v3 2/2] t7501: add tests for --amend --signoff
From: Ghanshyam Thakkar @ 2024-01-10 16:35 UTC (permalink / raw)
To: git; +Cc: phillip.wood123, christian.couder, gitster, Ghanshyam Thakkar
In-Reply-To: <20240109165304.8027-2-shyamthakkar001@gmail.com>
Add tests for amending the commit to add Signed-off-by trailer. And
also to check if it does not add another trailer if one already exists.
Currently, there are tests for --signoff separately in t7501, however,
they are not tested with --amend.
Therefore, these tests belong with other similar tests of --amend in
t7501-commit-basic-functionality.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index e005175d0b..546d60d7fc 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# fixme: test the various index usages, test reflog,
-# signoff
+# fixme: test the various index usages, test reflog
test_description='git commit'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -430,6 +429,30 @@ test_expect_success 'amend commit to fix date' '
'
+test_expect_success 'amend commit to add signoff' '
+
+ test_commit "msg" file content &&
+ git commit --amend --signoff &&
+ test_commit_message HEAD <<-EOF
+ msg
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+
+'
+
+test_expect_success 'amend does not add signoff if it already exists' '
+
+ test_commit --signoff "tenor" file newcontent &&
+ git commit --amend --signoff &&
+ test_commit_message HEAD <<-EOF
+ tenor
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+
+'
+
test_expect_success 'commit mentions forced date in output' '
git commit --amend --date=2010-01-02T03:04:05 >output &&
grep "Date: *Sat Jan 2 03:04:05 2010" output
--
2.43.0
^ permalink raw reply related
* [PATCH v3 1/2] t7501: add tests for --include and --only
From: Ghanshyam Thakkar @ 2024-01-10 16:35 UTC (permalink / raw)
To: git; +Cc: phillip.wood123, christian.couder, gitster, Ghanshyam Thakkar
In-Reply-To: <20240109165304.8027-2-shyamthakkar001@gmail.com>
Add tests for --include (-i) and --only (-o) of commit. This includes
testing --include with and without staged changes, testing --only with
and without staged changes and testing --only and --include together.
Some tests already exist in t7501 for testing --only, however, it is
only tested in combination with --amend, --allow-empty and to-be-born
branch, and not for testing --only separately.
Similarly there are no separate tests for --include.
These tests are an addition to other similar tests in t7501,
as described above in the case of --only, therefore, they belong
in t7501-commit-basic-functionality.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 43 ++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..e005175d0b 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# fixme: test the various index usages, test reflog,
# signoff
test_description='git commit'
@@ -150,6 +150,47 @@ test_expect_success 'setup: commit message from file' '
git commit -F msg -a
'
+test_expect_success '--include fails with untracked files' '
+ echo content >baz &&
+ test_must_fail git commit --include -m "initial" baz
+'
+
+test_expect_success '--include with staged changes' '
+ echo newcontent >baz &&
+ echo newcontent >file &&
+ git add file &&
+ git commit --include -m "file baz" baz &&
+
+ git diff --name-only >remaining &&
+ test_must_be_empty remaining &&
+ git diff --name-only --staged >remaining &&
+ test_must_be_empty remaining
+'
+
+test_expect_success '--only fails with untracked files' '
+ echo content >newfile &&
+ test_must_fail git commit --only -m "newfile" newfile
+'
+
+test_expect_success '--only excludes staged changes' '
+ echo content >file &&
+ echo content >baz &&
+ git add baz &&
+ git commit --only -m "file" file &&
+
+ git diff --name-only >actual &&
+ test_must_be_empty actual &&
+ git diff --name-only --staged >actual &&
+ test_grep "baz" actual
+'
+
+test_expect_success '--include and --only do not mix' '
+ test_when_finished "git reset --hard" &&
+ echo new >file &&
+ echo new >baz &&
+ test_must_fail git commit --include --only -m "bar" bar baz
+'
+
test_expect_success 'amend commit' '
cat >editor <<-\EOF &&
#!/bin/sh
--
2.43.0
^ permalink raw reply related
* [PATCH v3 0/2] t7501: add tests for --include, --only and
From: Ghanshyam Thakkar @ 2024-01-10 16:35 UTC (permalink / raw)
To: git; +Cc: phillip.wood123, christian.couder, gitster, Ghanshyam Thakkar
In-Reply-To: <20240109165304.8027-2-shyamthakkar001@gmail.com>
The v3 of the patch series addresses the comments from Junio and adds
the test to check --amend --signoff does not add signoff if it
already exists as suggested by Phillip Wood.
Also I have removed two tests from v2 which tested the same thing as
other tests in the series, namely:
- removed 'commit --include'. This pattern is tested in '--include with
staged changes'.
- removed 'commit --only'. This pattern is tested in '--only excludes
staged changes'.
Ghanshyam Thakkar (2):
t7501: add tests for --include and --only
t7501: add tests for --amend --signoff
t/t7501-commit-basic-functionality.sh | 68 ++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 2 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH 4/6] t1419: mark test suite as files-backend specific
From: Junio C Hamano @ 2024-01-10 16:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Eric Sunshine, git
In-Reply-To: <ZZ5HrY76O1x8QABG@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
On just this (non-technical) part...
>> > file, but not for loose references. Consequentially, all callers must
>> > still filter emitted refs with those exclude patterns.
>>
>> s/Consequentially/Consequently/
>
> Hum. I had the last time when you mentioned the in mind while writing
> the commit message, but seemingly misremembered the outcome. So I now
> looked things up in a dictionary, and both words seem to be used in
> equivalent ways. As a non-native speaker, could you maybe elaborate a
> bit to help me out? :)
As a non-native, I often find this
https://trends.google.com/trends/explore?q=consequentially,consequently&hl=en
fairly useful.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-10 16:22 UTC (permalink / raw)
To: Dragan Simic; +Cc: Rubén Justo, Jeff King, Git List
In-Reply-To: <a97b03a305a7b8b95341b63af1de0271@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> No worries. Regarding disabling the advices for disabling the advice
> messages, maybe it would be better to have only one configuration knob
> for that purpose, e.g. "core.verboseAdvice", as a boolean knob.
I am not sure if you understood Peff's example that illustrates why
it is a bad thing, as ...
> That
> way, fishing for the right knob for some advice message wouldn't turn
> itself into an issue,
... this is exactly what a single core.verboseAdvice knob that
squelches the "how to disable this particular advice message" part
from the message is problematic. Before you see and familialize
yourself with all advice messages, you may see one piece of advice X
and find it useful to keep, feeling no need to turn it off. If you
use that single knob to squelch the part to tell you how to turn
advice X off. But what happens when you hit another unrelated
advice Y then? Because your core.verboseAdvice is a single big red
button, the message does not say which advice.* variable to tweak
for this particular advice message Y.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-10 16:14 UTC (permalink / raw)
To: Jeff King; +Cc: Rubén Justo, Git List
In-Reply-To: <20240110110226.GC16674@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> If I'm reading your patch correctly, this is a single option that
> controls the extra line for _all_ advice messages. But I'd have expected
> this to be something you'd want to set on a per-message basis. Here's my
> thinking.
>
> The original idea for advice messages was that they might be verbose and
> annoying, but if you had one that showed up a lot you'd choose to shut
> it up individually. But you wouldn't do so for _all_ messages, because
> you might benefit from seeing others (including new ones that get
> added). The "Disable this..." part was added later to help you easily
> know which config option to tweak.
>
> The expectation was that you'd fall into one of two categories:
>
> 1. You don't see the message often enough to care, so you do nothing.
>
> 2. You do find it annoying, so you disable this instance.
>
> Your series proposes a third state:
>
> 3. You find the actual hint useful, but the verbosity of "how to shut
> it up" is too much for you.
>
> That make sense to me, along with being able to partially shut-up a
> message. But wouldn't you still need the "how to shut up" hint for
> _other_ messages, since it's customized for each situation?
Thanks for saying what I wanted to say in my one of the messages
much clearly than I could. The above is exactly why I would be more
sympathetic to "advice.foo = (yes/no/always)".
^ permalink raw reply
* Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists
From: Toon Claes @ 2024-01-10 15:20 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cTRHHJ3pzWJtVJf8rKhvAJFYqrO0JsyTRTi6T5s+gznDg@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> It may not be worth a reroll, but I found the explanation you gave in
> the cover letter more illuminating than what is written above for
> explaining why this change is desirable. In particular, the discussion
> of the reftable backend was very helpful.
Well, I wasn't sure the explanation would be relevant in the present,
because the reftable backend might happen relatively far into the
future.
--
Toon
^ permalink raw reply
* [PATCH] branch: error description when deleting a not fully merged branch
From: Rubén Justo @ 2024-01-10 14:55 UTC (permalink / raw)
To: Git List
The error message we show when the user tries to delete a not fully
merged branch describes the error and gives a hint to the user:
error: the branch 'foo' is not fully merged.
If you are sure you want to delete it, run 'git branch -D foo'.
Let's move the hint part so that it takes advantage of the advice
machinery:
error: the branch 'foo' is not fully merged
hint: If you are sure you want to delete it, run 'git branch -D foo'
hint: Disable this message with "git config advice.forceDeleteBranch false"
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
This change is a pending NEEDSWORK from a recent series about adjusting
the error messages in branch.c
Unfortunately the full message now becomes a three line message.
Hopefully we can find a way in the near future to keep it at two.
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 3 ++-
builtin/branch.c | 9 ++++++---
4 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 4d7e5d8759..5814d659b9 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -142,4 +142,7 @@ advice.*::
Advice shown when a user tries to create a worktree from an
invalid reference, to instruct how to create a new unborn
branch instead.
+ forceDeleteBranch::
+ Advice shown when a user tries to delete a not fully merged
+ branch without the force option set.
--
diff --git a/advice.c b/advice.c
index 50c79443ba..e91f5d7ab8 100644
--- a/advice.c
+++ b/advice.c
@@ -79,6 +79,7 @@ static struct {
[ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
[ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
[ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
+ [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 },
};
static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index 2affbe1426..5bef900142 100644
--- a/advice.h
+++ b/advice.h
@@ -10,7 +10,7 @@ struct string_list;
* Add the new config variable to Documentation/config/advice.txt.
* Call advise_if_enabled to print your advice.
*/
- enum advice_type {
+enum advice_type {
ADVICE_ADD_EMBEDDED_REPO,
ADVICE_ADD_EMPTY_PATHSPEC,
ADVICE_ADD_IGNORED_FILE,
@@ -50,6 +50,7 @@ struct string_list;
ADVICE_WAITING_FOR_EDITOR,
ADVICE_SKIPPED_CHERRY_PICKS,
ADVICE_WORKTREE_ADD_ORPHAN,
+ ADVICE_FORCE_DELETE_BRANCH,
};
int git_default_advice_config(const char *var, const char *value);
diff --git a/builtin/branch.c b/builtin/branch.c
index 0a32d1b6c8..2240433bc8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -24,6 +24,7 @@
#include "ref-filter.h"
#include "worktree.h"
#include "help.h"
+#include "advice.h"
#include "commit-reach.h"
static const char * const builtin_branch_usage[] = {
@@ -190,9 +191,11 @@ static int check_branch_commit(const char *branchname, const char *refname,
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("the branch '%s' is not fully merged.\n"
- "If you are sure you want to delete it, "
- "run 'git branch -D %s'"), branchname, branchname);
+ error(_("the branch '%s' is not fully merged"),
+ branchname);
+ advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
+ _("If you are sure you want to delete it, "
+ "run 'git branch -D %s'"), branchname);
return -1;
}
return 0;
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Dragan Simic @ 2024-01-10 14:44 UTC (permalink / raw)
To: Rubén Justo; +Cc: Jeff King, Git List
In-Reply-To: <97fdf6d4-1403-4fe9-b912-a85342a9fa56@gmail.com>
On 2024-01-10 15:32, Rubén Justo wrote:
> On 10-ene-2024 15:18:17, Dragan Simic wrote:
>> On 2024-01-10 12:02, Jeff King wrote:
>
>> Just to chime in and support this behavior of the advice messages.
>> Basically, you don't want to have them all disabled at the same time,
>> but to
>> have per-message enable/disable granularity. I'd guess that some of
>> the
>> messages are quite usable even to highly experienced users, and
>> encountering
>> newly added messages is also very useful. Thus, having them all
>> disabled
>> wouldn't be the best idea.
>
> Totally agree.
>
> This series is about disabling _the part in the advice about how to
> disable the advice_, but not the proper advice.
>
> Maybe the name ADVICE_ADVICE_OFF has caused confusion. Sorry if so.
No worries. Regarding disabling the advices for disabling the advice
messages, maybe it would be better to have only one configuration knob
for that purpose, e.g. "core.verboseAdvice", as a boolean knob. That
way, fishing for the right knob for some advice message wouldn't turn
itself into an issue, and the users would be able to turn the additional
"advices about advices" on and off easily, to be able to see what knobs
actually turn off the advice messages that have become annoying enough
to them.
^ permalink raw reply
* Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists
From: Eric Sunshine @ 2024-01-10 14:36 UTC (permalink / raw)
To: Toon Claes; +Cc: git
In-Reply-To: <20240110141559.387815-2-toon@iotcl.com>
On Wed, Jan 10, 2024 at 9:19 AM Toon Claes <toon@iotcl.com> wrote:
> Recently [1] the option --exists was added to git-show-ref(1). When you
> use this option against a ref that doesn't exist, but it is a parent
> directory of an existing ref, you're getting the following error:
>
> $ git show-ref --exists refs/heads
> error: failed to look up reference: Is a directory
>
> This error is confusing to the user. Instead, print the same error as
> when the ref was not found:
>
> error: reference does not exist
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
It may not be worth a reroll, but I found the explanation you gave in
the cover letter more illuminating than what is written above for
explaining why this change is desirable. In particular, the discussion
of the reftable backend was very helpful.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-10 14:32 UTC (permalink / raw)
To: Dragan Simic, Jeff King; +Cc: Git List
In-Reply-To: <aaf59fc82ef3132ece8e1f70623570a2@manjaro.org>
On 10-ene-2024 15:18:17, Dragan Simic wrote:
> On 2024-01-10 12:02, Jeff King wrote:
> Just to chime in and support this behavior of the advice messages.
> Basically, you don't want to have them all disabled at the same time, but to
> have per-message enable/disable granularity. I'd guess that some of the
> messages are quite usable even to highly experienced users, and encountering
> newly added messages is also very useful. Thus, having them all disabled
> wouldn't be the best idea.
Totally agree.
This series is about disabling _the part in the advice about how to
disable the advice_, but not the proper advice.
Maybe the name ADVICE_ADVICE_OFF has caused confusion. Sorry if so.
^ permalink raw reply
* [PATCH 0/1] Fix error message in git-show-ref(1) --exists
From: Toon Claes @ 2024-01-10 14:15 UTC (permalink / raw)
To: git; +Cc: Toon Claes
References can validly contain forward slashes in their name. With the ref files
backend, these are stored as a directory tree. This means when you look up a
reference, you might find a directory where you expected a file.
This causes the option --exists, recently added to git-show-ref(1), to return
the following error:
error: failed to look up reference: Is a directory
Other backends, like reftables, might store refs with forward slashes
differently. So they will not encounter the same error.
To make the error consistent across refs backend implementations, and to be more
clear to user, hide the error about having found a directory as a generic error:
error: reference does not exist
Toon Claes (1):
builtin/show-ref: treat directory directory as non-existing in
--exists
builtin/show-ref.c | 2 +-
t/t1403-show-ref.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
2.42.1
^ 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