* 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
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Dragan Simic @ 2024-01-10 14:18 UTC (permalink / raw)
To: Jeff King; +Cc: Rubén Justo, Git List
In-Reply-To: <20240110110226.GC16674@coredump.intra.peff.net>
On 2024-01-10 12:02, Jeff King wrote:
> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
>
>> Using advise_if_enabled() to display an advice will automatically
>> include instructions on how to disable the advice, along with the
>> main advice:
>>
>> hint: use --reapply-cherry-picks to include skipped commits
>> hint: Disable this message with "git config advice.skippedCherryPicks
>> false"
>>
>> This can become distracting or noisy over time, while the user may
>> still want to receive the main advice.
>>
>> Let's have a switch to allow disabling this automatic advice.
>
> 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.
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.
> 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?
>
> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,
> you run "git config advice.adviseOff false".
>
> But now you run into another hint, like:
>
> $ git foo
> hint: you can use --empty-commits to deal with isn't as good as
> --xyzzy
>
> and you want to disable it entirely. Which advice.* config option does
> so? You're stuck trying to find it in the manpage (which is tedious but
> also kind of tricky since you're now guessing which name goes with
> which
> message). You probably do want:
>
> hint: Disable this message with "git config advice.xyzzy false"
>
> in that case (at which point you may decide to silence it completely or
> partially).
>
> Which implies to me that the value of advice.* should be a tri-state to
> match the cases above: true, false, or a "minimal" / "quiet" mode
> (there
> might be a better name). And then you'd do:
>
> git config advice.skippedCherryPicks minimal
>
> (or whatever it is called) to get the mode you want, without affecting
> advice.xyzzy.
>
>> advice.c | 3 ++-
>> advice.h | 3 ++-
>> t/t0018-advice.sh | 8 ++++++++
>> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> Speaking of manpages, we'd presumably need an update to
> Documentation/config/advice.txt. :)
>
> -Peff
^ permalink raw reply
* [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists
From: Toon Claes @ 2024-01-10 14:15 UTC (permalink / raw)
To: git; +Cc: Toon Claes
In-Reply-To: <20240110141559.387815-1-toon@iotcl.com>
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
[1]: 9080a7f178 (builtin/show-ref: add new mode to check for reference
existence, 2023-10-31)
Signed-off-by: Toon Claes <toon@iotcl.com>
---
builtin/show-ref.c | 2 +-
t/t1403-show-ref.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index aaa2c39b2f..79955c2856 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -238,7 +238,7 @@ static int cmd_show_ref__exists(const char **refs)
if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
&unused_oid, &unused_referent, &unused_type,
&failure_errno)) {
- if (failure_errno == ENOENT) {
+ if (failure_errno == ENOENT || failure_errno == EISDIR) {
error(_("reference does not exist"));
ret = 2;
} else {
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index ec1957b709..d0a8f7b121 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -262,9 +262,9 @@ test_expect_success '--exists with non-commit object' '
test_expect_success '--exists with directory fails with generic error' '
cat >expect <<-EOF &&
- error: failed to look up reference: Is a directory
+ error: reference does not exist
EOF
- test_expect_code 1 git show-ref --exists refs/heads 2>err &&
+ test_expect_code 2 git show-ref --exists refs/heads 2>err &&
test_cmp expect err
'
--
2.42.1
^ permalink raw reply related
* [PATCH] t4129: prevent loss of exit code due to the use of pipes
From: Chandra Pratap via GitGitGadget @ 2024-01-10 12:54 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
From: Chandra Pratap <chandrapratap3519@gmail.com>
Piping the output of git commands like git-ls-files to another
command (grep in this case) hides the exit code returned by
these commands. Prevent this by storing the output of git-ls-files
to a temporary file and then "grep-ping" from that file. Replace
grep with test_grep as the latter is more verbose when it fails.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
t4129: prevent loss of exit code due to the use of pipes
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1636%2FChand-ra%2Ftest_pipe-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1636/Chand-ra/test_pipe-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1636
t/t4129-apply-samemode.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
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 &&
+ test_grep "^100755" ls-files-output
'
test_expect_success FILEMODE 'mode update (no index)' '
@@ -60,7 +61,8 @@ test_expect_success FILEMODE 'mode update (with index)' '
test_expect_success FILEMODE 'mode update (index only)' '
git reset --hard &&
git apply --cached patch-1.txt &&
- git ls-files -s file | grep "^100755"
+ git ls-files -s file > ls-files-output &&
+ test_grep "^100755" ls-files-output
'
test_expect_success FILEMODE 'empty mode is rejected' '
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 0/3] allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-10 12:40 UTC (permalink / raw)
To: Junio C Hamano, Taylor Blau, Jeff King; +Cc: Git List
In-Reply-To: <xmqq8r4yf897.fsf@gitster.g>
On 09-ene-2024 14:32:20, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> that configuration knob is probably misnamed, as you
> pointed out).
Agree. Maybe we have a better name: "showDisableInstructions".
> I would understand if the proposed change were to change the
> "advice.<key>" configuration variable from a Boolean to a tristate
> (yes = default, no = disabled, always = give advice without
> instruction), though. IOW, the message might look like so:
>
> hint: use --reapply-cherry-picks to include skipped commits
> hint: setting advice.skippedCherryPicks to 'false' disables this message
> hint: and setting it to 'always' removes this additional instruction.
That's an interesting twist ... and intuitive it seems, as Peff also
came up with a similar approach.
But do we need, or want, that fine grain?
Using advise_if_enabled() allows us to display a hint while
automatically adding the option to disable it, _explicitly_ (so far*),
to the user.
But it happens that advise_if_enabled() itself has a hint to give.
My goal in this series is about giving the user the possibility to
disable _that_ hint (in the hint).
The user choosing to disable that hint is telling us: "I know I can
disable a hint, stop telling me so, please". So I don't think
this opens up a new risk or problem finding how to disable a hint.
$ git -c advice.showDisableInstructions=1 command_with_a_no_longer_welcomed_hint
Is there a need to make that "hint in the hint" customizable _per hint_?
That probably means that a new "make-it-always-for-all" may be desirable
alongside the new tristate yes-no-always ...
I dunno.
* perhaps this multi-value possibility could be a path
to explore what Taylor outlined in another message in this thread:
the possibility of a "one-shot" hint.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-10 12:11 UTC (permalink / raw)
To: Taylor Blau; +Cc: Git List, Junio C Hamano, Jeff King
In-Reply-To: <ZZ2QGYBBmK8cSYBD@nand.local>
On 09-ene-2024 13:27:37, Taylor Blau wrote:
> > + [ADVICE_ADVICE_OFF] = { "adviceOff", 1 },
>
> The name seems to imply that setting `advice.adviceOff=true` would cause
> Git to suppress the turn-off instructions. But...
>
> > };
> >
> > static const char turn_off_instructions[] =
> > @@ -93,7 +94,7 @@ static void vadvise(const char *advice, int display_instructions,
> >
> > strbuf_vaddf(&buf, advice, params);
> >
> > - if (display_instructions)
> > + if (display_instructions && advice_enabled(ADVICE_ADVICE_OFF))
> > strbuf_addf(&buf, turn_off_instructions, key);
>
> ...it looks like the opposite is true. I guess the "adviceOff" part of
> this new configuration option suggests "show me advice on how to turn
> off advice of xyz kind in the future".
>
> Perhaps a clearer alternative might be "advice.showDisableInstructions"
> or something?
Yeah. We can then use ADVICE_SHOW_DISABLE_INSTRUCTIONS, which is also a
better name than the current ADVICE_ADVICE_OFF. Thanks.
I'll reroll also with a description of it in
Documentation/config/advice.txt, as Jeff has pointed out.
> I don't know, coming up with a direct/clear name of this
> configuration is challenging for me.
Well ... I'm not going to show the previous names I've been considering
for this ;-)
>
> >
> > for (cp = buf.buf; *cp; cp = np) {
> > diff --git a/advice.h b/advice.h
> > index 2affbe1426..1f2eef034e 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_ADVICE_OFF,
> > };
> >
> > int git_default_advice_config(const char *var, const char *value);
> > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> > index c13057a4ca..0b6a8b4a10 100755
> > --- a/t/t0018-advice.sh
> > +++ b/t/t0018-advice.sh
> > @@ -30,4 +30,12 @@ test_expect_success 'advice should not be printed when config variable is set to
> > test_must_be_empty actual
> > '
> >
> > +test_expect_success 'advice without the instructions to disable it' '
> > + cat >expect <<-\EOF &&
> > + hint: This is a piece of advice
> > + EOF
> > + test-tool -c advice.adviceOff=0 advise "This is a piece of advice" 2>actual &&
> > + test_cmp expect actual
> > +'
>
> Looking at this test, I wonder why we don't imitate the existing style
> of:
>
> test_config advice.adviceOff false &&
> test-tool advise "This is a piece of advice" 2>actual &&
> test_cmp expect actual
As a reference, implicitly, that is:
git config advice.adviceOff false &&
test_when_finished "git config --unset-all advice.adviceOff" &&
test-tool advise "This is a piece of advice" 2>actual &&
test_cmp expect actual
I think the proposed test is better and understandable enough.
As a matter of fact, when I was thinking how to write the test I was
expecting to have a working "-c" in test-tool, as if we have a (not
expected) "git-advise".
So ...
>
> instead of teaching the test-tool helpers how to interpret `-c`
> arguments. Doing so would allow us to drop the first couple of patches
> in this series and simplify things a bit.
... IMHO the first two patches allows us to write simpler and more
intuitive tests based on test-tool.
I hope this argument persuades you, but I am not against your proposal.
>
> Thanks,
> Taylor
Thank you for reviewing this series.
^ permalink raw reply
* Re: Limited operations in unsafe repositories
From: Jeff King @ 2024-01-10 12:05 UTC (permalink / raw)
To: brian m. carlson; +Cc: git
In-Reply-To: <ZZr-JLxubCvWe0EU@tapette.crustytoothpaste.net>
On Sun, Jan 07, 2024 at 07:40:20PM +0000, brian m. carlson wrote:
> I had looked at sending a patch to make `git rev-parse` operate in a
> special mode where it's impossible to invoke any binaries at all, but
> unfortunately, `get_superproject_working_tree` invokes binaries, so
> that's not possible. (If anyone is interested in picking this up, there
> is a start on it, failing many tests, in the `rev-parse-safe-directory`
> on my GitHub remote.)
>
> I guess I'm looking for us to provide some basic functionality that is
> guaranteed to work in this case, including `git rev-parse` and `git
> config -l`. I don't think it's useful for every program that wants to
> work with Git to need to implement its own repository discovery and
> config parsing, and those are essential needs for tooling that needs to
> work with untrusted repositories.
If I understand correctly, you want to have some very limited code paths
that we know are OK to run even in an untrusted repo. My concern there
would be that it's easy for "basic functionality" to accidentally call
into less-limited code, which suddenly becomes a vulnerability.
My thinking is to flip that around: run all code, but put protection in
the spots that do unsafe things, like loading config or examining
hooks. I.e., a patch like this:
diff --git a/config.c b/config.c
index 9ff6ae1cb9..c7bbd6bdda 100644
--- a/config.c
+++ b/config.c
@@ -2026,7 +2026,7 @@ static int do_git_config_sequence(const struct config_options *opts,
if (!opts->git_dir != !opts->commondir)
BUG("only one of commondir and git_dir is non-NULL");
- if (opts->commondir) {
+ if (opts->commondir && is_safe_repository())
repo_config = mkpathdup("%s/config", opts->commondir);
worktree_config = mkpathdup("%s/config.worktree", opts->git_dir);
} else {
diff --git a/hook.c b/hook.c
index f6306d72b3..4fcfd82dc5 100644
--- a/hook.c
+++ b/hook.c
@@ -12,6 +12,9 @@ const char *find_hook(const char *name)
{
static struct strbuf path = STRBUF_INIT;
+ if (!is_safe_repository())
+ return NULL;
+
strbuf_reset(&path);
strbuf_git_path(&path, "hooks/%s", name);
if (access(path.buf, X_OK) < 0) {
where is_safe_repository() is something like:
- if git_env_bool("GIT_ASSUME_SAFE", 1) is true, return true
immediately
- otherwise, see if we are matched by safe.directory
And then running:
git --assume-unsafe rev-parse ...
would set that variable to "0".
And then most functionality would just work, modulo trusting repo hooks
and config. I mentioned "loading config" earlier, but of course that
patch is just touching the usual "load all config sources" code. We'd
still allow parsing .git/config for repo discovery, and something like:
git --assume-unsafe config -l --local
would still work if the caller knew they would be careful with the
output.
The downside, of course, is that we might fail to include other
dangerous spots. Those two are the big ones, I think, but would we want
to prevent people from pointing to /dev/mem in info/alternates? I dunno.
It certainly is more protection than we offer today, but maybe saying
"this is safe" would produce a false sense of security.
I do think something like git-prompt.sh could benefit here (it could use
--assume-unsafe for all invocations so you don't get surprised just by
chdir-ing into a downloaded tarball).
-Peff
^ permalink raw reply related
* Re: [PATCH] index-pack: spawn threads atomically
From: Jeff King @ 2024-01-10 11:44 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZZgvUyQK6X/MacDC@nand.local>
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.
If do go with "--threads=1", I suspect several tests in that file need
it.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-10 11:39 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20240110110226.GC16674@coredump.intra.peff.net>
On 10-ene-2024 06:02:26, Jeff King wrote:
> On Tue, Jan 09, 2024 at 04:30:16PM +0100, Rubén Justo wrote:
>
> > Using advise_if_enabled() to display an advice will automatically
> > include instructions on how to disable the advice, along with the
> > main advice:
> >
> > hint: use --reapply-cherry-picks to include skipped commits
> > hint: Disable this message with "git config advice.skippedCherryPicks false"
> >
> > This can become distracting or noisy over time, while the user may
> > still want to receive the main advice.
> >
> > Let's have a switch to allow disabling this automatic advice.
>
> 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?
>
> E.g., suppose that after getting annoyed at advice.skippedCherryPicks,
> you run "git config advice.adviseOff false".
>
> But now you run into another hint, like:
>
> $ git foo
> hint: you can use --empty-commits to deal with isn't as good as --xyzzy
>
> and you want to disable it entirely. Which advice.* config option does
> so? You're stuck trying to find it in the manpage (which is tedious but
> also kind of tricky since you're now guessing which name goes with which
> message). You probably do want:
>
> hint: Disable this message with "git config advice.xyzzy false"
>
> in that case (at which point you may decide to silence it completely or
> partially).
>
> Which implies to me that the value of advice.* should be a tri-state to
> match the cases above: true, false, or a "minimal" / "quiet" mode (there
> might be a better name). And then you'd do:
>
> git config advice.skippedCherryPicks minimal
>
> (or whatever it is called) to get the mode you want, without affecting
> advice.xyzzy.
Your reasoning is sensible, but I'm not sure if we want that level of
granularity. My guts doesn't feel it, though I'm not opposed.
In the message before yours in this thread, Junio proposed a similar
approach, and I've been thinking about it. Let me answer to your
comments from that message too.
>
> > advice.c | 3 ++-
> > advice.h | 3 ++-
> > t/t0018-advice.sh | 8 ++++++++
> > 3 files changed, 12 insertions(+), 2 deletions(-)
>
> Speaking of manpages, we'd presumably need an update to
> Documentation/config/advice.txt. :)
This has made me jump first to this message in the thread ... I missed
this. Thank you!
>
> -Peff
^ permalink raw reply
* Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph
From: Jeff King @ 2024-01-10 11:39 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Scott Leggett
In-Reply-To: <ZZg3YIEDCjbbGyVX@nand.local>
On Fri, Jan 05, 2024 at 12:07:44PM -0500, Taylor Blau wrote:
> > I do think there's some fragility here that we might want to follow up
> > on. We have a global commit_graph_data_slab that contains graph
> > positions, and our global commit structs depend on the that slab
> > remaining valid. But close_commit_graph() is just about closing _one_
> > object store's graph. So it's dangerous to call that function and clear
> > the slab without also throwing away any "struct commit" we might have
> > parsed that depends on it.
>
> I definitely agree that there seems like a high risk of another
> potentially-dangerous bug happening as a result of this area.
>
> One thing that sticks out to me is that we have a scope mismatch
> between the commit structs, the raw_object_store, and the commit slabs.
> Slabs are tied to the lifetime of the raw_object_store, but the commit
> objects are tied to the global lifetime of the process. I wonder if it
> would make sense to have a slab per object-store, and require that you
> pass both the commit *and* the object-store you're looking it up in as
> arguments to any slab-related functions.
Right, that scope mismatch is the crux of the issue. I had imagined that
if you're really closing the object database you might want to just
throw away all of the object structs. But we don't have a way to do that
(and in general I'd be slightly scared if we did, since lots of code
likes to think of object pointers as immutable).
I don't know if your solution works, though. If I "partially" load a
commit (i.e., its "parsed" flag is 1, but maybe_tree is still NULL, not
having been loaded from the graph yet), then that commit object depends
on the slab. If we later ask the commit about its tree, then it needs to
refer to the slab to see if it came from the graph. Right now that
happens in the global slab. If we did it in a per-object-store slab,
we'd need each commit to know which object store it came from!
Of course we do have a "struct repository" argument in
repo_get_commit_tree(). So...maybe that's enough? It still doesn't get
us all the way there, though. In this case the culprit was submodules,
and we really would be closing an alternate repo. But we have actual
calls to close_commit_graph() sprinkled around. Any code which closes
any commit-graph and then still accesses a "struct commit" is
potentially buggy.
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).
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.
So yet another alternative is: don't clear the slab. Mark the entries as
"this came from a graph, but we don't know the correct graph position
anymore" (presumably with a new sentinel value for data->graph_pos). And
then functions like repo_get_commit_tree() would know that they need to
try harder (e.g., doing the unparse then and going back to the object
store to parse again).
Hmm. Actually, maybe COMMIT_NOT_FROM_GRAPH already functions that way.
If we see a commit that is parsed but has a NULL maybe_tree, then if we
see COMMIT_NOT_FROM_GRAPH we assume it's just a corrupt commit and
return NULL. But we could just try harder to re-parse it then. Like:
diff --git a/commit.c b/commit.c
index ef679a0b93..909898cf7f 100644
--- a/commit.c
+++ b/commit.c
@@ -423,6 +423,24 @@ struct tree *repo_get_commit_tree(struct repository *r,
if (commit_graph_position(commit) != COMMIT_NOT_FROM_GRAPH)
return get_commit_tree_in_graph(r, commit);
+ /*
+ * This is either a corrupt commit, or one which we partially loaded
+ * from a graph file but then subsequently threw away the graph data.
+ *
+ * Optimistically assume it's the latter and try to reload from
+ * scratch. This gives a performance penalty if it really is a corrupt
+ * commit, but presumably that happens rarely.
+ *
+ * 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).
+ *
+ * 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.
I'm content to leave it for now and let this discussion serve as a
reference if somebody does manage to run into it in practice.
-Peff
^ permalink raw reply related
* Re: [GSOC][RFC] Add more builtin patterns for userdiff, as Mircroproject.
From: Christian Couder @ 2024-01-10 11:33 UTC (permalink / raw)
To: Sergius Nyah; +Cc: git, gitster@pobox.com
In-Reply-To: <CANAnif95ux=vCNCKbVw0q_vYamQRkbFqSa9_-u6xRvK6r+2a+Q@mail.gmail.com>
Hi Sergius,
On Tue, Jan 9, 2024 at 8:59 PM Sergius Nyah <sergiusnyah@gmail.com> wrote:
>
> Hello everyone,
> I'm Sergius, a Computer Science undergraduate student, and I want to
> begin Contributing to the Git project. So far, I've gone through
> Matheus' tutorial on First steps Contributing to Git, and I found it
> very helpful. I've also read the Contribution guidelines keenly and
> built Git from source.
Thanks for your interest in contributing to Git!
> In accordance to the contributor guidelines, I came across this
> Mircoproject idea from: https://git.github.io/SoC-2022-Microprojects/
s/Mircoproject/microproject/
There is a similar typo in the subject of your email too.
> which I'm willing to work on. It talked about enhancing Git's
> "userdiff" feature in "userdiff.c" which is crucial for identifying
> function names in various programming languages, thereby improving the
> readability of "git diff" outputs.
>
> From my understanding, the project involves extending the `userdiff`
> feature to support additional programming languages that are currently
> not covered such as Shell, Swift, Go and the others.
As far as I can see in userdiff.c, Golang and Bash seem to be supported.
> Here is a sample of how a language is defined in `userdiff.c`:
>
> > #define PATTERNS(lang, rx, wrx) { \
> > .name = lang, \
> > .binary = -1, \
> > .funcname = { \
> > .pattern = rx, \
> > .cflags = REG_EXTENDED, \
> > }, \
> > .word_regex = wrx "|[^[:space:]]|[\xc0-\xff][\x80-\xbf]+", \
> > .word_regex_multi_byte = wrx "|[^[:space:]]", \
> > }
>
> In this code, `lang` is the name of the language, `rx` is the regular
> expression for identifying function names, and `wrx` is the word
> regex.
>
> Approach: I Identified the Programming Languages that are not
> currently supported by the userdiff feature by reviewing the existing
> patterns in userdiff.c and comparing them with some popular
> programming languages.
> For each supported language, I would define a regular expression that
> could help identify function names in that language. This could
> include researching each language's syntax and testing their
> expressions to ensure that they work well.
In your microproject, you only need to add support for ONE language
that is not supported yet. Please don't try to do more than that.
> Also, I'd add a new IPATTERN definition for each language to the
> "userdiff.c" file, then rebuild Git and test the changes by creating a
> repo with files in the newly supported languages then run "git diff"
> to ensure the line @@ ... @@ produces their correct function names.
> Then submit a patch.
Except for my comments above, this looks like a good plan. Thanks.
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Jeff King @ 2024-01-10 11:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Haller, git
In-Reply-To: <xmqq34v7lmb3.fsf@gitster.g>
On Mon, Jan 08, 2024 at 10:20:00AM -0800, Junio C Hamano wrote:
> Stefan Haller <lists@haller-berlin.de> writes:
>
> > Our git client (lazygit) has a need to store per-repo config files that
> > override the global one, much like git itself. The easiest way to do
> > that is to store those in a .git/lazygit.cfg file, and I'm wondering if
> > there's any reason why this is a bad idea?
>
> An obvious alternative is to have .lazygit directory next to .git directory
> which would give you a bigger separation, which can cut both ways.
Just to spell out one of those ways: unlike ".git", we will happily
check out ".lazygit" from an untrusted remote repository. That may be a
feature if you want to be able to share project-specific config, or it
might be a terrible security vulnerability if lazygit config files can
trigger arbitrary code execution.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox