* Re: [PATCH v3 1/1] Replace SID with domain/username
From: Junio C Hamano @ 2024-01-02 17:43 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine
In-Reply-To: <20231231091245.2853-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Replace SID with domain/username in error message, if owner of repository
> and user are not equal on windows systems.
>
> Old message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New massage:
"massage"???
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'DESKTOP-L78JVA6/Administrator'
> but the current user is:
> 'DESKTOP-L78JVA6/test'
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
The same "does this lose information?" comment applies to this one
as well, as the fundamental approach is unchanged.
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /* returns only FALSE, because the string pointers are NULL*/
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
> + /*Alloc needed space of the strings*/
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> + *str, &len_domain, &pe_use);
> + *(*str + len_domain) = '/';
> + if (translate_sid_to_user == FALSE) {
> + FREE_AND_NULL(*str);
> + }
Is this "FREE_AND_NULL" about clearing after you see an error? If
so, shouldn't "overwrite the byte after the domain part with a slash"
be done only when you have no error, i.e., perhaps
translate_sid_to_user = LookupAccountSidA(...);
if (!translate_sid_to_user)
FREE_AND_NULL(*str);
else
(*str)[len_domain] = '/';
or something along the line?
> + return translate_sid_to_user;
> +}
^ permalink raw reply
* Re: subtree split after deleting and re-running git-subtree add, fails with "fatal: cache for XXX already exists!"
From: Christian Couder @ 2024-01-02 18:23 UTC (permalink / raw)
To: Eli Schwartz; +Cc: git
In-Reply-To: <6de00946-9c5a-4854-9e49-069a22f8a782@gmail.com>
On Tue, Dec 26, 2023 at 1:58 AM Eli Schwartz <eschwartz93@gmail.com> wrote:
>
> Originally reported in https://github.com/eli-schwartz/aurpublish/issues/30
>
>
> Given a subtree that gets messed up, some users might naturally
> gravitate towards deleting the subtree, and recreating it again via
> `git subtree add`. This can result in a difficult to solve situation.
> Any attempt to split it seems to produce failure.
>
> Reproducer:
>
> git init testme && cd testme
> mkdir foo
> touch foo/bar
> git add foo/bar
> git commit -m ...
> split_commit=$(git subtree split -P foo --rejoin)
> # Added dir 'foo'
> echo "${split_commit}"
> # 42517e4b9fe310a64be2a777ef08c91bd582b385
>
> git rm -r foo
> git commit -m deleted
> git subtree add --prefix foo "${split_commit}"
> # Added dir 'foo'
> git subtree split -P foo --rejoin
> # fatal: cache for 42517e4b9fe310a64be2a777ef08c91bd582b385 already exists!
>
>
>
> The interesting thing here is that in git.git commit
> d2f0f819547de35ffc923fc963f806f1656eb2ca:
> "subtree: more consistent error propagation"
> the git-subtree program got a bit of a facelift w.r.t. proper error
> checking.
>
> In particular, in find_existing_splits, `cache_set $sub $sub` will fail
> here. But before that commit, the die did not propagate. It turns out
> that actually ignoring this was "fine" and resulted in successfully
> splitting (while also printing a "warning": back then, the word "fatal"
> did not appear anywhere in the message; now it does).
Thanks for reporting this issue! Unfortunately it looks like git
subtree is not very well maintained these days.
There is another thread where Zach FettersMoore proposed other fixes
in what look like a similar area:
https://lore.kernel.org/git/pull.1587.v6.git.1701442494319.gitgitgadget@gmail.com/
Maybe you could team up with Zach to review each other's fixes?
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Taylor Blau @ 2024-01-02 18:47 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, git, ps, christian.couder
In-Reply-To: <CAOLa=ZTPxWXnZ8kpBB7=cybNfdEv6d6O37Em7Vpmcw=enpY1_w@mail.gmail.com>
On Tue, Jan 02, 2024 at 07:18:48AM -0800, Karthik Nayak wrote:
> > As "git for-each-ref" takes pattern that is prefix match, e.g.,
> >
> > $ git for-each-ref refs/remotes/
> >
> > shows everything like refs/remotes/origin/main that begins with
> > refs/remotes/, I wonder if
> >
> > $ git for-each-ref ""
> >
> > should mean what you are asking for. After all, "git for-each-ref"
> > does *not* take "--branches" and others like "git log" family to
> > limit its operation to subhierarchy of "refs/" to begin with.
>
> But I don't think using an empty pattern is the best way to go forward.
> This would break the pattern matching feature. For instance, what if the
> user wanted to print all refs, but pattern match "*_HEAD"?
>
> Would that be
>
> $ git for-each-ref "" "*_HEAD"
>
> I think this would be confusing, since the first pattern is now acting
> as an option, since its not really filtering rather its changing the
> search space.
>
> Maybe "--all-refs" or "--all-ref-types" instead?
I tend to agree that the special empty pattern would be a good shorthand
for listing all references underneath refs/, including any top-level
psuedo-refs.
But I don't think that I quite follow what Karthik is saying here.
for-each-ref returns the union of references that match the given
pattern(s), not their intersection. So if you wanted to list just the
psudo-refs ending in '_HEAD', you'd do:
$ git for-each-ref "*_HEAD"
I think if you wanted to list all pseudo-refs, calling the option
`--pseudo-refs` seems reasonable. But if you want to list some subset of
psueod-refs matching a given pattern, you should specify that pattern
directly.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Taylor Blau @ 2024-01-02 18:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, Karthik Nayak, git, christian.couder
In-Reply-To: <ZY1PLtPue4PgbhwU@tanuki>
On Thu, Dec 28, 2023 at 11:34:22AM +0100, Patrick Steinhardt wrote:
> One interesting question is how we should treat files that look like a
> pseudoref, but which really aren't. I'm not aware of any such files
> written by Git itself, but it could certainly be that a user wrote such
> a file into the repository manually. But given that we're adding new
> behaviour that will be opt-in (e.g. via a new switch) I'd rather err on
> the side of caution and mark any such file as broken instead of silently
> ignoring them.
I probably wouldn't spend a ton of time worrying about this personally.
Without additional information, I think it's impossible for us to
determine a-priori whether or not a file underneath $GIT_DIR should be
interpreted as a pseudo-ref or not.
I agree with your reasoning that since this is opt-in via a new
command-line flag, that we're probably OK here enumerating the files in
$GIT_DIR, and printing out the ones that look like pseudo-refs.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Taylor Blau @ 2024-01-02 18:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Achu Luma, christian.couder, git, Christian Couder
In-Reply-To: <xmqqsf3ohkew.fsf@gitster.g>
On Tue, Dec 26, 2023 at 10:45:59AM -0800, Junio C Hamano wrote:
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> > deleted file mode 100644
> > index e5659df40b..0000000000
> > --- a/t/helper/test-ctype.c
> > +++ /dev/null
> > @@ -1,70 +0,0 @@
> > -#include "test-tool.h"
> > -
> > -static int rc;
> > -
> > -static void report_error(const char *class, int ch)
> > -{
> > - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> > - rc = 1;
> > -}
>
> So, if we have a is_foo() that characterises a byte that ought to
> be "foo" but gets miscategorised not to be "foo", we used to
> pinpoint exactly the byte value that was an issue. We did not do
> any early return ...
>
> > ...
> > -#define TEST_CLASS(t,s) { \
> > - int i; \
> > - for (i = 0; i < 256; i++) { \
> > - if (is_in(s, i) != t(i)) \
> > - report_error(#t, i); \
> > - } \
> > - if (t(EOF)) \
> > - report_error(#t, EOF); \
> > -}
>
> ... and reported for all errors in the "class".
>
> > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> > new file mode 100644
> > index 0000000000..41189ba9f9
> > --- /dev/null
> > +++ b/t/unit-tests/t-ctype.c
> > @@ -0,0 +1,76 @@
> > +#include "test-lib.h"
> > +
> > +static int is_in(const char *s, int ch)
> > +{
> > + /*
> > + * We can't find NUL using strchr. Accept it as the first
> > + * character in the spec -- there are no empty classes.
> > + */
> > + if (ch == '\0')
> > + return ch == *s;
> > + if (*s == '\0')
> > + s++;
> > + return !!strchr(s, ch);
> > +}
> > +
> > +/* Macro to test a character type */
> > +#define TEST_CTYPE_FUNC(func, string) \
> > +static void test_ctype_##func(void) \
> > +{ \
> > + int i; \
> > + for (i = 0; i < 256; i++) \
> > + check_int(func(i), ==, is_in(string, i)); \
> > +}
>
> Now, we let check_int() to do the checking for each and every byte
> value for the class. check_int() uses different reporting and shows
> the problematic value in a way that is more verbose and at the same
> time is a less specific and harder to understand:
>
> test_msg(" left: %"PRIdMAX, a);
> test_msg(" right: %"PRIdMAX, b);
>
> But that is probably the price to pay to use a more generic
> framework, I guess.
Perhaps I'm missing something here, since I haven't followed the
unit-test effort very closely, but this check_int() macro feels like it
might be overkill for what we're trying to do.
We know that the expected value is the result of is_in(string, i), so I
wonder if we might benefit from having an "assert_equals()" that looks
like:
assert_equals(is_in(string, i), func(i));
Where we follow the usual convention of treating the first argument as
the expected value, and the second as the actual value. Then we could
format our error message to be more specific, like:
test_msg("expected %d, got %d", expected, actual);
I think that this would be a little more readable, and still seems
flexible enough to support the kind of thing that check_int(..., ==,
...) is after.
> > +int cmd_main(int argc, const char **argv) {
> > + /* Run all character type tests */
> > + TEST(test_ctype_isspace(), "isspace() works as we expect");
> > + TEST(test_ctype_isdigit(), "isdigit() works as we expect");
> > + TEST(test_ctype_isalpha(), "isalpha() works as we expect");
> > + TEST(test_ctype_isalnum(), "isalnum() works as we expect");
> > + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect");
> > + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect");
> > + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect");
> > + TEST(test_ctype_isascii(), "isascii() works as we expect");
> > + TEST(test_ctype_islower(), "islower() works as we expect");
> > + TEST(test_ctype_isupper(), "isupper() works as we expect");
> > + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect");
> > + TEST(test_ctype_ispunct(), "ispunct() works as we expect");
> > + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect");
> > + TEST(test_ctype_isprint(), "isprint() works as we expect");
> > +
> > + return test_done();
> > +}
>
> As a practice to use the unit-tests framework, the patch looks OK.
> helper/test-ctype.c indeed is an oddball that runs once and checks
> everything it wants to check, for which the unit tests framework is
> much more suited.
As an aside, I don't think we need the "works as we expect" suffix in
each test description. I personally would be fine with something like:
TEST(test_ctype_isspace(), "isspace()");
TEST(test_ctype_isdigit(), "isdigit()");
...
But don't feel strongly about it.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] git-clone.txt: document -4 and -6
From: Jakub Wilk @ 2024-01-02 18:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Wong
In-Reply-To: <xmqq1qivd8d0.fsf@gitster.g>
* Junio C Hamano <gitster@pobox.com>, 2023-06-01 15:06:
>The patch is not _wrong_ per-se, but there are other options that are
>common among the "fetch" family of commands. I counted at least these
>should be shared between "fetch" and "clone", by splitting them out of
>"fetch-options.txt" into a new file, and including that new file from
>"fetch-options.txt" and "git-clone.txt".
Agreed such a split would be better, but I don't have energy to
implement it.
--
Jakub Wilk
^ permalink raw reply
* Re: [RFC PATCH] grep: default to posix digits with -P
From: Carlo Arenas @ 2024-01-02 19:02 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <55309061-5996-4f70-8e6b-b341fc632ddf@web.de>
On Mon, Jan 1, 2024 at 9:18 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 01.01.24 um 16:03 schrieb Carlo Marcelo Arenas Belón:
> > @@ -321,17 +327,22 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
> > }
> > options |= PCRE2_CASELESS;
> > }
> > - if (!opt->ignore_locale && is_utf8_locale() && !literal)
> > - options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);
> > + if (!opt->ignore_locale && is_utf8_locale() && !literal) {
> > + options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
> >
> > -#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER
> > - /*
> > - * Work around a JIT bug related to invalid Unicode character handling
> > - * fixed in 10.35:
> > - * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d
> > - */
> > - options &= ~PCRE2_UCP;
> > +#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER
> > + /*
> > + * Work around a JIT bug related to invalid Unicode character handling
> > + * fixed in 10.35:
> > + * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d
> > + */
> > + options |= PCRE2_UCP;
> > +#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER
> > + if (!opt->perl_digit)
> > + xoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);
> > #endif
> > +#endif
>
> Why do we need that extra level of indentation?
I was just trying to simplify the code by including all the logic in
one single set.
The original lack of indentation that was introduced by later fixes to
the code was IMHO also misguided since the obvious "objective" as set
in the original code that added PCRE2_UCP was that it should be used
whenever UTF was also in use as shown by
acabd2048ee0ee53728100408970ab45a6dab65e.
Of course, we soon found out that the original implementation of
PCRE2_MATCH_INVALID_UTF that came with PCRE2 10.34 was buggy and so an
exception was introduced in 14b9a044798ebb3858a1f1a1377309a3d6054ac8.
Note that the problematic code is only relevant when JIT is also
enabled, but JIT is almost always enabled.
> The old code turned PCRE2_UCP on by default and turned it off for older
> versions. The new code enables PCRE2_UCP only for newer versions. The
> result should be the same, no? So why change that part at all?
Because it gets us a little closer to the real reason why we need to
disable UCP for anything older than 10.35, and that is that there is a
bug there that is ONLY relevant if we are using JIT.
My hope though is that with the release of 10.43 (currently in RC1),
10.34 will become irrelevant soon enough and this whole code could be
cleaned up further.
> But the comment is now off, isn't it? The workaround was turning
> PCRE2_UCP off for older versions (because those were broken), not
> turning it on for newer versions (because it would be required by some
> unfixed regression).
The comment was never correct, because it was turning it off, because
the combination of JIT + MATCH_INVALID_UTF (which was released in
10.34) + UCP is broken.
> Do we need to nest the checks for GIT_PCRE2_VERSION_10_35_OR_HIGHER and
> GIT_PCRE2_VERSION_10_43_OR_HIGHER? Keeping them separate would help
> keep the #ifdef parts as short as possible and maintainable
> independently.
I thought that nesting them would make it simpler to maintain, since
there are somehow connected anyway.
The additional flags that are added in PCRE 10.43 are ONLY needed and
useful on top of PCRE2_UCP, which itself only makes sense on top of
UTF.
> > @@ -27,13 +34,13 @@ do
> > if ! test_have_prereq PERF_GREP_ENGINES_THREADS
> > then
> > test_perf "grep -P '$pattern'" --prereq PCRE "
> > - git -P grep -f pat || :
> > + git grep -P -f pat || :
> > "
> > else
> > for threads in $GIT_PERF_GREP_THREADS
> > do
> > test_perf "grep -P '$pattern' with $threads threads" --prereq PTHREADS,PCRE "
> > - git -c grep.threads=$threads -P grep -f pat || :
> > + git -c grep.threads=$threads grep -P -f pat || :
>
> What's going on here? You remove the -P option of git (--no-pager) and
> add the -P option of git grep (--perl-regexp). So this perf test never
> tested PCRE despite its name? Seems worth a separate patch.
My original code was a dud. This would make that at least more obvious,
> Do the performance numbers in acabd2048e (grep: correctly identify utf-8
> characters with \{b,w} in -P, 2023-01-08) still hold up in that light?
FWIW the performance numbers still hold up, but just because I did the
tests independently using hyperfine, and indeed when I did the first
version of this patch, I had a nice reproducible description that
showed how to get 20% better performance while searching the git
repository itself for something like 4 digits (as used in Copyright
dates).
My idea was to reply to my own post with instructions on how to test
this, which basically require to also compile the recently released
10.43RC1 and build against that.
Indeed, AFAIK the test would fail if built with an older version of
PCRE, but wasn't sure if making a prerequisite that hardcoded the
version in test-tool might be the best approach to prevent that,
especially with all the libification work.
Carlo
PS. your reply got lost somehow in my SPAM folder, so I apologize for
the late reply
^ permalink raw reply
* [PATCH V4 1/1] Replace SID with domain/username
From: Sören Krecker @ 2024-01-02 19:15 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <20240102191514.2583-1-soekkle@freenet.de>
Replace SID with domain/username in error message, if owner of repository
and user are not equal on windows systems. Each user should have a unique
SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).
This means that domain/username is not a loss of information. If the translation
fails the message contains the SID as string.
Old Prompted error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
New prompted error massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'DESKTOP-L78JVA6/Administrator'
but the current user is:
'DESKTOP-L78JVA6/test'
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
compat/mingw.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..bfd9573a29 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,27 @@ static PSID get_current_user_sid(void)
return result;
}
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+ SID_NAME_USE pe_use;
+ DWORD len_user = 0, len_domain = 0;
+ BOOL translate_sid_to_user;
+
+ /* returns only FALSE, because the string pointers are NULL*/
+ LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+ &pe_use);
+ /*Alloc needed space of the strings*/
+ ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
+ translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+ *str, &len_domain, &pe_use);
+ if (translate_sid_to_user == FALSE) {
+ FREE_AND_NULL(*str);
+ }
+ else
+ (*str)[len_domain] = '/';
+ return translate_sid_to_user;
+}
+
static int acls_supported(const char *path)
{
size_t offset = offset_1st_component(path);
@@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
} else if (report) {
LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
- if (ConvertSidToStringSidA(sid, &str1))
+ if (user_sid_to_user_name(sid, &str1))
+ to_free1 = str1;
+ else if (ConvertSidToStringSidA(sid, &str1))
to_free1 = str1;
else
str1 = "(inconvertible)";
@@ -2776,7 +2799,10 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
str2 = "(none)";
else if (!IsValidSid(current_user_sid))
str2 = "(invalid)";
- else if (ConvertSidToStringSidA(current_user_sid, &str2))
+ else if (user_sid_to_user_name(current_user_sid, &str2))
+ to_free2 = str2;
+ else if (ConvertSidToStringSidA(current_user_sid,
+ &str2))
to_free2 = str2;
else
str2 = "(inconvertible)";
@@ -2784,8 +2810,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
"'%s' is owned by:\n"
"\t'%s'\nbut the current user is:\n"
"\t'%s'\n", path, str1, str2);
- LocalFree(to_free1);
- LocalFree(to_free2);
+ free(to_free1);
+ free(to_free2);
}
}
--
2.39.2
^ permalink raw reply related
* [PATCH V4 0/1] Replace SID with domain/username on Windows
From: Sören Krecker @ 2024-01-02 19:15 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <xmqqsf3feilq.fsf@gitster.g>
Hi everone,
I improve the commit message with information from Microsoft, fixes a memmory access error and the message in case of an error.
Vielen dank für die Hinweise von Junio C Hamano.
Sören Krecker (1):
Replace SID with domain/username
compat/mingw.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.39.2
^ permalink raw reply
* Re: [PATCH] git-clone.txt: document -4 and -6
From: Taylor Blau @ 2024-01-02 19:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Wilk, git, Eric Wong
In-Reply-To: <xmqq1qivd8d0.fsf@gitster.g>
On Thu, Jun 01, 2023 at 03:06:35PM +0900, Junio C Hamano wrote:
> Jakub Wilk <jwilk@jwilk.net> writes:
>
> > These options were added in c915f11eb4 ("connect & http: support -4 and
> > -6 switches for remote operations").
> >
> > Signed-off-by: Jakub Wilk <jwilk@jwilk.net>
> > ---
> > Documentation/git-clone.txt | 8 ++++++++
> > 1 file changed, 8 insertions(+)
>
> The patch is not _wrong_ per-se, but there are other options that
> are common among the "fetch" family of commands. I counted at least
> these should be shared between "fetch" and "clone", by splitting
> them out of "fetch-options.txt" into a new file, and including that
> new file from "fetch-options.txt" and "git-clone.txt". Those marked
> with (?) are described in different phrasing between "clone" and
> "fetch", and may fall into the "let's keep them separate, because
> they mean different things" category (later):
>
> * --jobs
> * --upload-pack
> * --quiet (?)
> * --verbose (?)
> * --progress
> * --server-option
> * --ipv[46]
>
> Note that these happen to share the same name, but to "clone" and
> "fetch" they different things, so leaving them separate is the right
> thing to do.
>
> * --no-tags
> * --recurse-submodules
I wrote this ugly shell incantation to find an exhaustive list of
potentially shareable options:
$ grep '^-.*::$' <Documentation/fetch-options.txt |
tr -d ':' | sed -e 's/\[=/=[/' -e 's/<[^>]*>//' |
grep -Eo '^[^=]+' | awk -F] '
/\[no-\]/ { print "--" $2; print "--no-" $2; next }
{ print $0 }
' |
while read arg
do
if grep -Fwq -- $arg Documentation/fetch-options.txt &&
grep -Fwq -- $arg Documentation/git-clone.txt
then
echo $arg;
fi
done
It turned up the following results:
-a
--depth
--shallow-since
--shallow-exclude
--no-tags
--recurse-submodules
-j, --jobs
-u, --upload-pack
-q, --quiet
-v, --verbose
--progress
-o, --server-option
-a is a false-positive (it comes from "you can simply run `git repack
-a`", which is in the clone documentation).
Even though depth, and the shallow options are shared by both fetch and
clone, they have different wording in each context, so they should be
kept separate.
I agree with your thinking that `--no-tags` and `--recurse-submodules`
should be kept separate.
That makes our two lists equal (modulo the --ipv[46] options, which were
previously not documented in git-clone(1) until this patch).
Thanks,
Taylor
^ permalink raw reply
* [RFC PATCH 0/6] completion: improvements for git-bisect
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
Bring completion for bisect up to date with current features.
This is RFC mainly because of an implementation issue in patch 2: I think
the mechanism of requiring COMPREPLY to be empty, possibly writing into it,
then checking it post-call is too fragile, but I'm not sure how people would
like to fix this. I'd prefer to just add an assert() of some sort to enforce
the empty-COMPREPLY precondition, rather than rewrite the guts of the former
_git_log() function to use some other intermediate return mechanism (this
would be overkill in this simple context IMO). But I'm not sure how to
do that in a completion context: for sure nothing that is done here should
ever have a chance of showing garbage on a user's screen, but of course for
assert() to be useful a dev would need to see it somehow. Suggestions welcome.
Other than that issue I think it's all reasonable and ready, though there
are a couple other decisions that I guess people might disagree with:
* good/old/new/bad terms are always completed (even if they're
invalild because --term-(good|bad) have been used). The idea here
is that if the user has become confused about their own terms it's
better to let git give them an error message than to have
normally-working completion not happen.
* 'view' is recognized as a subcommand, but not as a completion
candidate. This lets complete of git bisect v<TAB> keep working which
seems convenient and some poor person somewhere probably really wants :)
view is just an alias so loss of interface recall is minimal.
Britton Leo Kerin (6):
completion: complete new old actions, start opts
completion: git-log opts to bisect visualize
completion: move to maintain define-before-use
completion: custom git-bisect terms
completion: recognize but do not complete 'view'
completion: add comment
contrib/completion/git-completion.bash | 310 +++++++++++++++----------
1 file changed, 181 insertions(+), 129 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.43.0
^ permalink raw reply
* [RFC PATCH 1/6] completion: complete new old actions, start opts
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..15d22ff7d9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1449,7 +1449,7 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad good skip reset visualize replay log run"
+ local subcommands="start bad new good old terms skip reset visualize replay log run help"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__git_find_repo_path
@@ -1462,7 +1462,20 @@ _git_bisect ()
fi
case "$subcommand" in
- bad|good|reset|skip|start)
+ start)
+ case "$cur" in
+ --*)
+ __gitcomp "--term-new --term-bad --term-old --term-good --first-parent --no-checkout"
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
+ esac
+
+ case "$subcommand" in
+ bad|new|good|old|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 2/6] completion: git-log opts to bisect visualize
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
To do this the majority of _git_log has been factored out into the new
__git_complete_log_opts. This is needed because the visualize command
accepts git-log options but not rev arguments (they are fixed to the
commits under bisection).
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 30 ++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 15d22ff7d9..3472fab514 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1472,6 +1472,16 @@ _git_bisect ()
;;
esac
;;
+ visualize)
+ case "$cur" in
+ -*)
+ __git_complete_log_opts
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
esac
case "$subcommand" in
@@ -2074,11 +2084,12 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
-{
- __git_has_doubledash && return
- __git_find_repo_path
+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
+# selected option argument completions for git-log options and put them in
+# COMPREPLY.
+__git_complete_log_opts ()
+{
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
merge="--merge"
@@ -2171,6 +2182,17 @@ _git_log ()
return
;;
esac
+
+}
+
+_git_log ()
+{
+ __git_has_doubledash && return
+ __git_find_repo_path
+
+ __git_complete_log_opts
+ [ -z "$COMPREPLY" ] || return
+
__git_complete_revlist
}
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 4/6] completion: custom git-bisect terms
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4940ad3e24..a09598c5c1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1581,10 +1581,19 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad new good old terms skip reset visualize replay log run help"
+ __git_find_repo_path
+
+ local term_bad term_good
+ if [ -f "$__git_repo_path"/BISECT_START ]; then
+ term_bad=`__git bisect terms --term-bad`
+ term_good=`__git bisect terms --term-good`
+ fi
+
+ local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+
local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
if [ -z "$subcommand" ]; then
- __git_find_repo_path
if [ -f "$__git_repo_path"/BISECT_START ]; then
__gitcomp "$subcommands"
else
@@ -1617,7 +1626,7 @@ _git_bisect ()
esac
case "$subcommand" in
- bad|new|good|old|reset|skip|start)
+ bad|new|"$term_bad"|good|old|"$term_good"|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 3/6] completion: move to maintain define-before-use
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 265 ++++++++++++-------------
1 file changed, 132 insertions(+), 133 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3472fab514..4940ad3e24 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1445,6 +1445,138 @@ _git_archive ()
__git_complete_file
}
+# Options that go well for log, shortlog and gitk
+__git_log_common_options="
+ --not --all
+ --branches --tags --remotes
+ --first-parent --merges --no-merges
+ --max-count=
+ --max-age= --since= --after=
+ --min-age= --until= --before=
+ --min-parents= --max-parents=
+ --no-min-parents --no-max-parents
+"
+# Options that go well for log and gitk (not shortlog)
+__git_log_gitk_options="
+ --dense --sparse --full-history
+ --simplify-merges --simplify-by-decoration
+ --left-right --notes --no-notes
+"
+# Options that go well for log and shortlog (not gitk)
+__git_log_shortlog_options="
+ --author= --committer= --grep=
+ --all-match --invert-grep
+"
+# Options accepted by log and show
+__git_log_show_options="
+ --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
+"
+
+__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
+
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
+__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
+
+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
+# selected option argument completions for git-log options and put them in
+# COMPREPLY.
+__git_complete_log_opts ()
+{
+ local merge=""
+ if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ merge="--merge"
+ fi
+ case "$prev,$cur" in
+ -L,:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L,:*)
+ __git_complete_symbol --cur="${cur#:}" --sfx=":"
+ return
+ ;;
+ -G,*|-S,*)
+ __git_complete_symbol
+ return
+ ;;
+ esac
+ case "$cur" in
+ --pretty=*|--format=*)
+ __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
+ " "" "${cur#*=}"
+ return
+ ;;
+ --date=*)
+ __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
+ return
+ ;;
+ --decorate=*)
+ __gitcomp "full short no" "" "${cur##--decorate=}"
+ return
+ ;;
+ --diff-algorithm=*)
+ __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
+ return
+ ;;
+ --submodule=*)
+ __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
+ return
+ ;;
+ --ws-error-highlight=*)
+ __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
+ return
+ ;;
+ --no-walk=*)
+ __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
+ return
+ ;;
+ --diff-merges=*)
+ __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
+ return
+ ;;
+ --*)
+ __gitcomp "
+ $__git_log_common_options
+ $__git_log_shortlog_options
+ $__git_log_gitk_options
+ $__git_log_show_options
+ --root --topo-order --date-order --reverse
+ --follow --full-diff
+ --abbrev-commit --no-abbrev-commit --abbrev=
+ --relative-date --date=
+ --pretty= --format= --oneline
+ --show-signature
+ --cherry-mark
+ --cherry-pick
+ --graph
+ --decorate --decorate= --no-decorate
+ --walk-reflogs
+ --no-walk --no-walk= --do-walk
+ --parents --children
+ --expand-tabs --expand-tabs= --no-expand-tabs
+ $merge
+ $__git_diff_common_options
+ "
+ return
+ ;;
+ -L:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L:*)
+ __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
+ return
+ ;;
+ -G*)
+ __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
+ return
+ ;;
+ -S*)
+ __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
+ return
+ ;;
+ esac
+
+}
+
_git_bisect ()
{
__git_has_doubledash && return
@@ -2052,139 +2184,6 @@ _git_ls_tree ()
__git_complete_file
}
-# Options that go well for log, shortlog and gitk
-__git_log_common_options="
- --not --all
- --branches --tags --remotes
- --first-parent --merges --no-merges
- --max-count=
- --max-age= --since= --after=
- --min-age= --until= --before=
- --min-parents= --max-parents=
- --no-min-parents --no-max-parents
-"
-# Options that go well for log and gitk (not shortlog)
-__git_log_gitk_options="
- --dense --sparse --full-history
- --simplify-merges --simplify-by-decoration
- --left-right --notes --no-notes
-"
-# Options that go well for log and shortlog (not gitk)
-__git_log_shortlog_options="
- --author= --committer= --grep=
- --all-match --invert-grep
-"
-# Options accepted by log and show
-__git_log_show_options="
- --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
-"
-
-__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
-
-__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
-__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-
-
-# Find only porcelain (i.e. not git-rev-list) option (not argument) and
-# selected option argument completions for git-log options and put them in
-# COMPREPLY.
-__git_complete_log_opts ()
-{
- local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
- fi
- case "$prev,$cur" in
- -L,:*:*)
- return # fall back to Bash filename completion
- ;;
- -L,:*)
- __git_complete_symbol --cur="${cur#:}" --sfx=":"
- return
- ;;
- -G,*|-S,*)
- __git_complete_symbol
- return
- ;;
- esac
- case "$cur" in
- --pretty=*|--format=*)
- __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
- " "" "${cur#*=}"
- return
- ;;
- --date=*)
- __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
- return
- ;;
- --decorate=*)
- __gitcomp "full short no" "" "${cur##--decorate=}"
- return
- ;;
- --diff-algorithm=*)
- __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
- return
- ;;
- --submodule=*)
- __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
- return
- ;;
- --ws-error-highlight=*)
- __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
- return
- ;;
- --no-walk=*)
- __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
- return
- ;;
- --diff-merges=*)
- __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
- return
- ;;
- --*)
- __gitcomp "
- $__git_log_common_options
- $__git_log_shortlog_options
- $__git_log_gitk_options
- $__git_log_show_options
- --root --topo-order --date-order --reverse
- --follow --full-diff
- --abbrev-commit --no-abbrev-commit --abbrev=
- --relative-date --date=
- --pretty= --format= --oneline
- --show-signature
- --cherry-mark
- --cherry-pick
- --graph
- --decorate --decorate= --no-decorate
- --walk-reflogs
- --no-walk --no-walk= --do-walk
- --parents --children
- --expand-tabs --expand-tabs= --no-expand-tabs
- $merge
- $__git_diff_common_options
- "
- return
- ;;
- -L:*:*)
- return # fall back to Bash filename completion
- ;;
- -L:*)
- __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
- return
- ;;
- -G*)
- __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
- return
- ;;
- -S*)
- __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
- return
- ;;
- esac
-
-}
-
_git_log ()
{
__git_has_doubledash && return
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 6/6] completion: add comment
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3bb790220a..7f9a626e1b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1589,8 +1589,14 @@ _git_bisect ()
term_good=`__git bisect terms --term-good`
fi
+ # We will complete any custom terms, but still always complete the
+ # more usual bad/new/good/old because git bisect gives a good error
+ # message if these are given when not in use and that's better than
+ # silent refusal to complete if the user is confused.
+ #
# We want to recognize 'view' but not complete it, because it overlaps
# with 'visualize' too much and is just an alias for it.
+ #
local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
local all_subcommands="$completable_subcommands view"
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 5/6] completion: recognize but do not complete 'view'
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Completing it might annoy some existing users by creating completion
ambiguity on 'v' and 'vi' without adding anything useful in terms of
interface discovery/recall (because 'view' is just an alias anyway).
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a09598c5c1..3bb790220a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1589,13 +1589,16 @@ _git_bisect ()
term_good=`__git bisect terms --term-good`
fi
- local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ # We want to recognize 'view' but not complete it, because it overlaps
+ # with 'visualize' too much and is just an alias for it.
+ local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ local all_subcommands="$completable_subcommands view"
- local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ local subcommand="$(__git_find_on_cmdline "$all_subcommands")"
if [ -z "$subcommand" ]; then
if [ -f "$__git_repo_path"/BISECT_START ]; then
- __gitcomp "$subcommands"
+ __gitcomp "$completable_subcommands"
else
__gitcomp "replay start"
fi
@@ -1613,7 +1616,7 @@ _git_bisect ()
;;
esac
;;
- visualize)
+ visualize|view)
case "$cur" in
-*)
__git_complete_log_opts
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] git-clone.txt: document -4 and -6
From: Taylor Blau @ 2024-01-02 20:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Wilk, git, Eric Wong
In-Reply-To: <ZZRqgmDycyAXCrGZ@nand.local>
On Tue, Jan 02, 2024 at 02:56:50PM -0500, Taylor Blau wrote:
> It turned up the following results:
>
> -a
> --depth
> --shallow-since
> --shallow-exclude
> --no-tags
> --recurse-submodules
> -j, --jobs
> -u, --upload-pack
> -q, --quiet
> -v, --verbose
> --progress
> -o, --server-option
Hmm. I think the story is a little more complicated. Just looking at the
ones that we agree on:
* -j, --jobs
* -u, --upload-pack
* --progress
* -o, --server-option
* --[ipv]4
* --[ipv]6
Note that the 'clone' and 'fetch' versions for many of these options
have different wording. For example, in Documentation/git-clone.txt we
have:
-j::
--jobs=<n>::
Number of parallel children to be used for all forms of fetching.
Whereas the description in the original fetch-options.txt is more
verbose.
In fact, the story is even more complicated than that, since even though
the 'push' builtin would benefit from having a shared source of
documentation for the --ipv4 and --ipv6 options, 'push' does not have a
--jobs option.
'clone', 'fetch', and 'pull' all share an '--upload-pack' option as you
note, though 'push' naturally doesn't (so we would need another ifdef
there, too). But the instances in 'clone', 'fetch', and 'pull' all
differ in their wording (e.g., the 'clone' documentation says "the
repository to clone from ..." but the others say "the repository to
fetch from ...").
--progress could be shared, as could --server-option, and the two
--ipv4/6 options. But the number of nested ifdefs necessary to share the
other options probably dose not justify the effort to do so.
Thanks,
Taylor
^ permalink raw reply
* [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Ghanshyam Thakkar @ 2024-01-02 22:07 UTC (permalink / raw)
To: git; +Cc: newren, gitster, johannes.schindelin
Hello,
I'm currently an undergrad beginning my journey of contributing to the
Git project. I am seeking feedback on doing "Heed core.bare from
template config file when no command line override given" described
here
https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/
by Elijah Newren, as a microproject. I would like to know from the
community, if the complexity and scope of the project is appropriate
for a microproject.
Approach:
As described by Elijah in commit message that fixing this cannot be
done in the create_default_files() function as it occurs too late.
This is because both clone and init have different checks and steps
for working with bare flag, like clone creates a new directory
[name].git and sets the GIT_DIR_ENVIRONMENT to it (when not provided
with an explicit dir name arg), while init sets the
GIT_DIR_ENVIRONMENT to the current working directory (when not
provided with an dir name arg). Also there are other steps like
setting no_checkout in a bare repository in builtin/clone.c. These are
all command specific steps which occur in builtin/clone.c and
builtin/init-db.c ,before we ever hit the TODO comment via
[builtin/clone.c]cmd_clone()->[setup.c]init_db()->
[setup.c]create_default_files().
Therefore, rather than centralizing the code in setup.c and adding a
bunch of if-else statements to handle different command specific
scenarios related to bare option, I propose to add a check for
template file config just after parsing of the flags and args in
builtin/init-db.c and builtin/clone.c.
e.g. in builtin/init-db.c :
static int template_bare_config(const char *var, const char *value,
const struct config_context *ctx, void *cb)
{
if(!strcmp(var,"core.bare")) {
is_bare_repository_cfg = git_config_bool(var, value);
}
return 0;
}
int cmd_init_db(int argc, const char **argv, const char *prefix)
{
...
...
if(is_bare_repository_cfg==-1) {
if(!template_dir)
git_config_get_pathname("init.templateDir",
&template_dir);
if(template_dir) {
const char* template_config_path
= xstrfmt("%s/config",
struct stat st;
if(!stat(template_config_path, &st) &&
!S_ISDIR(st.st_mode)) {
git_config_from_file(template_bare_cfg,
template_config_path, NULL);
}
}
...
...
return init_db(git_dir, real_git_dir, template_dir, hash_algo,
initial_branch, init_shared_repository, flags);
}
I also wanted to know if the global config files should have an effect
in deciding if the repo is bare or not.
Curious to know your thoughts on, if this is the right approach or
does it require doing refactoring to bring all the logic in setup.c.
Based on your feedback, I can quickly send a patch.
p.s.: Apologies for the weird indenting, due to 70 character limit.
consider it just a prototype.
Thanks!
^ permalink raw reply
* Re: [PATCH] git-clone.txt: document -4 and -6
From: Junio C Hamano @ 2024-01-02 22:15 UTC (permalink / raw)
To: Taylor Blau; +Cc: Jakub Wilk, git, Eric Wong
In-Reply-To: <ZZRzxZNb2Aq+2feW@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> Note that the 'clone' and 'fetch' versions for many of these options
> have different wording. For example, in Documentation/git-clone.txt we
> have:
>
> -j::
> --jobs=<n>::
> Number of parallel children to be used for all forms of fetching.
>
> Whereas the description in the original fetch-options.txt is more
> verbose.
Yes, so it will be impossible to unify without changing the
resulting text. But unless one description is clearly better for
one subcommand while the other description is also clearly better
for the other subcommand, we should be able to pick a better one
that would serve both subcommands, and that way we would improve
description for one subcommand while keeping the other one the same,
right?
> In fact, the story is even more complicated than that, since even though
> the 'push' builtin would benefit from having a shared source of
> documentation for the --ipv4 and --ipv6 options, 'push' does not have a
> --jobs option.
Sure, it won't be just "write what is shared across all the transfer
commands in a single file and include it from all". The direction
of transfer is a reason why the options may differ, of course, so we
may need to have two (or three) include files if we want to go that
route.
> --progress could be shared, as could --server-option, and the two
> --ipv4/6 options. But the number of nested ifdefs necessary to share the
> other options probably dose not justify the effort to do so.
^ permalink raw reply
* Re: [BUG] Asks for "To" even if "To" already specified in letter
From: Taylor Blau @ 2024-01-02 22:23 UTC (permalink / raw)
To: Askar Safin; +Cc: git
In-Reply-To: <CAPnZJGBbA1VLAdP5mZnbF77-x-87JjU3Ku2MhrQu0SFoJL7ggw@mail.gmail.com>
On Sat, Dec 30, 2023 at 06:20:43AM +0300, Askar Safin wrote:
> Hi. I found a bug. Steps to reproduce:
> - Create file /tmp/m with following text:
> ===
> Subject: subj
> To: example@example.com
>
> text
> ===
> - Send it using command "git send-email /tmp/m"
>
> You will see that git asks for "To". git says: "To whom should the
> emails be sent (if anyone)?"
> I don't like this. git should just use "To" from /tmp/m without asking.
>
> Seen with git 2.43.0.
>
> If I execute "git send-email --to-cmd='#' /tmp/m" or
> "git send-email --to-cmd=':' /tmp/m" or
> "git send-email --to-cmd='true' /tmp/m", then "To" is not asked.
I was going to suggest that you use the `--to-cmd=true` trick. I'm
definitely not an expert in the send-email code, but from a cursory
look, I think that this is do-able.
One thing you could do is read all of the messages ahead of time, parse
their headers, and then extract any "To:" headers that you find. This is
pretty similar to what the pre_process_file() function is already doing.
But this might not be the right approach, since FIFOs can only be read
once, and we already have some logic to handle FIFOs specially (see
3c8d3adeae (send-email: export patch counters in validate environment,
2023-04-14)).
But I think that something much simpler would work, which to avoid
asking for a "To:" value altogether, even if one isn't provided. If you
did something like:
--- 8< ---
diff --git a/git-send-email.perl b/git-send-email.perl
index 821b2b3a13..2941278315 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1018,13 +1018,6 @@ sub file_declares_8bit_cte {
my $to_whom = __("To whom should the emails be sent (if anyone)?");
my $prompting = 0;
-if (!@initial_to && !defined $to_cmd) {
- my $to = ask("$to_whom ",
- default => "",
- valid_re => qr/\@.*\./, confirm_only => 1);
- push @initial_to, parse_address_line($to) if defined $to; # sanitized/validated later
- $prompting++;
-}
sub expand_aliases {
return map { expand_one_alias($_) } @_;
--- >8 ---
I think that would more or less do the trick. send-email will happily
continue on even without an initial $to value, and validate it later
when it's actually needed.
I'm not familiar enough with the code to know if this is the right
approach, so hopefully some other send-email experts can chime in and
let me know if I'm on the right track ;-).
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH] git-clone.txt: document -4 and -6
From: Taylor Blau @ 2024-01-02 22:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Wilk, git, Eric Wong
In-Reply-To: <xmqqr0izcrfm.fsf@gitster.g>
On Tue, Jan 02, 2024 at 02:15:57PM -0800, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Note that the 'clone' and 'fetch' versions for many of these options
> > have different wording. For example, in Documentation/git-clone.txt we
> > have:
> >
> > -j::
> > --jobs=<n>::
> > Number of parallel children to be used for all forms of fetching.
> >
> > Whereas the description in the original fetch-options.txt is more
> > verbose.
>
> Yes, so it will be impossible to unify without changing the
> resulting text. But unless one description is clearly better for
> one subcommand while the other description is also clearly better
> for the other subcommand, we should be able to pick a better one
> that would serve both subcommands, and that way we would improve
> description for one subcommand while keeping the other one the same,
> right?
Right. I meant to illustrate merely that this would probably involve
some word-smithing instead of just pure cut-and-paste, so that it may
not be worth the effort.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2] mem-pool: fix big allocations
From: Taylor Blau @ 2024-01-02 22:29 UTC (permalink / raw)
To: René Scharfe
Cc: Git List, Jameson Miller, Phillip Wood, Elijah Newren,
Junio C Hamano
In-Reply-To: <1c39c0e7-05b2-4726-a90c-f78df4356a41@web.de>
Hi René,
On Thu, Dec 28, 2023 at 08:19:06PM +0100, René Scharfe wrote:
> Changes since v1:
> - simply use check() instead of a custom check_ptr() macro
> - drop unnecessary comparison of next_free and end pointers
Great find and fix. It's nice to see some examples of the testing
framework being used. I'll have to play around with it sometime :-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] sideband.c: replace int with size_t for clarity
From: Taylor Blau @ 2024-01-02 22:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, Chandra Pratap via GitGitGadget, git,
Chandra Pratap, Chandra Pratap
In-Reply-To: <xmqqedfejc32.fsf@gitster.g>
On Fri, Dec 22, 2023 at 11:01:37AM -0800, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
> Just this part.
>
> > Further down, we read
> > for (i = 0; i < ARRAY_SIZE(keywords); i++) {
> >
> > However, a size of an array can never be negative, so that
> > an unsigned data type is a better choice than a signed.
> > And, arrays can have more elements than an int can address,
> > at least in theory.
> > For a reader it makes more sense, to replace
> > int i;
> > with
> > size_t i;
>
> It is a very good discipline to use size_t to index into an array
> whose size is externally controled (e.g., we slurp what the end user
> or the server on the other end of the connection gave us into a
> piece of memory we allocate) to avoid integer overflows as "int" is
> often narrower than "size_t". But this particular one is a Meh; the
> keywords[] is a small hardcoded array whose size and contents are
> totally under our control.
I certainly agree in theory, though I've always erred on the side of
always using size_t for indexing into arrays, even if they're small. It
removes a potential pitfall if you are working with an
externally-controlled array and happen to forget to use size_t.
But if there is an existing index variable with type "int", and we can
easily validate that it's small, I probably wouldn't bother changing it
if I was editing nearby code.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH V4 1/1] Replace SID with domain/username
From: Junio C Hamano @ 2024-01-03 0:43 UTC (permalink / raw)
To: Sören Krecker, Johannes Schindelin; +Cc: git, sunshine
In-Reply-To: <20240102191514.2583-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Replace SID with domain/username in error message, if owner of repository
> and user are not equal on windows systems. Each user should have a unique
> SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).
That paragraph your URL refers to does say that a SID that is used
for an account will never be reused to identify a different account.
But I am not sure if it means a user will never be assigned more
than one SID (in other words, the reverse is not necessarily true).
The paragraph also mentions that a SID can identify a non-user
entity like a computer account (as opposed to "a user account")---I
do not know what its implications are in the context of this patch,
though.
> This means that domain/username is not a loss of information.
This statement does not (grammatically) make sense, but more
importantly, loss of information may not be a bad thing in this
case. If more than one SIDs are given to a user account and
processes working for that account, these different SIDs may be
translated, by using LookupAccountSidA(), to the same string for a
single user@domain, and it would be an operation that loses
information in that sense.
But if what we *care* about is user@domain between the current
process and the owner of the directory in question being the same
(or not), then such a loss of information is a *good* thing.
So I dunno. Arguing what we care about (is that exact SID equality
between the "owner of the directory" and the "user, which the
current process is working on behalf of", or do we care about the
equality of the "accounts"?) may be a better way to justify this
change, if you ask me.
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /* returns only FALSE, because the string pointers are NULL*/
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
> + /*Alloc needed space of the strings*/
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> + *str, &len_domain, &pe_use);
> + if (translate_sid_to_user == FALSE) {
> + FREE_AND_NULL(*str);
> + }
Style: do not enclose a single-statement block inside {}.
> + else
> + (*str)[len_domain] = '/';
> + return translate_sid_to_user;
> +}
> @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
> } else if (report) {
> LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
>
> - if (ConvertSidToStringSidA(sid, &str1))
> + if (user_sid_to_user_name(sid, &str1))
> + to_free1 = str1;
> + else if (ConvertSidToStringSidA(sid, &str1))
> to_free1 = str1;
Do these two helper functions return pointers pointing into the same
kind of memory that you can free with the same function? That is ...
> ...
> "'%s' is owned by:\n"
> "\t'%s'\nbut the current user is:\n"
> "\t'%s'\n", path, str1, str2);
> - LocalFree(to_free1);
> - LocalFree(to_free2);
> + free(to_free1);
> + free(to_free2);
... the original code seems to say that the piece of memory we
obtain from ConvertSidToStringSidA() must not be freed by calling
free() but use something special called LocalFree(). I am assuing
that your user_sid_to_user_name() returns a regular piece of memory
that can be freed by calling regular free()? Do we need to keep
track of where we got the memory from and use different function to
free each variable, or something (again I do not do Windows so I'll
defer all of these to Dscho, who is CC'ed this time).
Thanks and a happy new year.
> }
> }
^ 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