* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Stefan Haller @ 2024-03-06 21:00 UTC (permalink / raw)
To: Elijah Newren, Junio C Hamano
Cc: git, Derrick Stolee, Phillip Wood, Christian Couder
In-Reply-To: <CABPp-BGO2ftEMHJDrf6yg3J4AfpKn=rpf_5Wt_WAS+Hi70KqPQ@mail.gmail.com>
On 06.03.24 03:57, Elijah Newren wrote:
> 1) What if there is a branch that is "just a copy" of one of the
> branches earlier in the "stack"? Since it's "just a copy", shouldn't
> it be excluded for similar reasons to what you are arguing? And, if
> so, which branch is the copy?
This is a good point, but in my experience it's a lot more rare. Maybe
I'm looking at all this just from my own experience, and there might be
other usecases that are very different from mine, but as far as I am
concerned, copies of branches are not long-lived. There is no point in
having two branches point at the same commit. When I create a copy of a
branch, I do that only to rebase the copy somewhere else _immediately_,
leaving the original branch where it was. Which means that I encounter
copied branches only at the top of the stack, not in the middle. Which
means that I'm fine with keeping the current behavior of "rebase
--update-ref" to update both copies of that middle-of-the-stack branch,
because it never happens in practice for me.
> 2) Further, a "stack", to me at least, suggests a linear history
> without branching (i.e. each commit has at most one parent _and_ at
> most one child among the commits in the stack). I designed `git
> replay` to handle diverging histories (i.e. rebasing multiple branches
> that _might_ share a subset of common history but none necessarily
> need to fully contain the others, though perhaps the branches do share
> some other contained branches), and I want it to handle replaying
> merges as well. While `git rebase --update-refs` is absolutely
> limited to "stacks", and thus your argument might make sense in the
> context of `git rebase`, since you are bringing `git replay` into the
> mix, it needs to apply beyond a stack of commits. It's not clear to
> me how to genericize your suggestions to handle cases other than a
> simple stack of commits, though.
I don't see a contradiction here. I don't tend to do this in practice,
but I can totally imagine a tree of stacked branches that share some
common base branches in the beginning and then diverge into different
branches from there. It's true that "rebase --update-refs", when told to
rebase one of the leaf branches, will destroy this tree because it pulls
the base branches away from under the other leaf branches, but this is
unrelated to my proposal, it has this problem today already. And it's
awesome that git replay has a way to avoid this by rebasing the whole
tree at once, keeping everything intact. Still, I don't see what's bad
about excluding branches that point at the same commits as the leaf
branches it is told to rebase when using "replay --contains". (I suppose
what I'm suggesting is to treat "--contains" to mean "is included in the
half-open interval from base to tip" of the revision range you are
rebasing, rather than the closed interval.)
Maybe I should make this more explicit again: I'm not trying to solve
the problem of making a copy of a stack of branches, and rebasing that
copy somewhere else. I think this can't be solved except by making
branch stacks a new concept in git, which I'm not sure we want to do.
> 3) This is mostly covered in (1) and (2), but to be explicit: `git
> replay` is completely against the HEAD-is-special assumptions that are
> pervasive within `git rebase`, and your problem is entirely phrased as
> HEAD-is-special due to your call out of "the current branch". Is your
> argument limited to such special cases? (If so, it might still be
> valid for `git rebase`, of course.)
No, I don't think I need HEAD to be special. "The thing that I'm
rebasing" is special, and it is always HEAD for git rebase, but it can
be something else for replay.
> 4a) `git replay` does what Junio suggests naturally, since it doesn't
> update the refs but instead gives commands which can be fed to `git
> update-ref --stdin`. Thus, users can inspect the output of `git
> replay` and only perform the updates they want (by feeding a subset of
> the lines to update-ref --stdin).
At this point I probably need to explain that I'm rarely using the
command line. I'm a user and co-maintainer of lazygit, and I want to
make lazygit work in such a way that "it does the right thing" in as
many cases as possible.
> 4b) For `git replay`, --contained is just syntactic sugar -- it isn't
> necessary. git replay will allow you to list multiple branches that
> you want replayed, so you can specify which branches are relevant to
> you.
That's great, even if it means that I have to redo some of the work that
--contains would already do for me, just because I want a slightly
different behavior.
> 4c) For `git rebase --update-refs`, you can add `--interactive` and
> then delete the `update-ref` line(s) corresponding to the refs you
> don't want updated.
Yes, that's what I always do today to work around the problem. It's just
easy to forget, and I find it annoying that I have to take this extra
step every time.
One last remark: whenever I describe my use case involving copies of
branches, people tell me not to do that, use detached heads instead, or
other ways to achieve what I want. But then I don't understand why my
proposal would make a difference for them. If you don't use copied
branches, then why do you care whether "rebase --update-refs" or "replay
--contained" moves those copies or not? I still haven't heard a good
argument for why the current behavior is desirable, except for the one
example of a degenerate stack that Phillip Wood described in [1].
-Stefan
[1] <https://public-inbox.org/git/
98548a5b-7d30-543b-b943-fd48d8926a33@gmail.com/>
^ permalink raw reply
* [PATCH v2] userdiff: better method/property matching for C#
From: Steven Jeuris via GitGitGadget @ 2024-03-06 20:21 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Jeff King, Steven Jeuris,
Steven Jeuris
In-Reply-To: <pull.1682.git.git.1708882423691.gitgitgadget@gmail.com>
From: Steven Jeuris <steven.jeuris@3shape.com>
- Support multi-line methods by not requiring closing parenthesis.
- Support multiple generics (comma was missing before).
- Add missing `foreach`, `lock` and `fixed` keywords to skip over.
- Remove `instanceof` keyword, which isn't C#.
- Also detect non-method keywords not positioned at the start of a line.
- Added tests; none existed before.
The overall strategy is to focus more on what isn't expected for
method/property definitions, instead of what is, but is fully optional.
Signed-off-by: Steven Jeuris <steven.jeuris@gmail.com>
---
userdiff: better method/property matching for C#
Change since v1: I removed "from" from the list of keywords to skip.
First, I considered adding "await", but I discovered both "await" and
"from" are "contextual keywords", which unlike the other keywords
currently listed, aren't reserved, and can thus cause false negatives.
I.e., it is valid to have a method named "await" or "from". In edge
cases, this may lead to false positives, but a different exclusion rule
will need to be added to handle these.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1682%2FWhathecode%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1682/Whathecode/master-v2
Pull-Request: https://github.com/git/git/pull/1682
Range-diff vs v1:
1: cdd8dd4d871 ! 1: 00315519014 userdiff: better method/property matching for C#
@@ Commit message
- Support multi-line methods by not requiring closing parenthesis.
- Support multiple generics (comma was missing before).
- - Add missing `foreach`, `from`, `lock` and `fixed` keywords to skip over.
+ - Add missing `foreach`, `lock` and `fixed` keywords to skip over.
- Remove `instanceof` keyword, which isn't C#.
- Also detect non-method keywords not positioned at the start of a line.
- Added tests; none existed before.
@@ t/t4018/csharp-method-skip-body (new)
+ {
+ }
+ int[] numbers = [5, 4, 1, 3, 9, 8, 6, 7, 2, 0];
-+ var test =
-+ from num in Numbers(
-+ ) select num;
+
+ // Control
+ if (false)
@@ userdiff.c: PATTERNS("cpp",
+ * Jump over keywords not used by methods which can be followed by parentheses without special characters in between,
+ * making them look like methods.
+ */
-+ "!(^|[ \t]+)(do|while|for|foreach|from|if|else|new|default|return|switch|case|throw|catch|using|lock|fixed)([ \t(]+|$)\n"
++ "!(^|[ \t]+)(do|while|for|foreach|if|else|new|default|return|switch|case|throw|catch|using|lock|fixed)([ \t(]+|$)\n"
+ /* Methods/constructors:
+ * the strategy is to identify a minimum of two groups (any combination of keywords/type/name),
+ * without intermediate or final characters which can't be part of method definitions before the opening parenthesis.
t/t4018/csharp-method | 10 +++
t/t4018/csharp-method-explicit | 12 +++
t/t4018/csharp-method-generics | 11 +++
t/t4018/csharp-method-modifiers | 13 ++++
t/t4018/csharp-method-multiline | 10 +++
t/t4018/csharp-method-params | 10 +++
t/t4018/csharp-method-skip-body | 112 ++++++++++++++++++++++++++++
t/t4018/csharp-method-special-chars | 11 +++
t/t4018/csharp-method-with-spacing | 10 +++
t/t4018/csharp-property | 11 +++
userdiff.c | 16 ++--
11 files changed, 221 insertions(+), 5 deletions(-)
create mode 100644 t/t4018/csharp-method
create mode 100644 t/t4018/csharp-method-explicit
create mode 100644 t/t4018/csharp-method-generics
create mode 100644 t/t4018/csharp-method-modifiers
create mode 100644 t/t4018/csharp-method-multiline
create mode 100644 t/t4018/csharp-method-params
create mode 100644 t/t4018/csharp-method-skip-body
create mode 100644 t/t4018/csharp-method-special-chars
create mode 100644 t/t4018/csharp-method-with-spacing
create mode 100644 t/t4018/csharp-property
diff --git a/t/t4018/csharp-method b/t/t4018/csharp-method
new file mode 100644
index 00000000000..85ff0cb8b5b
--- /dev/null
+++ b/t/t4018/csharp-method
@@ -0,0 +1,10 @@
+class Example
+{
+ string Method(int RIGHT)
+ {
+ // Filler
+ // Filler
+
+ return "ChangeMe";
+ }
+}
diff --git a/t/t4018/csharp-method-explicit b/t/t4018/csharp-method-explicit
new file mode 100644
index 00000000000..083aa094ce2
--- /dev/null
+++ b/t/t4018/csharp-method-explicit
@@ -0,0 +1,12 @@
+using System;
+
+class Example : IDisposable
+{
+ void IDisposable.Dispose() // RIGHT
+ {
+ // Filler
+ // Filler
+
+ // ChangeMe
+ }
+}
diff --git a/t/t4018/csharp-method-generics b/t/t4018/csharp-method-generics
new file mode 100644
index 00000000000..c472d4a18df
--- /dev/null
+++ b/t/t4018/csharp-method-generics
@@ -0,0 +1,11 @@
+class Example<T1, T2>
+{
+ Example<int, string> Method<TA, TB>(TA RIGHT, TB b)
+ {
+ // Filler
+ // Filler
+
+ // ChangeMe
+ return null;
+ }
+}
diff --git a/t/t4018/csharp-method-modifiers b/t/t4018/csharp-method-modifiers
new file mode 100644
index 00000000000..f1c008a4749
--- /dev/null
+++ b/t/t4018/csharp-method-modifiers
@@ -0,0 +1,13 @@
+using System.Threading.Tasks;
+
+class Example
+{
+ static internal async Task Method(int RIGHT)
+ {
+ // Filler
+ // Filler
+
+ // ChangeMe
+ await Task.Delay(1);
+ }
+}
diff --git a/t/t4018/csharp-method-multiline b/t/t4018/csharp-method-multiline
new file mode 100644
index 00000000000..0a20b0cb49c
--- /dev/null
+++ b/t/t4018/csharp-method-multiline
@@ -0,0 +1,10 @@
+class Example
+{
+ string Method_RIGHT(
+ int a,
+ int b,
+ int c)
+ {
+ return "ChangeMe";
+ }
+}
diff --git a/t/t4018/csharp-method-params b/t/t4018/csharp-method-params
new file mode 100644
index 00000000000..18598449008
--- /dev/null
+++ b/t/t4018/csharp-method-params
@@ -0,0 +1,10 @@
+class Example
+{
+ string Method(int RIGHT, int b, int c = 42)
+ {
+ // Filler
+ // Filler
+
+ return "ChangeMe";
+ }
+}
diff --git a/t/t4018/csharp-method-skip-body b/t/t4018/csharp-method-skip-body
new file mode 100644
index 00000000000..c8c9621634d
--- /dev/null
+++ b/t/t4018/csharp-method-skip-body
@@ -0,0 +1,112 @@
+using System.Linq;
+using System;
+
+class Example : IDisposable
+{
+ string Method(int RIGHT)
+ {
+ // Method calls
+ MethodCall();
+ MethodCall(1, 2);
+ MethodCall(
+ 1, 2);
+
+ // Assignments
+ var constantAssignment = "test";
+ var methodAssignment = MethodCall();
+ var multiLineMethodAssignment = MethodCall(
+ );
+
+ // Initializations/disposal
+ new Example();
+ new Example(
+ );
+ new Example { };
+ using (this)
+ {
+ }
+ var def =
+ this is default(
+ Example);
+
+ // Iteration statements
+ do { } while (true);
+ do MethodCall(
+ ); while (true);
+ while (true);
+ while (true) {
+ break;
+ }
+ for (int i = 0; i < 10; ++i)
+ {
+ }
+ foreach (int i in Enumerable.Range(0, 10))
+ {
+ }
+ int[] numbers = [5, 4, 1, 3, 9, 8, 6, 7, 2, 0];
+
+ // Control
+ if (false)
+ {
+ return "out";
+ }
+ else { }
+ if (true) MethodCall(
+ );
+ else MethodCall(
+ );
+ switch ("test")
+ {
+ case "one":
+ return MethodCall(
+ );
+ case "two":
+ break;
+ }
+ (int, int) tuple = (1, 4);
+ switch (tuple)
+ {
+ case (1, 4):
+ MethodCall();
+ }
+
+ // Exceptions
+ try
+ {
+ throw new Exception("fail");
+ }
+ catch (Exception)
+ {
+ }
+ finally
+ {
+ }
+ try { } catch (Exception) {}
+ try
+ {
+ throw GetException(
+ );
+ }
+ catch (Exception) { }
+
+ // Others
+ lock (this)
+ {
+ }
+ unsafe
+ {
+ byte[] bytes = [1, 2, 3];
+ fixed (byte* pointerToFirst = bytes)
+ {
+ }
+ }
+
+ return "ChangeMe";
+ }
+
+ public void Dispose() {}
+
+ string MethodCall(int a = 0, int b = 0) => "test";
+ Exception GetException() => new Exception("fail");
+ int[] Numbers() => [0, 1];
+}
diff --git a/t/t4018/csharp-method-special-chars b/t/t4018/csharp-method-special-chars
new file mode 100644
index 00000000000..ec3565fd000
--- /dev/null
+++ b/t/t4018/csharp-method-special-chars
@@ -0,0 +1,11 @@
+class @Some_Type
+{
+ @Some_Type @Method_With_Underscore(int RIGHT)
+ {
+ // Filler
+ // Filler
+
+ // ChangeMe
+ return new @Some_Type();
+ }
+}
diff --git a/t/t4018/csharp-method-with-spacing b/t/t4018/csharp-method-with-spacing
new file mode 100644
index 00000000000..4143929a711
--- /dev/null
+++ b/t/t4018/csharp-method-with-spacing
@@ -0,0 +1,10 @@
+class Example
+{
+ string Method ( int RIGHT )
+ {
+ // Filler
+ // Filler
+
+ return "ChangeMe";
+ }
+}
diff --git a/t/t4018/csharp-property b/t/t4018/csharp-property
new file mode 100644
index 00000000000..1792117f964
--- /dev/null
+++ b/t/t4018/csharp-property
@@ -0,0 +1,11 @@
+class Example
+{
+ public bool RIGHT
+ {
+ get { return true; }
+ set
+ {
+ // ChangeMe
+ }
+ }
+}
diff --git a/userdiff.c b/userdiff.c
index e399543823b..5a9e8a0ef55 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -89,12 +89,18 @@ PATTERNS("cpp",
"|\\.[0-9][0-9]*([Ee][-+]?[0-9]+)?[fFlL]?"
"|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->\\*?|\\.\\*|<=>"),
PATTERNS("csharp",
- /* Keywords */
- "!^[ \t]*(do|while|for|if|else|instanceof|new|return|switch|case|throw|catch|using)\n"
- /* Methods and constructors */
- "^[ \t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe|async)[ \t]+)*[][<>@.~_[:alnum:]]+[ \t]+[<>@._[:alnum:]]+[ \t]*\\(.*\\))[ \t]*$\n"
+ /*
+ * Jump over keywords not used by methods which can be followed by parentheses without special characters in between,
+ * making them look like methods.
+ */
+ "!(^|[ \t]+)(do|while|for|foreach|if|else|new|default|return|switch|case|throw|catch|using|lock|fixed)([ \t(]+|$)\n"
+ /* Methods/constructors:
+ * the strategy is to identify a minimum of two groups (any combination of keywords/type/name),
+ * without intermediate or final characters which can't be part of method definitions before the opening parenthesis.
+ */
+ "^[ \t]*(([][[:alnum:]@_<>.,]*[^=:{ \t][ \t]+[][[:alnum:]@_<>.,]*)+\\([^;]*)$\n"
/* Properties */
- "^[ \t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[ \t]+)*[][<>@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+)[ \t]*$\n"
+ "^[ \t]*((([][[:alnum:]@_<>.,]+)[ \t]+[][[:alnum:]@_]*)+[^=:;,()]*)$\n"
/* Type definitions */
"^[ \t]*(((static|public|internal|private|protected|new|unsafe|sealed|abstract|partial)[ \t]+)*(class|enum|interface|struct|record)[ \t]+.*)$\n"
/* Namespace */
base-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction
From: Justin Tobler @ 2024-03-06 19:57 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZehauDgP4L40QKcH@tanuki>
On 24/03/06 12:59PM, Patrick Steinhardt wrote:
> > Something not really related to this patch, but I noticed and had a
> > question about.
> >
> > If I'm understanding this correctly, when a newly compacted table is
> > empty, it becomes possible for a range of indexes to no longer exist
> > within the stack. If this occurs in the middle of the stack, future
> > compaction will likely combine the tables on either side and restore the
> > missing index range. If the empty table was at the end of the stack,
> > would this effectly reset the max index to something lower for future
> > tables written to the stack? If so, could this lead to issues with
> > separate concurrent table writes?
>
> Very good question indeed, but I think we should be fine here. This is
> mostly because concurrent writers will notice when "tables.list" has
> changed, and, if so, abort the transaction with an out-of-date error.
>
> A few scenarios with concurrent processes, one process which compacts
> the stack (C) and one which modifies it (M):
>
> - M acquires the lock before C compacts: M sees the whole stack and
> uses the latest update index to update it, resulting in a newly
> written table. When C then locks afterwards, it may decide to
> compact and drop some tables in the middle of the stack. This may
> lead to a gap in update indices, but this is fine.
>
> - M acquires the lock while C compacts: M sees the whole stack and
> uses the latest update index to update the stack. C then acquires
> the lock to write the merged tables, notices that its compacted
> tables still exist and are in the same order, and thus removes them.
> We now have a gap in update indices, but this is totally fine.
>
> - M acquires the lock after C compacts: M will refresh "tables.list"
> after it has acquired the lock itself. Thus, it won't ever see the
> now-dropped empty table.
>
> M cannot write its table when C has the "tables.list" lock, so this
> scenario cannot happen. In the same spirit, two Ms cannot race with each
> other either as only one can have the "tables.list" lock, and the other
> one would abort with an out-of-date error when it has subsequently
> acquired the lock and found the "tables.list" contents to have been
> updated concurrently.
Thanks Patrick for the great explanation! Digging into this a bit
further, I see that we return `REFTABLE_LOCK_ERROR` when the list file
lock already exists or has changed when attempting to add a new table to
the list.
When performing compaction in `stack_compact_range()`, after initially
acquiring the table list lock, we also check if the stack is up-to-date
with `stack_uptodate()`. I noticed that this check is not performed
again after the table list is locked for the second time. At first I
thought this could be problematic, but I realized that this would only
be an issue for concurrent compactions and because the tables are locked
it should not matter.
-Justin
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: René Scharfe @ 2024-03-06 18:16 UTC (permalink / raw)
To: Christian Couder; +Cc: Josh Steadmon, git, Phillip Wood, Achu Luma
In-Reply-To: <CAP8UFD2M2+x-pRX2nzCXbLW=nKcW4_RWc9qua5q-fU8QbGu1oA@mail.gmail.com>
Am 04.03.24 um 11:00 schrieb Christian Couder:
> And anyway it doesn't make sense to have meaningful messages as second
> arguments to the TEST() macros if we always want to use a test harness
> that just discards them. Either:
>
> - we decide that we will always use some test harness, and then we
> might just want to remove that second argument and yeah we can have
> thousands of tests output lines from a single binary, or
> - we acknowledge that we don't always want to use a test harness,
> and then we want a relatively short and meaningful output from a
> single binary.
Different situations require different levels of detail. If all checks
pass, we just need that one bit of information. If some fail, we need
as much helpful context as we can get. Success messages are interesting
for someone who added a new test and as a progress indicator, but are
useless otherwise (to me at least).
Perhaps it would make sense to show ok message only after a delay if
writing to a terminal, similar to what progress.c does? This would
effectively silence them, since our current unit tests currently take
only a fraction of a second to finish.
René
^ permalink raw reply
* Re: [PATCH v3 00/14] FSMonitor edge cases on case-insensitive file systems
From: Jeff Hostetler @ 2024-03-06 18:10 UTC (permalink / raw)
To: Patrick Steinhardt, Jeff Hostetler via GitGitGadget
Cc: git, Torsten Bögershausen, Jeff Hostetler
In-Reply-To: <ZehofMaSZyUq8S1N@tanuki>
On 3/6/24 7:58 AM, Patrick Steinhardt wrote:
> On Mon, Feb 26, 2024 at 09:39:11PM +0000, Jeff Hostetler via GitGitGadget wrote:
>> Here is version 3. I think I have addressed the remaining comments.
>>
>> I cleaned up the test code to use the test_expect_failure at the beginning
>> and squashed in the test_expect_success version of tests into the final
>> commit in the series.
>>
>> I moved the invalidate_ce_fsm() commit earlier in the series, so that the
>> final commit actually uses all of the up-to-this-point changes to fix the
>> problem.
>>
>> I converted a few "should not happens" to BUG()s.
>>
>> Thanks to everyone for their time and attention reviewing this. Jeff
>
> I gave this whole patch series a read and didn't much to complain about.
> There are a couple of nits, but none of them really require a reroll in
> my opinion.
>
> Thanks!
>
> Patrick
Thanks!!!
Jeff
^ permalink raw reply
* Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names
From: René Scharfe @ 2024-03-06 18:16 UTC (permalink / raw)
To: phillip.wood, git; +Cc: Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <0947cb09-8b07-4fcd-bbe2-ae37c2cd5ec7@gmail.com>
Hello Phillip,
Am 04.03.24 um 10:51 schrieb Phillip Wood:
> On 03/03/2024 10:13, René Scharfe wrote:
>> TEST_CTYPE_FUNC defines a function for testing a character classifier,
>> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.
>>
>> Avoid the need to define a class-specific function by letting
>> TEST_CHAR_CLASS do all the work. This is done by using the internal
>> functions test__run_begin() and test__run_end(), but they do exist to be
>> used in test macros after all.
>
> Those internal functions exist to implement the TEST() macro, they
> are not really intended for use outside that (which is why they are
> marked as private in the header file). If we ever want to update the
> implementation of TEST() it will be a lot harder if we're using the
> internal implementation directly in test files. Unit tests should be
> wrapping TEST() if it is appropriate but not the internal
> implementation directly.
forcing tests to be expressions and not allow them to use statements is
an unusual requirement. I don't see how the added friction would make
tests any better. It just requires more boilerplate code and annoying
repetition. What kind of changes do you envision that would be
hindered by allowing statements?
> Ideally we wouldn't need TEST_CTYPE_FUNC as there would only be a
> single function that was passed a ctype predicate, an input array and
> an array of expected results. Unfortunately I don't think that is
> possible due the the way the ctype predicates are implemented. Having
> separate macros to define the test function and to run the test is
> annoying but I don't think it is really worth exposing the internal
> implementation just to avoid it.
The classifiers are currently implemented as macros. We could turn them
into inline functions and would then be able to pass them to a test
function. Improving testability is a good idea, but also somehow feels
like the tail wagging the dog. It would be easy, though, I think. And
less gross than:
>> Alternatively we could unroll the loop to provide a very long expression
>> that tests all 256 characters and EOF and hand that to TEST, but that
>> seems awkward and hard to read.
... which would yield unsightly test macros and huge test binaries. But
it would certainly be possible, and keep the definitions of the actual
tests clean.
René
^ permalink raw reply
* Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter
From: Junio C Hamano @ 2024-03-06 17:49 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, git
In-Reply-To: <CAOLa=ZRQ6sYLBqjWiMXGS7rcv8pJ7jSEneiJ8ZcHtALrrcwMeg@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> When creating a new reflog iterator, we first have to reload the stack
>> that the iterator is being created. This is done so that any concurrent
>
> Nit: s/created./created for.
Yeah, I couldn't grok that sentence while reviewing it.
Thanks.
^ permalink raw reply
* Re: [PATCH] setup: allow cwd=.git w/ bareRepository=explicit
From: Junio C Hamano @ 2024-03-06 17:27 UTC (permalink / raw)
To: Kyle Lippincott via GitGitGadget; +Cc: git, Kyle Lippincott
In-Reply-To: <pull.1645.git.1705709303098.gitgitgadget@gmail.com>
"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Teach git to not reject uses of git inside of the .git directory: check
> if cwd is .git (or a subdirectory of it) and allow it even if
> safe.bareRepository=explicit.
> diff --git a/setup.c b/setup.c
> index b38702718fb..b095e284979 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1371,7 +1371,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>
> if (is_git_directory(dir->buf)) {
> trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
> - if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
> + if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
> + !ends_with_path_components(dir->buf, ".git"))
> return GIT_DIR_DISALLOWED_BARE;
> if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
> return GIT_DIR_INVALID_OWNERSHIP;
I wish we had caught it much before we added DISALLOWED_BARE thing,
but I wonder how well would this escape-hatch interact with
secondary worktrees, where their git directory is not named ".git"
and not immediately below the root level of the working tree?
In a secondary worktree the root level of its working tree has a
file ".git", whose contents may look like
gitdir: /home/gitster/git.git/.git/worktrees/git.old
where
- /home/gitster/git.git/ is the primary worktree with the
repository.
- /home/gitster/git.git/.git/worktrees/git.old looks like a bare
repository.
- /home/gitster/git.git/.git/worktrees/git.old/gitdir gives a way
to discover the secondary worktree, whose contents just records
the path to the ".git" file, e.g., "/home/gitster/git.old/.git"
that had "gitdir: ..." in it.
So perhaps we can also use the presence of "gitdir" file, check the
contents of it tn ensure that ".git" file there takes us back to
this (not quite) bare repository we are looking at, and allow access
to it, or something?
Thoughts?
^ permalink raw reply
* Re: [PATCH] Allow git-config to append a comment
From: Ralph Seichter @ 2024-03-06 17:24 UTC (permalink / raw)
To: Junio C Hamano, Ralph Seichter via GitGitGadget; +Cc: git
In-Reply-To: <xmqqttljtkn4.fsf@gitster.g>
* Junio C Hamano:
> Make sure your title will mix well in "git shortlog --no-merges"
> output from recent commits by other contributors.
Thank you for your in-depth comment. This is the first time I have
ever considered contributing to Git, so I have a lot to learn. My pull
request [1] on GitGitGadget has been approved by Johannes Schindelin,
by the way, and the PR is based on an issue Johannes created [2] after
a brief discussion him and I had on Discord [3]. I have updated the
subject line, as you suggested.
[1] https://github.com/gitgitgadget/git/pull/1681
[2] https://github.com/gitgitgadget/git/issues/1680
[3] https://discord.com/channels/1042895022950994071/1213052410281467906
I don't know if it is the polite thing to ask you to please refer to
the information I linked, or if I should duplicate the information
here? The short version is that I need to be able to distinguish
between config entries made by automation (like scripts, or Ansible
in my particular case) and those made by a human.
>> git config --comment "I changed this. --A. Script" \
>> --add safe.directory /home/alice/somerepo.git
>
> If you are illustrating a sample input, please also explain what
> output it produces. What do the resulting lines in the config file
> look like after you run this command?
The result of running the above command looks as follows:
[safe]
directory = /home/alice/somerepo.git #I changed this. --A. Script
> Why are you adding "# comment" to your config file? Who reads these
> comments, with what tool, and for what purpose?
I mentioned human-readable comments in the patch, and humans are
indeed the intended audience. If a human finds changes made to a Git
config file, and a comment states that the modification was e.g. made
by automation, it adds beneficial information for said human. I can for
example create a comment with a URL pointing to a website providing
additional explanations.
> Now how do we find out about this comment? "git config -l" would
> not give us that. Are we expected to look at it in our editor or
> pager?
Yes. I routinely use cat/vim to inspect/modify my Git config
files. They are suitable for human consumption, after all. Also,
comments can already be manually added in creative ways, and are
ignored when Git reads config data. Comments being read only by
humans is pretty much their whole point, in my opinion.
> Can we come up with a code that reliably decides when to remove the
> first comment we see above?
Your examples about difficulties removing comments hinge on there
being multiline comments, as far as I can tell. My patch only supports
single-line comments, and only as a suffix to newly added key-value
pairs. This is a deliberate design choice.
> The above is an illustration of what I want to see the author, who
> comes up with this "wouldn't it be wonderful to teach git-config to
> add comment?" idea, thinks through. The first patch might be to
> just add a comment when a variable=value is added, but we want to
> see the vision of how the whole end-user experience should look
> like, in which this first small piece will fit.
I don't have a greater vision for comments. Their use is to provide
information for humans, no more, no less. There is also no idea of a
user experience beyond pager/editor in my mind. The patch addresses
specific needs of mine, and Johannes suggested it as a new feature,
and that's all the motivation behind it.
> And the amount of the change required for that tiny bit of
> "improvement" (if we can call it, which is dubious) does not seem
> worth it.
As I mentioned in my PR, it also does not seem elegant to me to modify
so many files. Alas, C does not offer expanding function signatures by
adding parameters with default values, like Python does. Adding a new
function like, perhaps, git_config_set_in_file_gently_with_comment()
could be a remedy?
-Ralph
^ permalink raw reply
* Re: [PATCH v3 00/14] FSMonitor edge cases on case-insensitive file systems
From: Junio C Hamano @ 2024-03-06 17:09 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Jeff Hostetler via GitGitGadget, git, Jeff Hostetler,
Torsten Bögershausen, Jeff Hostetler
In-Reply-To: <ZehofMaSZyUq8S1N@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Feb 26, 2024 at 09:39:11PM +0000, Jeff Hostetler via GitGitGadget wrote:
>> Here is version 3. I think I have addressed the remaining comments.
>>
>> I cleaned up the test code to use the test_expect_failure at the beginning
>> and squashed in the test_expect_success version of tests into the final
>> commit in the series.
>>
>> I moved the invalidate_ce_fsm() commit earlier in the series, so that the
>> final commit actually uses all of the up-to-this-point changes to fix the
>> problem.
>>
>> I converted a few "should not happens" to BUG()s.
>>
>> Thanks to everyone for their time and attention reviewing this. Jeff
>
> I gave this whole patch series a read and didn't much to complain about.
> There are a couple of nits, but none of them really require a reroll in
> my opinion.
>
> Thanks!
>
> Patrick
Thanks.
^ permalink raw reply
* Re: [PATCH v6 0/9] Enrich Trailer API
From: Junio C Hamano @ 2024-03-06 17:09 UTC (permalink / raw)
To: Christian Couder
Cc: Josh Steadmon, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Randall S. Becker,
Kristoffer Haugsbakk, Linus Arver
In-Reply-To: <CAP8UFD1Zd+9q0z1JmfOf60S2vn5-sD3SafDvAJUzRFwHJKcb8A@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Mar 5, 2024 at 8:07 PM Josh Steadmon <steadmon@google.com> wrote:
>>
>> On 2024.03.05 10:03, Junio C Hamano wrote:
>> >
>> > It's been nearly a week since this was posted. Any more comments,
>> > or is everybody happy with this iteration? Otherwise I am tempted
>> > to mark the topic for 'next' soon.
>> >
>> > Thanks.
>>
>> I scanned through v6 yesterday and have nothing new to add. LGTM.
>
> I took another look at it, and I am fine with it now too. Acked.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/8] builtin/config: introduce subcommands
From: Junio C Hamano @ 2024-03-06 17:06 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <cover.1709724089.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> - `git config foo.bar` -> `git config get foo.bar`
>
> - `git config foo.bar value` -> `git config set foo.bar value`
I actually have been perfectly OK with the above two, but I agree
that ...
> - `git config foo.bar value value-pattern` -> `git config set-all
> foo.bar value value-pattern`
... this was less than discoverable, and would be a good update.
This one ...
> - `git config --get-urlmatch` -> `git config get-urlmatch`.
... is a Meh to me, personally. I'd not actively push it
enthusiastically, but I'd passively accept its existence.
> Most importantly, this should help discoverability quite a lot by now
> also having a proper synopsis in both the manpage, but also in `git
> config -h`.
>
> Of course, backwards compatibility is a big concern. We don't want to
> just switch over to the new syntax and break all existing scripts and
> muscle memory. This patch series thus abuses the fact that the implicit
> modes (`git config foo.bar`, `git config foo.bar value` and `git config
> foo.bar value value-pattern`) all require a key as first argument. As
> keys _must_ have a dot, this allows us to unambiguously discern those
> from actual subcommands.
Clever ;-).
^ permalink raw reply
* Re: [PATCH v6 0/9] Enrich Trailer API
From: Junio C Hamano @ 2024-03-06 16:59 UTC (permalink / raw)
To: Christian Couder
Cc: Josh Steadmon, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Randall S. Becker,
Kristoffer Haugsbakk, Linus Arver
In-Reply-To: <CAP8UFD1Zd+9q0z1JmfOf60S2vn5-sD3SafDvAJUzRFwHJKcb8A@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Mar 5, 2024 at 8:07 PM Josh Steadmon <steadmon@google.com> wrote:
>>
>> On 2024.03.05 10:03, Junio C Hamano wrote:
>> >
>> > It's been nearly a week since this was posted. Any more comments,
>> > or is everybody happy with this iteration? Otherwise I am tempted
>> > to mark the topic for 'next' soon.
>> >
>> > Thanks.
>>
>> I scanned through v6 yesterday and have nothing new to add. LGTM.
>
> I took another look at it, and I am fine with it now too. Acked.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe
From: Junio C Hamano @ 2024-03-06 16:57 UTC (permalink / raw)
To: Jean-Noël Avila; +Cc: Atneya Nair, git, jeffhost, me, nasamuffin
In-Reply-To: <10042df8-5d06-47cd-9202-ea6965f50784@gmail.com>
Jean-Noël Avila <avila.jn@gmail.com> writes:
>> -const char *read_gitfile_gently(const char *path, int *return_error_code)
>> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
>> {
>> const int max_file_size = 1 << 20; /* 1MB */
>> int error_code = 0;
>> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>> struct stat st;
>> int fd;
>> ssize_t len;
>> - static struct strbuf realpath = STRBUF_INIT;
>> + static struct strbuf shared = STRBUF_INIT;
>> + if (!result_buf) {
>> + result_buf = &shared;
>> + }
>>
>
> Question about general style: is it accepted practice to override a
> parameter of a function?
We do not forbid it. We have a rule against enclosing a single
statement block inside {braces}, though ;-).
> I would have written:
If it matters to know what the caller-supplied value of the
parameter was, then we would probably write that way. If it does
not, then the above is perfectly fine. Even with the above, if a
later code really wanted to, it can compare the pointers to find out
if the caller was uninterested in the result (i.e., passed NULL),
but at that point, we may be better off to (re)write it your way.
>> - strbuf_realpath(&realpath, dir, 1);
>> - path = realpath.buf;
>> + strbuf_realpath(result_buf, dir, 1);
>> + path = result_buf->buf;
>> + // TODO is this valid?
>> + if (!path) die(_("Unexpected null from realpath '%s'"), dir);
>
> In fact, this is not a null path, but an empty path (null is not part of
> the string).
> By the way, shouldn't this be an internal bug instead of a message to
> the user?
Unless the strbuf instance the result_buf pointer points at is
corrupt, its .buf member should *NEVER* be NULL. Testing for NULL
is meaningless, unless you are manually futzing with the members of
strbuf (you shouldn't).
Thanks for carefully reading.
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Junio C Hamano @ 2024-03-06 16:48 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, John Cai via GitGitGadget, git, John Cai
In-Reply-To: <20240306075128.GA4099518@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> You can do:
>
> git for-each-ref --format='%(refname) %(objectname) %(symref)'
We can even do that "ref target || object name" thing with
--format='%(refname) %(if)%(symref)%(then)%(symref)%(else)%(objectname)'
if we wanted to. But if we have both available, I think the output
that adds the symref target, if available, after the object name, is
better than the output that switches between the two.
> to get the resolved values next to the symrefs (if any). I think that
> does a full resolution, though (so again, if you had one->two->three,
> you can never learn about the intermediate "two").
Yeah, I know we discussed the usefulness of tag-of-tag-of-something,
but this is a similar one.
> I think for-each-ref in the above command works by calling
> resolve_refdup() itself, and then recording the result. It would be nice
> to get it from the iterator, though (more efficient, and avoids any
> races).
Indeed. Thanks for an interesting thought.
^ permalink raw reply
* Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction
From: Junio C Hamano @ 2024-03-06 16:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <ZehauDgP4L40QKcH@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> Naive question, why do we include the headers in `system.h`? I assume
>> this is because they are common? Are there other benefits to this
>> indirection?
>
> Well, "system.h" is supposedly the glue between the common Git codebase
> and the reftable library, so all Git-specific headers should be added
> here instead of being added individually to the respective files in the
> library. Whether that is ultimately a sensible thing and whether it
> really helps us all that much is a different question though.
That matches my understanding of what have been done in reftable/
directory. If a project other than Git wants to use the reftable
code, they only need to prepare a shim and write their own
"system.h" to provide services equivalent to what Git supplies.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles
From: Junio C Hamano @ 2024-03-06 16:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <Zehav4V_8GGZG94Q@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> Since the tempfile is now being created through the tempfile API, I
>> think the file mode can be set directly through `mks_tempfile_m()`
>> instead of creating the tempfile and then using chmod. Just something I
>> thought to mention.
>
> Unfortunately not. The problem is that `mks_tempfile_m()` will munge
> passed-in permissions via "core.sharedRepository", but we already pre
> calculated the target mode in `config.default_permissions`. Thus, the
> result would have wrong permissions if we used `mks_tempfile_m()`.
I somehow found that default_permissions thing always disturbing.
Even if we keep a separate mechanism for determining the file
permission (perhaps in order to give ourselves a better separation
as "an independent library" from the rest of Git), shouldn't the
permission setting that is computed by the mechanism and stored in
config.default_permissions be consistent with the permission the
rest of git computes based on core.sharedRepository?
^ permalink raw reply
* Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles
From: Justin Tobler @ 2024-03-06 16:34 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <Zehav4V_8GGZG94Q@tanuki>
On 24/03/06 12:59PM, Patrick Steinhardt wrote:
> On Tue, Mar 05, 2024 at 04:30:18PM -0600, Justin Tobler wrote:
> > On 24/03/04 12:10PM, Patrick Steinhardt wrote:
> > > We do not register new tables which we're about to add to the stack with
> > > the tempfile API. Those tables will thus not be deleted in case Git gets
> > > killed.
> > >
> > > Refactor the code to register tables as tempfiles.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > > reftable/stack.c | 29 ++++++++++++-----------------
> > > 1 file changed, 12 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/reftable/stack.c b/reftable/stack.c
> > > index b64e55648a..81544fbfa0 100644
> > > --- a/reftable/stack.c
> > > +++ b/reftable/stack.c
> > > @@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,
> > > struct strbuf tab_file_name = STRBUF_INIT;
> > > struct strbuf next_name = STRBUF_INIT;
> > > struct reftable_writer *wr = NULL;
> > > + struct tempfile *tab_file = NULL;
> > > int err = 0;
> > > - int tab_fd = 0;
> > > + int tab_fd;
> > >
> > > strbuf_reset(&next_name);
> > > format_name(&next_name, add->next_update_index, add->next_update_index);
> > > @@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,
> > > stack_filename(&temp_tab_file_name, add->stack, next_name.buf);
> > > strbuf_addstr(&temp_tab_file_name, ".temp.XXXXXX");
> > >
> > > - tab_fd = mkstemp(temp_tab_file_name.buf);
> > > - if (tab_fd < 0) {
> > > + tab_file = mks_tempfile(temp_tab_file_name.buf);
> > > + if (!tab_file) {
> > > err = REFTABLE_IO_ERROR;
> > > goto done;
> > > }
> > > if (add->stack->config.default_permissions) {
> > > - if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
> > > + if (chmod(get_tempfile_path(tab_file),
> > > + add->stack->config.default_permissions)) {
> > > err = REFTABLE_IO_ERROR;
> > > goto done;
> > > }
> > > }
> >
> > Since the tempfile is now being created through the tempfile API, I
> > think the file mode can be set directly through `mks_tempfile_m()`
> > instead of creating the tempfile and then using chmod. Just something I
> > thought to mention.
>
> Unfortunately not. The problem is that `mks_tempfile_m()` will munge
> passed-in permissions via "core.sharedRepository", but we already pre
> calculated the target mode in `config.default_permissions`. Thus, the
> result would have wrong permissions if we used `mks_tempfile_m()`.
Ah that makes sense. Thanks for the explanation!
-Justin
^ permalink raw reply
* Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter
From: Karthik Nayak @ 2024-03-06 16:13 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <b0414221ecad1920c84f4ab498e55edec57f06b6.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 217 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> When creating a new reflog iterator, we first have to reload the stack
> that the iterator is being created. This is done so that any concurrent
Nit: s/created./created for.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v4] build: support z/OS (OS/390).
From: Junio C Hamano @ 2024-03-06 16:10 UTC (permalink / raw)
To: Haritha via GitGitGadget; +Cc: git, Haritha D, Ghanshyam Thakkar, rsbecker
In-Reply-To: <pull.1663.v4.git.git.1709703857881.gitgitgadget@gmail.com>
"Haritha via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Haritha D <harithamma.d@ibm.com>
>
> Introduced z/OS (OS/390) as a platform in config.mak.uname
>
> Signed-off-by: Haritha D <harithamma.d@ibm.com>
> ---
> This PR enables a successful git build on z/OS.
Good.
> config.mak.uname | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index dacc95172dc..d0dcca2ec55 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -638,6 +638,18 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
> SHELL_PATH = /usr/coreutils/bin/bash
> endif
> +ifeq ($(uname_S),OS/390)
> + NO_SYS_POLL_H = YesPlease
> + NO_STRCASESTR = YesPlease
> + NO_REGEX = YesPlease
> + NO_MMAP = YesPlease
> + NO_NSEC = YesPlease
> + NO_STRLCPY = YesPlease
> + NO_MEMMEM = YesPlease
> + NO_GECOS_IN_PWENT = YesPlease
> + HAVE_STRINGS_H = YesPlease
> + NEEDS_MODE_TRANSLATION = YesPlease
> +endif
I somehow expected you to throw in the -L thing in this block,
perhaps like
CC_LD_DYNPATH =
to help those who are on OS/390 but do not run configure (made from
configure.ac) to create the config.mak.autogen file, but if you are
always building with configure and not testing such a configuration,
then doing so and shipping an untested code would not be prudent, so
let's accept this patch as-is.
Thanks for working on this. Will queue.
^ permalink raw reply
* Re: [PATCH 19/22] t/t8*: merge "grep | sed" pipelines
From: Junio C Hamano @ 2024-03-06 16:03 UTC (permalink / raw)
To: Todd Zullinger; +Cc: Beat Bolli, git, Beat Bolli
In-Reply-To: <ZefSPAiTLrRVeXm6@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> Junio C Hamano wrote:
>> Isn't -E a GNUism?
> ...
> https://www.austingroupbugs.net/view.php?id=528 suggests
> that -E has been adopted
Then that is OK. Thanks for a good news.
> and, importanly, is more widely
> supported than -r (if we were considering using that rather
> than rewriting this to not use ERE syntax).
At least I wasn't, so it is irrelevant to this review, but it still
is nice to know about it [*]. Thanks.
[Footnote]
* or is the knowledge of '-r' itself also irrelevant, now '-E' is
kosher and widely usable?
^ permalink raw reply
* diff on the commandline
From: Andreas Lüttgens @ 2024-03-06 16:02 UTC (permalink / raw)
To: git
Hi,
like the new feature of diffing against the command line very much.
Would very much appreciate if pressing the ESCAPE key would stop the
git command and return to the command line.
Thank you for your great work,
kind regards,
Andreas
~/src/prj/lit/manual/wdfelements master* ❯ git diff ../
../ HEAD master
03e57af -- [HEAD] wdfelements.pdf (2 minutes ago)
cf8d179 -- [HEAD^] CALIB_MODE (5 hours ago)
1c402ee -- [HEAD^^] .. (6 hours ago)
4b7d0b3 -- [HEAD~3] .. (6 hours ago)
05d9148 -- [HEAD~4] .. (7 hours ago)
69d3458 -- [HEAD~5] .. (13 hours ago)
cff7654 -- [HEAD~6] .. (13 hours ago)
51ed121 -- [HEAD~7] .. (14 hours ago)
bfc349c -- [HEAD~8] .. (19 hours ago)
8172043 -- [HEAD~9] .. (19 hours ago)
00dc48a -- [HEAD~10] .. (20 hours ago)
c9453fe -- [HEAD~11] .. (20 hours ago)
add366a -- [HEAD~12] .. (20 hours ago)
cda039d -- [HEAD~13] .. (20 hours ago)
^ permalink raw reply
* Re: [PATCH] Allow git-config to append a comment
From: Junio C Hamano @ 2024-03-06 16:01 UTC (permalink / raw)
To: Ralph Seichter via GitGitGadget; +Cc: git, Ralph Seichter
In-Reply-To: <pull.1681.git.1709532018372.gitgitgadget@gmail.com>
"Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Ralph Seichter <github@seichter.de>
> Subject: Re: [PATCH] Allow git-config to append a comment
Thanks for a patch.
Make sure your title will mix well in "git shortlog --no-merges"
output from recent commits by other contributors. See
Documentation/SubmittingPatches[[describe-changes]]. Perhaps
Subject: [PATCH] config: add --comment option to add a comment
> Introduce the ability to append comments to modifications
> made using git-config. Example usage:
>
> git config --comment "I changed this. --A. Script" \
> --add safe.directory /home/alice/somerepo.git
If you are illustrating a sample input, please also explain what
output it produces. What do the resulting lines in the config file
look like after you run this command?
I find the overall idea somewhat incomplete. Perhaps stepping back
a bit and thinking through the _whole_ use case of "comment" in the
configuration files is in order, before thinking about an easier way
to add one. Why are you adding "# comment" to your config file?
Who reads these comments, with what tool, and for what purpose?
What will they do after they learn something from what their former
selves wrote as comments?
Let's imagine we had this in our global configuration file in the
beginning:
$ cat ~/.gitconfig
...
[safe]
directory = /home/bob/otherrepo.git
...
and then we run this:
$ git config --global --comment 'the reason why I added ~alice/
is because ...' --add safe.directory /home/alice/somerepo.git
$ git config --global --add safe.directory /home/charlie/somerepo.git
which may modify the configuration file with the new entry with a
comment. The part about safe.directory might now read like so:
[safe]
directory = /home/bob/otherrepo.git
# the reason why I added ~alice/
# is because ...
directory = /home/alice/somerepo.git
directory = /home/charlie/somerepo.git
Now how do we find out about this comment? "git config -l" would
not give us that. Are we expected to look at it in our editor or
pager?
$ vi ~/.gitconfig
$ less ~/.gitconfig
Why are we interested in looking at these comments in the first
place [*]?
Side note: I do not ask "why are we interested in leaving
these comments in the file"---it goes without saying that
it is because we want to be able to later read them.
Presumably, we are trying to remind ourselves why we added a
particular variable=value. By learning that, what are we going to
do next? Perhaps once the reason to mark /home/alice/somerepo.git
as a safe.directory disappears, we would want to remove this entry?
But then, after running
$ less ~/.gitconfig
and then learning that we no longer need /home/alice/somerepo.git
marked as a safe.directory, how would we remove it from the
configuration file, and what happens to the comment?
$ git config --global --unset safe.directory /home/alice/somerepo.git
may remove the value, but it would not remove the comment, and
you'll be left with something like this:
[safe]
directory = /home/bob/otherrepo.git
# the reason why I added ~alice/
# is because ...
directory = /home/charlie/somerepo.git
The remaining comment looks as if it talks about "charlie", but it
simply is stale.
Should it have removed the comment, then? I actually do not think
so. A comment before a particular variable definition may not be
the result of the use of this new "config --comment" option, but
perhaps the user added it manually. Or even worse, the comment may
not even be about an individual entry. Imagine
[section]
# definitions for per-user stuff begin here
var = /home/alice
# these two directories are actually charlie's
var = /home/bob
var = /home/charlie
# definitions for per-project stuff
var = /project/a
var = /project/b
Can we come up with a code that reliably decides when to remove the
first comment we see above? The answer given by human might be
"when all the entries about /home/alice, /home/bob, and
/home/charlie get removed", but can we implement that in code
without interpreting the natural language? I doubt it. We may even
be better off to keep the comment to remind ourselves where to add
the per-user stuff in the section when we hire a new user the next
time.
With the new "--comment" thing might be able to add comments without
using an editor, but the user would need to view the configuration
file with a pager or an editor bypassing Git to make use of what was
recorded in there. And it is likely that these comments will need
to be edited or removed using an editor because "git config" command
would not be usable for maintaining them. For that matter, if you
made a typo in your "--comment" option from the command line, you
would have to resort to your editor to fix that typo.
The above is an illustration of what I want to see the author, who
comes up with this "wouldn't it be wonderful to teach git-config to
add comment?" idea, thinks through. The first patch might be to
just add a comment when a variable=value is added, but we want to
see the vision of how the whole end-user experience should look
like, in which this first small piece will fit.
Identify the whole life cycle of the result of using the feature,
what the end-user experience of using the result of using the
feature would look like, etc. In this case, "the result of using
the feature" is the comments that are initially placed next to a
newly defined variable-value pair, and at least to me, the end-user
experience to manage the life cycle of these comments is not all
that improved from the status quo, which is that you need your pager
and editor to view them, edit them, typofix them, and remove them.
I am somewhat negative on the idea to make the initial addition of
the comment vastly different from all other uses of the comment by
introducing this "--comment" option. If we have a vision, if not
design and/or code, to help managing other parts of the lifecycle,
not just adding in the beginning, it might be a different story, but
I cannot quite see it.
> Documentation/git-config.txt | 10 +++++++---
> builtin/config.c | 21 ++++++++++++++-------
> builtin/gc.c | 4 ++--
> builtin/submodule--helper.c | 2 +-
> builtin/worktree.c | 4 ++--
> config.c | 21 +++++++++++++--------
> config.h | 4 ++--
> sequencer.c | 28 ++++++++++++++--------------
> submodule-config.c | 2 +-
> submodule.c | 2 +-
> t/t1300-config.sh | 9 +++++++--
> worktree.c | 4 ++--
> 12 files changed, 66 insertions(+), 45 deletions(-)
And the amount of the change required for that tiny bit of
"improvement" (if we can call it, which is dubious) does not seem
worth it.
Thanks.
^ permalink raw reply
* Re: Discuss GSoC: Implement consistency checks for refs
From: Patrick Steinhardt @ 2024-03-06 14:45 UTC (permalink / raw)
To: shejialuo; +Cc: git
In-Reply-To: <ZehtpMtxPLuYYmgO@ArchLinux>
[-- Attachment #1: Type: text/plain, Size: 4189 bytes --]
On Wed, Mar 06, 2024 at 09:20:36PM +0800, shejialuo wrote:
> Hi All,
>
> I am interested in "Implement consistency checks for refs" GSoC idea.
> However, implementing a feautre is much harder. So I wanna ask you some
> questions to better work on.
Sure!
> As [1] shows, I think the idea is easy to understand. We need to ensure
> the consistency of the refs. The current `git-fsck` only checks the
> connectivity from ref to the object file. There is a possiblity that ref
> itself could be corrupted. And we should avoid it through this project.
I know this is splitting hairs, but git-fsck(1) doesn't give us the
tools to avoid corruption. It only gives us the tools to detect it after
the fact.
> I have read some source codes. Based on what I have learned, I know
> there are two backends. One is file and another is reftable. I have
> no idea about the reftable currently. So at now, I will focus on the
> file backend.
Yeah, the "reftable" backend is new in the Git v2.45 release cycle, so
it's totally expected that most peeople have no idea about it. It's also
part of the motivation for this project though. Because as you noted, it
is a binary format that is thus not as readily parseable by a human as
the old "files, backend. This makes it much more important to provide
the tooling to detect whether things look as expected.
> I think the principle behind the `git-fsck` is that it will traverse
> every object file, read its content and use SHA-1 to hash the content
> and compare the value with the stored ref value. So if we want to add
> consistency checks for refs. We may need to add a new file to store the
> last commit state (not only last commit state, do we need to consider
> the stash state). However, from my perspective, it's a bad idea to use a
> file to store the refs' states and we cannot use object file to check
> whether the ref is corrupted.
I agree a 100% -- tracking ref states in a secondary database is not a
good idea.
> So this is my first question, what mechanism should we use to provide
> consistency? And to what extend for the consistency. And I think this
> mechanism should be general for both text-based and binary-based refs.
The exact extent will need some discussion. What's clear is that it does
not need to be perfect from the beginning, and we are sure to discover
more checks over time that may make sense.
Some ideas from the top of my head:
- generic
- Ensure that all ref names are conformant.
- Ensure that there are no directory/file conflicts for the ref
names.
- files
- Ensure that "packed-refs" is well-formatted.
- Ensure that refs in "packed-refs" are ordered lexicographically.
- Check for corrupted loose refs in "refs/".
- reftable
- Ensure that there are no garbage files in "reftable/".
- Ensure that "tables.list" is well-formatted.
- Ensure that each table is well-formatted.
- Ensure that refs in each table are ordered correctly.
This list is not exhaustive, there may of course be other checks that
may make sense. Any additional ideas by you or other interested students
are be welcome.
For what it's worth, not all of the checks need to be implemented as
part of GSoC. At a minimum, it should result in the infra to allow for
backend-specific checks and a couple of checks for at least one of the
backends.
> And I have a more general qeustion, I think I need understand `fsck.c`
> and of couse the reftable format. However, I am confused whether I need
> to understand the ref internal. And could you please provide me more
> infomration to make this idea more clear.
You will certainly need to learn about ref internals a bit. There are
some common rules and restrictions that are important in order to figure
out what we want to check in the first place. Understanding the
"reftable" format would be great, but you may also get away with only
implementing generic or "files"-backend specific consistency checks.
This depends on the scope you are aiming for.
Patrick
> Thanks,
> Jialuo
>
> [1] https://lore.kernel.org/git/ZakIPEytlxHGCB9Y@tanuki/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/9] Enrich Trailer API
From: Christian Couder @ 2024-03-06 14:41 UTC (permalink / raw)
To: Josh Steadmon, Junio C Hamano, Linus Arver via GitGitGadget, git,
Christian Couder, Emily Shaffer, Randall S. Becker,
Christian Couder, Kristoffer Haugsbakk, Linus Arver
In-Reply-To: <Zedtd6esmIgayeoU@google.com>
On Tue, Mar 5, 2024 at 8:07 PM Josh Steadmon <steadmon@google.com> wrote:
>
> On 2024.03.05 10:03, Junio C Hamano wrote:
> >
> > It's been nearly a week since this was posted. Any more comments,
> > or is everybody happy with this iteration? Otherwise I am tempted
> > to mark the topic for 'next' soon.
> >
> > Thanks.
>
> I scanned through v6 yesterday and have nothing new to add. LGTM.
I took another look at it, and I am fine with it now too. Acked.
^ 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