* Re: [PATCH v3] sideband.c: remove redundant 'NEEDSWORK' tag
From: Junio C Hamano @ 2023-12-27 22:22 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget
Cc: git, Torsten Bögershausen, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1625.v3.git.1703672407895.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> - * NEEDSWORK: use "size_t n" instead for clarity.
> ++ * It is fine to use "int n" here instead of "size_t n" as all calls to this
> ++ * function pass an 'int' parameter.
This does not sound like a sufficient justification, though.
We should also explain why "int" is good enough for these callers.
Otherwise, using size_t throughout the callchain would become
another viable solution.
^ permalink raw reply
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Junio C Hamano @ 2023-12-27 22:15 UTC (permalink / raw)
To: Elijah Newren
Cc: Elijah Newren via GitGitGadget, git, Sebastian Thiel,
Josh Triplett
In-Reply-To: <CABPp-BGSTYDUR1oYYXkCSh-1i2zwxBM=-gnoe-ezNbtPi5CV2A@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> There are
> precisely two choices in our design for how older Git versions can
> treat precious files:
> * ignored-and-expendable
> * untracked-and-precious
> If we pick syntax that causes older Git versions to treat precious
> files as ignored-and-expendable, we risk deleting important files.
Yes but not really. I'd expect the adoption of precious feature and
the adoption of versions of Git that supports that feature will go
more or less hand in hand. Projects that, for any reason, need to
keep their participants at pre-precious versions of Git would
naturally refrain from marking the "precious" paths in their "ignore"
mechanism before their participants are ready, so even if we chose
syntax that will make the precious ones mistaken as merely ignored,
the damage would be fairly small.
^ permalink raw reply
* [PATCH 1/1] doc: use singular form of repeatable path arg
From: Britton Leo Kerin @ 2023-12-27 20:53 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin, Britton L Kerin
In-Reply-To: <20231227205340.9886-1-britton.kerin@gmail.com>
This is more correct because the <path>... doc syntax already indicates
that the arg is "array-type". It's how other tools do it. Finally, the
later document text mentions 'path' arguments, while it doesn't mention
'paths'.
Signed-off-by: Britton L Kerin <britton.kergin@gmail.com>
---
Documentation/git-bisect.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 191b4a42b6..58821f7e11 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending
on the subcommand:
git bisect start [--term-(new|bad)=<term-new> --term-(old|good)=<term-old>]
- [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
+ [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<path>...]
git bisect (bad|new|<term-new>) [<rev>]
git bisect (good|old|<term-old>) [<rev>...]
git bisect terms [--term-good | --term-bad]
--
2.43.0
^ permalink raw reply related
* [PATCH 0/1] doc: git-bisect: change plural form to singular
From: Britton Leo Kerin @ 2023-12-27 20:53 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
Correct the usage in git-bisect documentation to use singular form for a
repeatable argument as other commands do.
I reported this tiny issue previously but didn't see a response so I
thought I'd use it as a chance to get up to speed on the patch
submission process. Sorry if no response meant no interest on this
issue.
Britton Leo Kerin (1):
doc: use singular form of repeatable path arg
Documentation/git-bisect.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
--
2.43.0
^ permalink raw reply
* Re: Suppress 'info: please complete authentication in browser'
From: brian m. carlson @ 2023-12-27 17:47 UTC (permalink / raw)
To: Brian Hart; +Cc: git@vger.kernel.org
In-Reply-To: <1858825158.1864122.1703697091856@mailbusiness.ionos.com>
[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]
On 2023-12-27 at 17:11:31, Brian Hart wrote:
> Hello,
Hey,
> Here is the .sh script I am using, e.g., to do a pull on each directory in a root directory:
>
> ```
> git config --global user.email "XXXXX" --replace-all; git config --global user.name "XXXXXX" --replace-all; git config --global credential.helper wincred --replace-all; for d in */; do cd $d; echo Processing repository in "${d%/}"...; git config credential.helper wincred; git remote update; git pull --all -v --no-rebase; git add --renormalize .; cd ..; done
> ```
If your goal is to use a specific configuration, you may find it easier
to use the GIT_CONFIG_GLOBAL environment variable to set a custom global
configuration file and then GIT_CONFIG_NOSYSTEM=1 to unset the system
configuration from being used. My guess is that there's a
`credential.helper=manager` setting in some config file, which you'd be
able to see with `git config -l --show-origin`, and that using this
configuration would fix that.
However, if you see this problem after that, I'd recommend contacting
the Git for Windows team on their issue tracker at
https://github.com/git-for-windows/git/issues/ or the Git Credential
Manager Core team at
https://github.com/git-ecosystem/git-credential-manager/issues/. The
Git project itself doesn't distribute any binaries or anything but the
core Git code itself, so if you have a problem with Git for Windows or
Git Credential Manager Core, you should contact those folks directly.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Suppress 'info: please complete authentication in browser'
From: Brian Hart @ 2023-12-27 17:11 UTC (permalink / raw)
To: git@vger.kernel.org
Git for Windows version: 2.43.0
Hello,
I am running a nightly shell script that syncs my local and remote Git repos. It kicks off at 7:30 PM each day. When I come into work the following morning, sporadically, I find that part of the script has not been executing for hours because a modal popup displays.
The text in the console is:
`info: please complete authentication in your browser`
At the same time, a modal pop-up appears. The modal popup needs a button clicked to open the browser and sign in. Upon doing so, the browser says "authentication successful," and the script continues -- but this is after I've woken up from being asleep, with the script otherwise sitting suspended all night long.
I've heard various suggestions before about using the Windows Credential Manager (I believe I am already) and such. It seems as if Git is programmed to ignore the Credential Manager periodically and demand the use of the browser to re-authenticate.
Here is the .sh script I am using, e.g., to do a pull on each directory in a root directory:
```
git config --global user.email "XXXXX" --replace-all; git config --global user.name "XXXXXX" --replace-all; git config --global credential.helper wincred --replace-all; for d in */; do cd $d; echo Processing repository in "${d%/}"...; git config credential.helper wincred; git remote update; git pull --all -v --no-rebase; git add --renormalize .; cd ..; done
```
As you can see I am using `git config --global credential.helper wincred --replace-all` and I am not sure why this should not work. NOTE: The 'XXXX' are for privacy.
How can I suppress the modal pop-up so I can run my overnight Git scripts?
Regards,
Brian Hart
^ permalink raw reply
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Phillip Wood @ 2023-12-27 14:40 UTC (permalink / raw)
To: René Scharfe, Christian Couder, Junio C Hamano
Cc: Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon
In-Reply-To: <f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de>
On 27/12/2023 11:57, René Scharfe wrote:
> Am 27.12.23 um 11:57 schrieb Christian Couder:
>> On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Achu Luma <ach.lumap@gmail.com> writes:
>>
>>>> +/* 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.
>>
>> I have added Phillip and Josh in Cc: as they might have ideas about this.
>
> You can write custom messages for custom tests using test_assert().
Another possibility is to do
for (int i = 0; i < 256; i++) {
if (!check_int(func(i), ==, is_in(string, i))
test_msg(" i: %02x", i);
}
To print the character code as well as the actual and expected return
values of check_int(). The funny spacing is intended to keep the output
aligned. I did wonder if we should be using
check(func(i) == is_in(string, i))
instead of check_int() but I think it is useful to have the return value
printed on error in case we start returning "53" instead of "1" for
"true" [1]. With the extra test_msg() above we can now see if the test
fails because of a mis-categorization or because func() returned a
different non-zero value when we were expecting "1".
>> Also it might not be a big issue here, but when the new unit test
>> framework was proposed, I commented on the fact that "left" and
>> "right" were perhaps a bit less explicit than "actual" and "expected".
If people are worried about this then it would be possible to change the
check_xxx() macros pass the stringified relational operator into the
various check_xxx_loc() functions and then print "expected" and "actual"
when the operator is "==" and "left" and "right" otherwise.
Best Wishes
Phillip
[1] As an aside I wonder if the ctype functions would make good test
balloons for using _Bool by changing sane_istest() to be
#define sane_istest(x,mask) ((bool)(sane_ctype[(unsigned char)(x)] &
(mask)))
so that we check casting to _Bool coerces non-zero values to "1"
^ permalink raw reply
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: René Scharfe @ 2023-12-27 11:57 UTC (permalink / raw)
To: Christian Couder, Junio C Hamano
Cc: Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon
In-Reply-To: <CAP8UFD1vC6=7ESx1w7S9Q9P0Bsc+c03wgHToNBaP+ivvm9BKBg@mail.gmail.com>
Am 27.12.23 um 11:57 schrieb Christian Couder:
> On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Achu Luma <ach.lumap@gmail.com> writes:
>
>>> +/* 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.
>
> I have added Phillip and Josh in Cc: as they might have ideas about this.
You can write custom messages for custom tests using test_assert().
> Also it might not be a big issue here, but when the new unit test
> framework was proposed, I commented on the fact that "left" and
> "right" were perhaps a bit less explicit than "actual" and "expected".
True.
>>> +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.
The added repetition is a bit grating. With a bit of setup, loop
unrolling and stringification you can retain the property of only having
to mention the class name once. Demo patch below.
>> 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.
>
> Yeah, I agree.
Indeed: test-ctype does unit tests, so the unit test framework should
be a perfect fit. It still feels a bit raw that this point, though,
but that's to be expected.
René
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
index 41189ba9f9..7903856cec 100644
--- a/t/unit-tests/t-ctype.c
+++ b/t/unit-tests/t-ctype.c
@@ -13,13 +13,23 @@ static int is_in(const char *s, int ch)
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)); \
+struct ctype {
+ const char *name;
+ const char *expect;
+ int actual[256];
+};
+
+static void test_ctype(const struct ctype *class)
+{
+ for (int i = 0; i < 256; i++) {
+ int expect = is_in(class->expect, i);
+ int actual = class->actual[i];
+ int res = test_assert(TEST_LOCATION(), class->name,
+ actual == expect);
+ if (!res)
+ test_msg("%s classifies char %d (0x%02x) wrongly",
+ class->name, i, i);
+ }
}
#define DIGIT "0123456789"
@@ -40,37 +50,39 @@ static void test_ctype_##func(void) \
"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
"\x7f"
-TEST_CTYPE_FUNC(isdigit, DIGIT)
-TEST_CTYPE_FUNC(isspace, " \n\r\t")
-TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
-TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
-TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
-TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
-TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
-TEST_CTYPE_FUNC(isascii, ASCII)
-TEST_CTYPE_FUNC(islower, LOWER)
-TEST_CTYPE_FUNC(isupper, UPPER)
-TEST_CTYPE_FUNC(iscntrl, CNTRL)
-TEST_CTYPE_FUNC(ispunct, PUNCT)
-TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
-TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+#define APPLY16(f, n) \
+ f(n + 0x0), f(n + 0x1), f(n + 0x2), f(n + 0x3), \
+ f(n + 0x4), f(n + 0x5), f(n + 0x6), f(n + 0x7), \
+ f(n + 0x8), f(n + 0x9), f(n + 0xa), f(n + 0xb), \
+ f(n + 0xc), f(n + 0xd), f(n + 0xe), f(n + 0xf)
+#define APPLY256(f) \
+ APPLY16(f, 0x00), APPLY16(f, 0x10), APPLY16(f, 0x20), APPLY16(f, 0x30),\
+ APPLY16(f, 0x40), APPLY16(f, 0x50), APPLY16(f, 0x60), APPLY16(f, 0x70),\
+ APPLY16(f, 0x80), APPLY16(f, 0x90), APPLY16(f, 0xa0), APPLY16(f, 0xb0),\
+ APPLY16(f, 0xc0), APPLY16(f, 0xd0), APPLY16(f, 0xe0), APPLY16(f, 0xf0),\
+
+#define CTYPE(name, expect) { #name, expect, { APPLY256(name) } }
int cmd_main(int argc, const char **argv) {
+ struct ctype classes[] = {
+ CTYPE(isdigit, DIGIT),
+ CTYPE(isspace, " \n\r\t"),
+ CTYPE(isalpha, LOWER UPPER),
+ CTYPE(isalnum, LOWER UPPER DIGIT),
+ CTYPE(is_glob_special, "*?[\\"),
+ CTYPE(is_regex_special, "$()*+.?[\\^{|"),
+ CTYPE(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"),
+ CTYPE(isascii, ASCII),
+ CTYPE(islower, LOWER),
+ CTYPE(isupper, UPPER),
+ CTYPE(iscntrl, CNTRL),
+ CTYPE(ispunct, PUNCT),
+ CTYPE(isxdigit, DIGIT "abcdefABCDEF"),
+ CTYPE(isprint, LOWER UPPER DIGIT PUNCT " "),
+ };
/* 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");
+ for (int i = 0; i < ARRAY_SIZE(classes); i++)
+ TEST(test_ctype(&classes[i]), "%s works", classes[i].name);
return test_done();
}
^ permalink raw reply related
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Christian Couder @ 2023-12-27 10:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon
In-Reply-To: <xmqqsf3ohkew.fsf@gitster.g>
On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Achu Luma <ach.lumap@gmail.com> writes:
> > +/* 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.
I have added Phillip and Josh in Cc: as they might have ideas about this.
Also it might not be a big issue here, but when the new unit test
framework was proposed, I commented on the fact that "left" and
"right" were perhaps a bit less explicit than "actual" and "expected".
> > +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.
Yeah, I agree.
> Let's see how others react and then queue.
>
> Thanks.
^ permalink raw reply
* [PATCH v3] sideband.c: remove redundant 'NEEDSWORK' tag
From: Chandra Pratap via GitGitGadget @ 2023-12-27 10:20 UTC (permalink / raw)
To: git; +Cc: Torsten Bögershausen, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1625.v2.git.1703351016486.gitgitgadget@gmail.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
sideband.c: replace int with size_t for clarity
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1625
Range-diff vs v2:
1: fdacc69ae3b ! 1: 273415aa6a4 sideband.c: replace int with size_t for clarity
@@ Metadata
Author: Chandra Pratap <chandrapratap3519@gmail.com>
## Commit message ##
- sideband.c: replace int with size_t for clarity
-
- Replace int with size_t for non-negative values to improve
- clarity and remove the 'NEEDSWORK' tag associated with it.wq
+ sideband.c: remove redundant 'NEEDSWORK' tag
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
@@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
* passed as the first N characters of the SRC array.
*
- * NEEDSWORK: use "size_t n" instead for clarity.
++ * It is fine to use "int n" here instead of "size_t n" as all calls to this
++ * function pass an 'int' parameter.
*/
--static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
-+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n)
+ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
{
- int i;
-
-@@ sideband.c: static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
-
- for (i = 0; i < ARRAY_SIZE(keywords); i++) {
- struct keyword_entry *p = keywords + i;
-- int len = strlen(p->keyword);
-+ size_t len = strlen(p->keyword);
-
- if (n < len)
- continue;
sideband.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sideband.c b/sideband.c
index 6cbfd391c47..a89201d52ac 100644
--- a/sideband.c
+++ b/sideband.c
@@ -69,7 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
* of the line. This should be called for a single line only, which is
* passed as the first N characters of the SRC array.
*
- * NEEDSWORK: use "size_t n" instead for clarity.
+ * It is fine to use "int n" here instead of "size_t n" as all calls to this
+ * function pass an 'int' parameter.
*/
static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
{
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Elijah Newren @ 2023-12-27 6:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren via GitGitGadget, git, Sebastian Thiel,
Josh Triplett
In-Reply-To: <xmqq8r5gfc3j.fsf@gitster.g>
On Tue, Dec 26, 2023 at 9:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > We have traditionally considered all ignored files to be expendable, but
> > users occasionally want ignored files that are not considered
> > expendable. Add a design document covering how to split ignored files
> > into two types: 'trashable' (what all ignored files are currently
> > considered) and 'precious' (the new type of ignored file).
>
> The proposed syntax is a bit different from what I personally prefer
> (which is Phillip's [P14] or something like it), but I consider that
> the more valuable parts of this document is about how various
> commands ought to interact with precious paths, which shouldn't
> change regardless of the syntax.
I agree that syntax and command behavior are mostly separate issues,
but unfortunately they are not orthogonal. In particular, syntax of
precious file specification is directly tied to fallback behavior for
older Git clients, and it might potentially affect backward
compatibility of non-cone-mode sparse-checkout syntax as well.
I think fallback behavior is of particular importance. There are
precisely two choices in our design for how older Git versions can
treat precious files:
* ignored-and-expendable
* untracked-and-precious
If we pick syntax that causes older Git versions to treat precious
files as ignored-and-expendable, we risk deleting important files.
Alternatively, if we pick syntax that causes older Git versions to
treat precious files as untracked-and-precious, they won't be ignored
by e.g. git-status, and are easier to accidentally add with git-add.
I felt the "precious" bit was much more important than the "ignored"
bit of "precious" files, so I thought untracked-and-precious was a
better fallback. However, to get that, we have to rule out lots of
the syntax proposals, such as Phillip's [P14].
Anyway, I'm open to alternative syntax, but we need to measure it
against the relevant criteria, which I believe are:
(A) ease for users to understand, remember, and use
(B) size of backward compatibility break with .gitignore syntax
(C) appropriateness of implied fallback behavior for older Git
clients with precious files
(D) room for additional extension in .gitignore files
(E) potential affects on backward compatibility of non-cone
sparse-checkout syntax
We probably also need to agree on the relative importance of these
criteria; personally, I would probably order them from most important
to least as C, B, E, A, D.
Phillip's P14 is better for D, and perhaps a little better for B, but
I thought slightly worse for A, and much worse for C. (I think
there's no significant relative difference for E between his proposed
syntax and mine.)
> Thanks for putting this together.
^ permalink raw reply
* Re: [PATCH] precious-files.txt: new document proposing new precious file type
From: Junio C Hamano @ 2023-12-27 5:28 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget
Cc: git, Sebastian Thiel, Josh Triplett, Elijah Newren
In-Reply-To: <pull.1627.git.1703643931314.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> We have traditionally considered all ignored files to be expendable, but
> users occasionally want ignored files that are not considered
> expendable. Add a design document covering how to split ignored files
> into two types: 'trashable' (what all ignored files are currently
> considered) and 'precious' (the new type of ignored file).
The proposed syntax is a bit different from what I personally prefer
(which is Phillip's [P14] or something like it), but I consider that
the more valuable parts of this document is about how various
commands ought to interact with precious paths, which shouldn't
change regardless of the syntax.
Thanks for putting this together.
^ permalink raw reply
* [PATCH] precious-files.txt: new document proposing new precious file type
From: Elijah Newren via GitGitGadget @ 2023-12-27 2:25 UTC (permalink / raw)
To: git; +Cc: Sebastian Thiel, Josh Triplett, Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
We have traditionally considered all ignored files to be expendable, but
users occasionally want ignored files that are not considered
expendable. Add a design document covering how to split ignored files
into two types: 'trashable' (what all ignored files are currently
considered) and 'precious' (the new type of ignored file).
Helped-by: Sebastian Thiel <sebastian.thiel@icloud.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
precious-files.txt: new document proposing new precious file type
A couple months ago, we had another in-depth discussion of precious
files[1]. As with previous times, multiple strategies were discussed
(including multiple new ones), meaning we keep making the possible
solution space wider and never nail down an agreed path. I also got the
feeling we were potentially pigeonholing on a subset of the problem
space, and I thought it'd be good to better enumerate what areas of Git
are affected.
So, I went through the exercise of creating a design document to: (1)
provide a specific design proposal and explore it, (2) cover at a high
level the breadth of issues that an implementor needs to at least think
about and which reviewers should be aware of in terms of readiness of a
potential implementation, and (3) provide links to other discussions and
alternative proposals for completeness.
I had some off-list discussions with Sebastian about this proposal, and
he provided some helpful feedback. The idea at this point is that if
folks agree with the general direction, that he is going to be
implementing at least the first cut basic capability. I'll help review
changes, but I'm mostly interested in avoiding unfortunate surprises.
So...does the proposed direction seem reasonable to folks?
[1]
https://lore.kernel.org/git/79901E6C-9839-4AB2-9360-9EBCA1AAE549@icloud.com/
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1627%2Fnewren%2Fprecious-files-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1627/newren/precious-files-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1627
Documentation/technical/precious-files.txt | 540 +++++++++++++++++++++
1 file changed, 540 insertions(+)
create mode 100644 Documentation/technical/precious-files.txt
diff --git a/Documentation/technical/precious-files.txt b/Documentation/technical/precious-files.txt
new file mode 100644
index 00000000000..05c205b57bb
--- /dev/null
+++ b/Documentation/technical/precious-files.txt
@@ -0,0 +1,540 @@
+Precious Files Design Document
+==============================
+
+Table of Contents
+ * Objective
+ * Background
+ * File categorization exceptions
+ * Proposal
+ * Precious file specification
+ * Breakdown of suggested behaviors by command
+ * Backward compatibility notes
+ * Slightly incompatible syntax
+ * Interaction with sparse-checkout parsing
+ * Behavior of traditional flags
+ * Interaction with older Git clients
+ * Commands with modified meaning
+ * Implementation hints
+ * Data structures
+ * Code areas
+ * Minimum
+ * Out of scope
+ * Previous discussions
+ * Alternatives considered
+
+Objective
+---------
+Support "Precious" Files in git, a set of files which are considered
+ignored (e.g. do not show up in "git status" output) but are not expendable
+(thus won't be removed to make room for a file when switching or merging
+branches).
+
+Background
+----------
+In git we have different types of files, with various subdivisions:
+ * tracked
+ * present (i.e. part of sparse checkout)
+ * not present (i.e. not part of sparse checkout)
+ * not tracked
+ * ignored (also treated as expendable)
+ * untracked (more precisely, not-tracked-and-not-ignored, but often
+ referred to as simply "untracked" despite the fact that such a term
+ is easily mistaken as a synonym to "not tracked". However, we haven't
+ been fully consistent, and some places like `git ls-files --others`
+ may use "untracked" to refer to the larger not-tracked category).
+ Not considered expendable.
+
+Over the years, the fact that ignored files are unconditionally treated as
+expendable (so that other operations like git checkout might wipe them out
+to make room for files on the other branch) has occasionally caused
+problems. Many have expressed a desire for subdividing the ignored class,
+so that we have both ignored-and-expendable (possibly referred to as
+"trashable", covering the only type of ignored file we have today) and
+introducing ignored-and-not-expendable (often referred to as "precious").
+
+File categorization exceptions
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Our division above into nice categories is actually a bit of a lie.
+
+Once upon a time untracked files were considered expendable[1]. Even after
+that changed, we still had lots of edge cases where untracked files were
+deleted when they shouldn't be, and ignored files weren't deleted when they
+should be[2]. While that has been (mostly) fixed, despite the general
+intent to preserve untracked files, we have special cases that are
+documented as not preserving them[4,5]. There are also a few codepaths
+that have comments about locations that might (or definitely do)
+erroneously delete untracked paths[6]. And at least one code path that is
+known to erroneously delete untracked paths which has not been commented:
+`git checkout <tree> <pathspec>`. And there may be more.
+
+[1] https://lore.kernel.org/git/CABPp-BFyR19ch71W10oJDFuRX1OHzQ3si971pMn6dPtHKxJDXQ@mail.gmail.com/
+[2] https://lore.kernel.org/git/pull.1036.v3.git.1632760428.gitgitgadget@gmail.com/
+[3] https://lore.kernel.org/git/de416f887d7ce24f20ad3ad4cc838394d6523635.1632760428.git.gitgitgadget@gmail.com/
+[4] https://lore.kernel.org/git/xmqqr1e2ejs9.fsf@gitster.g/
+[5] https://lore.kernel.org/git/de416f887d7ce24f20ad3ad4cc838394d6523635.1632760428.git.gitgitgadget@gmail.com/
+[6] https://lore.kernel.org/git/6b42a80bf3d46e16980d0724e8b07101225239d0.1632760428.git.gitgitgadget@gmail.com/
+
+This history and these exceptions matter to this proposal because:
+ * it highlights how much work can be involved in trying to treat a class
+ of files as not expendable
+ * the existing corner cases where untracked files are erroneously
+ treated as expendable will probably also double as corner cases where
+ precious files are treated as expendable
+ * the past fixes for treating untracked files as precious will likely
+ highlight the needed types of code changes to treat ignored files as
+ precious
+
+Proposal
+--------
+We propose adding another class of files: ignored-but-not-expendable,
+referred to by the shorthand of "precious". The proposal is simple at a
+high level, but there are many details to consider:
+ * How to specify precious files (extended .gitignore syntax? attributes?)
+ * Which commands should be modified, and how?
+ * How to handle flags that are essentially a partial implementation of
+ a precious capability (e.g. [--[no-]overwrite-ignore])?
+ * How will older Git clients behave on a repo with precious files?
+The subsequent sections will try to address these questions in more detail.
+
+One thing to highlight here is that the class formerly called
+`ignored` now has two subtypes: (1) the type we already have,
+ignored-and-expendable (sometimes referred to below as "trashable")
+and (2) the new type, ignored-and-not-expendable (referred to as
+"precious").
+
+Precious file specification
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+As per [P2]:
+
+ """
+ Even though I referred to the precious _attribute_ in some of these
+ discussions, between the attribute mechanism and the ignore
+ mechanism, I am actually leaning toward suggesting to extend the
+ exclude/ignore mechanism to introduce the "precious" class. That
+ way, we can avoid possible snafu arising from marking a path in
+ .gitignore as ignored, and in .gitattrbutes as precious, and have to
+ figure out how these two settings are to work together.
+ """
+
+we specify precious files via an extension to .gitignore. In particular,
+lines starting with a '$' character specify that the file is precious.
+For example:
+ $.config
+would say the file `.config` is precious.
+
+Now that there are three types of files specified by .gitignore files --
+untracked, trashable (ignored-and-expendable), and precious
+(ignored-and-not-expendable), the meaning of `!` at the begining of a line
+needs careful clarification. It could be seen as "not ignored" or as "not
+trashable", given the subdivision of ignored files that has occurred. We
+specifically take it to mean "not ignored", i.e. "untracked".
+
+This leaves us with a simple set of rules to provide to users about lines
+in their '.gitignore' file:
+ * No special prefix character => ignored-and-expendable ("trashable")
+ * A '$' prefix character => ignored-and-not-expendable ("precious")
+ * A '!' prefix character => not ignored, i.e. untracked
+
+It's worth noting that the traditional use of '!' as a negation
+character needs updating, given the introduction of a ternary state
+("not trashable" could mean either untracked or precious, which is
+ambiguous). Refrain from referring to '!' as a negation character to
+avoid confusion. To assist users in making this mindset shift, flag
+any line beginning with '!$' as an error. As always,
+backslash-escaping remains an option, allowing users to specify
+entries like '!\$foo' to mark a file named '$foo' as untracked.
+
+Breakdown of suggested behaviors by command
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+See also "Out of Scope" section below, particularly for:
+ * apply, am [without -3]
+ * checkout/restore
+ * checkout-index
+ * additional information on merge backends
+
+Documentation:
+ * audit for references to "ignore" and "ignored", to see which ones need
+ to now replace those with either "ignored-and-expendable" (or
+ "trashable"), and which can remain "ignored".
+ * audit for "exclude" and "excluded" (the older terminology for ignored
+ files) and update them as well.
+ * add references to "precious" (and perhaps "trashable) as needed (don't
+ forget the glossary)
+ * rm: update the documentation:
+ "Ignored files are deemed expendable and won't stop" ->
+ "Ignored files, unless specifically marked precious, are by default
+ deemed expendable and won't stop"
+ * ensure all codepaths touched by 0e29222e0c2 ("Documentation: call out
+ commands that nuke untracked files/directories", 2021-09-27) also call
+ out that they'll nuke precious files in addition to untracked ones.
+ * change the documentation for '!' in gitignore to stop using the term
+ 'negates'; it's potentially misleading now (negating a ternary value
+ yields an ambiguous value). Instead, the prefix is used to mark
+ untracked (or "not ignored") files.
+ * note that the --[no-]overwrite-ignore option is deprecated, and, since
+ it predated the introduction of precious files is also a misnomer. The
+ correct name of the option would actually be --[no-]overwrite-trashable
+ but it is too late to rename.
+ * consider documenting that merge's --no-overwrite-ignore option is
+ virtually worthless (only works with the fast-forwarding backend).
+ * consider auditing the code for 'untracked' and fixing those to be
+ 'not tracked' in cases where both 'untracked' and 'ignored' files
+ are meant
+
+checkout/switch:
+ * will need to not overwrite precious files when they are in the way of
+ switching branches, unless --force/-f is specified.
+
+checkout/restore:
+ * when passed a <tree> as a source, do not overwrite precious files
+ (NOR untracked files!), unless --force/-f is specified. [Could be
+ considered a stretch goal...]
+
+merge:
+ * do not overwrite precious files when they are in the way of merging
+ branches. (Must be handled in each and every merge strategy;
+ user-defined merge strategies may get this wrong.)
+
+read-tree:
+ * -u: do not overwrite precious files when they are in the way, unless...
+ * --reset and -u: overwrite precious files as well as untracked files.
+ Add to the warning under --reset about overwritten untracked files to
+ note that precious files are also overwritten.
+
+am -3, cherry-pick, rebase, revert, : same as above for checkout/switch and
+ merge.
+
+add:
+ * same as today, just make sure when we split the ignored array (ignored &
+ ignored_nr) into multiple categories that it continues working
+
+rm:
+ * make sure submodules are not removed if precious files are present.
+ Currently, rm will remove submodules if only ignored files are present.
+
+check-ignore:
+ * since this command exists for debugging gitignore rules, there needs to
+ be some kind of mechanism for differentiating between trashable and
+ precious files. It is okay if this comes with a new command-line flag,
+ but there should be some tests showing how it behaves both with and
+ without that flag when precious files are present
+
+clean:
+ * clarify the meaning of -x and -X options: -X now means only remove
+ trashable files. -x means remove both untracked and trashable files.
+ (See also [P17])
+ * add a --all option for removing all not-tracked files: untracked,
+ trashable, and precious.
+ * Other than --all, it is not worth adding flags for cleaning subsets of
+ not-tracked files that include precious files (thus, no flag for just
+ precious, or trashable and precious, or untracked and precious)
+ * Patterns with a leading '$' can be passed to --exclude, if wanted.
+
+ls-files:
+ * --ignored/-i: shows every kind of ignored file (thus behaving the same
+ as today, since there is no way to distinguish between the types of
+ ignored in the output)
+ * add new `--ignored=precious` and `--ignored=trashable` flags for
+ differentiating. A plain `--ignored` is like having both
+ `--ignored=precious` and `--ignored=trashable` specified.
+ * --exclude,--exclude-from can now take patterns with a leading '$' and
+ the file will be considered precious rather than trashable.
+
+status:
+ * --ignored (without additional parameters) continues behaving as-is: it
+ prints both trashable and precious files in its "Ignored" category with
+ no distinguishing.
+ * --ignored --short will continue showing trashable files with '!!', but
+ will now show precious files using '$$'.
+ * --ignored --porcelain={v1,v2} will continue showing precious files
+ with the '!' character, since scripts may not be prepared to parse a
+ leading '$'. We can't break those scripts, even if it'd avoid the
+ off chance that those scripts act on the information about "ignored"
+ files and end up nuking precious files.
+ * --ignored --porcelain=v3 will need to be introduced to show precious
+ files with a leading '$'.
+
+sparse-checkout:
+ * the --rules-file option should be tested with a pattern with a leading
+ '$' to make sure it prints an expected error.
+ * it might be worth noting somewhere that sparse-checkout treats
+ ignored files as precious; when sparsifying, it attempts to remove
+ directories that do not match the sparse specification, but will
+ leave them present if any of the tracked files are modified, or if
+ there are any not-tracked files present. That includes ignored
+ files. That means no additional work is needed for precious
+ support; I just mention it for completeness.
+
+Backward compatibility notes
+----------------------------
+There are multiple issues that impinge on backward compatibility (either in
+terms of special care we need to take, or in terms of messaging we may need
+to send out about changes):
+ * Slightly incompatible syntax
+ * Interaction with sparse-checkout parsing
+ * Behavior of traditional flags
+ * Interaction with older Git clients
+ * Commands with modified meaning
+We'll discuss each in its own subsection below.
+
+Slightly incompatible syntax
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+This new syntax obviously breaks backward compatibility in that an ignored
+path named `$.config` would now have to be specified as `\$.config`. This
+is similar to how introducing `!` as a prefix in .gitignore files was a
+backward compatibility break. We expect and hope that the fallout will be
+minor. See also [P10].
+
+Interaction with sparse-checkout parsing
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The $GIT_DIR/info/sparse-checkout file also makes use of gitignore syntax
+and the gitignore parsing to read the file. It differs in that the files
+specified are considered the files to be included (i.e. present in the
+working copy) rather than which files should be excluded, but otherwise
+has until now used identical syntax and parsing.
+
+However, for sparse-checkout there is no third type of file, so the '$'
+prefix makes no sense for it. As such, it should be an error for any
+lines to begin with '$' in a sparse-checkout file.
+
+(This also means that if anyone really did have a path beginning with '$'
+in sparse-checkout files previously, then they now need to backslash escape
+them, the same as with .gitignore files.)
+
+While we could theoretically avoid this small backward compatibility break
+for sparse-checkout parsing by just treating a leading '$' the way it
+traditionally has been done, I am worried about practically maintaining that
+solution:
+ * the gitignore parsing is peppered with references like 'exclude' that
+ are specific to the gitignore case
+ * because of the above, it is _heavily_ confusing to attempt to read and
+ understand the gitignore handling while considering the sparse-checkout
+ case. I've been tripped up by it *many* times.
+ * I think trying to reuse the existing parsing engine and have it handling
+ both old and new syntax is a recipe for failure. It'd be much cleaner
+ to have errors thrown if the processing turns up any "precious" files,
+ or perhaps if any line starts with '$'.
+ * I think making a copy of the existing parsing, and then letting them
+ diverge, means the two will eventually diverge even further, and we
+ would need to make a copy of all the documentation about gitignore rules
+ for sparse-checkout, all for the non-default non-cone case we are
+ already recommending users away from.
+
+Behavior of traditional flags
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+There are two flags to consider here: the --porcelain flag to git-status,
+and the --no-overwrite-ignore command to checkout & merge commands. For
+the --porcelain flag to git-status, see the "Breakdown of suggested
+behaviors by command" and look for git-status there. The rest of this
+section will focus on --[no-]overwrite-ignore.
+
+People have wanted precious files long enough, that they implemented an
+interim kludge of sorts -- a command line option that can be passed to
+various subcommands that treats all ignored files as precious:
+--no-overwrite-ignore.
+
+In particular, this flag can be passed to both git-checkout, and git-merge.
+However, in merge's case, the support depended the flag being passed to the
+backend and the backend supporting it. The builtin/merge.c code only ever
+bothered to pass this flag down to the fast-forwarding merge handling code,
+so it never worked with any backends that actually create a merge commit.
+
+We do need to keep these flags working, at least as much as they did
+previously. However, we don't want to consider them desired features,
+which would lead us to making related equivalents for precious files like
+--overwrite-precious. Instead we will:
+ * Keep --[no-]overwrite-ignore working, as much as it already was.
+ * Recommend users mark precious files in their gitignore files instead of
+ using these flags
+
+Interaction with older Git clients
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Older Git clients will not understand precious files. This means that:
+ * precious files will be considered untracked and not ignored.
+ * most comands will preserve these files, since untracked-and-not-ignored
+ are not considered expendable.
+ * git status will continue listing these files
+ * git add will add these files without requiring -f.
+
+This seems like a reasonable tradeoff that only has minor annoyances. The
+alternative of having the precious files treated as ignored has the very
+risky trade-off of deleting files which the users marked as important for
+us to keep.
+
+Commands with modified meaning
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+In clean, we adjust the meaning of both -x and -X:
+ -X: remove only trashable files
+ -x: remove untracked and trashable files (but preserve precious ones)
+
+Implementation hints
+--------------------
+
+Data structures
+~~~~~~~~~~~~~~~
+ * We will want to add a `precious` and `precious_nr` in dir_struct,
+ similar to the current entries/nr or ignored/ignored_nr.
+ * We may want to rename `ignored` and `ignored_nr` in dir_struct to
+ `trashable` and `trashable_nr`.
+
+Code areas
+~~~~~~~~~~
+ * "preserve_ignored", a flag in the code for handling the
+ --[no-]overwrite-ignore flag, is a very helpful marker about what needs
+ to be tweaked and how to tweak it to preserve more files. In particular,
+ note that --no-overwrite-ignore works by telling the machinery in dir.c
+ to not do the setup_standard_excludes() stuff, so that all ignored files
+ just look like untracked files. We'll need something slightly smarter,
+ which makes precious files look like untracked while trashable files
+ still appear in ignored. Shouldn't be too bad.
+ * we might need to add another entry to the unpack_trees_reset_type
+ enum. Or perhaps we still keep both UNPACK_RESET_PROTECT_UNTRACKED
+ and UNPACK_RESET_OVERWRITE_UNTRACKED but rename them with
+ s/UNTRACKED/NOT_EXPENDABLE/ so it is clear they handle both untracked and
+ precious files. Not sure which is needed yet.
+ * dir_struct->flags _might_ need new entries.
+ * ensure all relevant codepaths touched by 94b7f1563ac ("Comment important
+ codepaths regarding nuking untracked files/dirs", 2021-09-27) are either
+ fixed or also mention precious files
+ * am/rebase/checkout[without -f]: see 480d3d6bf90 ("Change unpack_trees'
+ 'reset' flag into an enum", 2021-09-27)
+ * Merge backends:
+ * (see also "Out of scope" section)
+ * merge-ort can be fixed by fixing the checkout code.
+ * merge-resolve and merge-octopus can probably be fixed by fixing
+ git-reset.
+ * stash:
+ * there is an existing --include-untracked option. There was no reason
+ to add a --include-ignored, because ignored files were trashable. Do
+ we need to add a --include-precious, though?
+ * this is a sad pile of shell-reimplemented-in-C. It's just awful.
+ See b34ab4a43ba ("stash: remove unnecessary process forking",
+ 2020-12-01) and ba359fd5070 ("stash: fix stash application in
+ sparse-checkouts", 2020-12-01) and 94b7f1563ac ("Comment important
+ codepaths regarding nuking untracked files/dirs", 2021-09-27).
+ Fixing stash to not nuke precious files (and to not nuke untracked
+ files either) might mean expunging the stupid
+ shell-reimplemented-in-C design, or at least moving things more in
+ that direction.
+ * rebase (merge backend), revert, cherry-pick, am -3: should automatically
+ be handled by getting merge-ort to work, which should work by making
+ checkout/switch work.
+ * bisect: should work by making checkout work
+
+Minimum
+~~~~~~~
+
+I think for a minimum implementation, we need to ensure that the following
+are handled:
+ * parsing:
+ * parsing of lines starting with '$' in .gitignore
+ * erroring on lines starting with '!$' in .gitignore
+ * erroring on lines starting with '$' in $GIT_DIR/info/sparse-checkout
+ * commands with support:
+ * switch/checkout
+ * merge when using the ort backend
+ * read-tree -u [without --reset] (due to internal use)
+ * ls-files
+
+Out of scope
+------------
+The following tasks are currently out of scope for this proposal:
+
+apply, am [without -3]: apply won't overwrite any file in the working
+ directory even when a new file is in the patch. It should overwrite
+ trashable files. We could log that bug via testcase, but make sure
+ there's a companion testcase that ensures overwriting untracked or
+ precious files continues to make apply throw an error. However, since
+ apply/am don't misbehave for precious files, we can defer this to later.
+
+checkout-index: similar to apply; won't overwrite any existing files, but
+ trashable files should be overwritten
+
+reset --hard:
+ * `git reset --hard` is a little funny and we have thought about changing
+ it[4]. However, that can be left for later and will not be tackled as
+ part of the work of introducing "precious" files as a concept.
+
+merge backends:
+ * it may make sense to try to make --no-overwrite-ignore work with more
+ merge backends, both because it's technically documented behavior, and
+ because doing so may be a step towards getting precious files supported
+ * when multiple merge strategies are specified, builtin/merge.c will
+ stash and restore state between the attempt of different strategies.
+ Since the reset_hard() function invokes `read-tree --reset -u`, there
+ might be a way to cause it to trash untracked files or to trash
+ precious files, depending on what the merge strategies did. It seems
+ unlikely (maybe the strategy handles D/F conflicts or rename
+ conflicts by renaming files in the way, and happens to rename a
+ precious file to a path that is considered either untracked or
+ precious -- merge-recursive certainly did this something like this
+ once upon a time and still might); we can probably ignore it for now.
+ * merge-recursive is a lost cause; it'd be a _huge_ amount of effort to
+ fix, but we intend to deprecate and delete it soon anyway (making all
+ requests for recursive just trigger ort instead).
+ * user-defined merge strategies are up to their authors to get right.
+ Odds are they won't, but odds are they already incorrectly nuke
+ untracked files too because who'd pay attention to a special case
+ like files being in the way of a merge? Anyway, "not our problem". :-)
+
+Previous discussions
+--------------------
+
+A far from exhaustive sampling of various past conversations on the topic:
+
+[P1] https://lore.kernel.org/git/7vipsnar23.fsf@alter.siamese.dyndns.org/
+[P2] https://lore.kernel.org/git/xmqqttqytnqb.fsf@gitster.g/
+[P3] https://lore.kernel.org/git/79901E6C-9839-4AB2-9360-9EBCA1AAE549@icloud.com/
+[P4] https://lore.kernel.org/git/87a6q9kacx.fsf@evledraar.gmail.com/
+[P5] https://lore.kernel.org/git/20190216114938.18843-1-pclouds@gmail.com/
+[P6] https://lore.kernel.org/git/87ftsi68ke.fsf@evledraar.gmail.com/
+[P7] https://lore.kernel.org/git/xmqqo7ub4sfh.fsf@gitster.g/
+[P8] https://lore.kernel.org/git/7v4oepaup7.fsf@alter.siamese.dyndns.org/
+[P9] https://lore.kernel.org/git/20181112232209.GK890086@genre.crustytoothpaste.net/
+[P10] https://lore.kernel.org/git/xmqqttqvg4lw.fsf@gitster.g/
+[P11] https://lore.kernel.org/git/xmqqk1hrr91s.fsf@gitster-ct.c.googlers.com/
+[P12] https://lore.kernel.org/git/9C4A2AFD-AAA2-4ABA-8A8B-2133FD870366@icloud.com/
+[P13] https://lore.kernel.org/git/xmqqfs2e3292.fsf@gitster.g/
+[P14] https://lore.kernel.org/git/0deee2bc-1775-4459-906d-1d44b3103499@gmail.com/
+[P15] https://lore.kernel.org/git/ZSkpOc%2FdcGcrFQNU@ugly/
+[P16] https://lore.kernel.org/git/xmqqil79t82q.fsf@gitster.g/
+[P17] https://lore.kernel.org/git/xmqqo7h6tnib.fsf@gitster.g/
+
+Alternatives considered
+-----------------------
+There have been multiple alternatives considered, along a few different
+axes:
+ * .gitattributes instead of .gitignore
+ * leaving sparse-checkout alone
+ * Trashable [P9,P11]
+ * Alternative gitignore syntax
+
+The choice of .gitattributes vs .gitignore was already addressed in the
+"Precious file specification" section.
+
+The choice to modify or leave alone the parsing of
+$GIT_DIR/info/sparse-checkout was already addressed in the "Interaction
+with sparse-checkout parsing" section.
+
+One alternative raised in the past was treating ignored files as not
+expendable by default, and then introducing a new category of
+ignored-but-expendable. This new category has been dubbed "trashable" in
+the past. That may have been a reasonable solution if Git did not have a
+large userbase already, but moving in this direction would cause severe
+problems for existing builds everywhere[P9] and would require users to
+doubly configure most files (since it is expected that
+ignored-but-expendable is a much larger class of files than
+ignored-but-precious). See also [P11].
+
+There have been multiple alternative suggestions for extending gitignore
+syntax to handle precious files and optionally future extensions as well.
+For example: [P10, P12, P13, P14, P15, P16] However:
+ * There have been on and off requests for precious files for about 14
+ years
+ * We are not aware of other types of extensions needed; there might
+ not be any
+ * The alternatives all seem much more complex to explain to users than
+ the simple proposal here.
+In particular, we like the simplicity of the providing the simple mapping
+to users from the penultimate paragraph of the "Precious file
+specification" section (the one regarding no-prefix vs. '!' vs '$').
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2] diff: implement config.diff.renames=copies-harder
From: Elijah Newren @ 2023-12-27 2:24 UTC (permalink / raw)
To: Sam James; +Cc: git, Junio C Hamano
In-Reply-To: <20231226202102.3392518-1-sam@gentoo.org>
Hi,
It helps when providing a new iteration of a patch to do two things:
(1) provide a range-diff against the previous version to make it
easier for reviewers to see what has changed
(2) either reply to the previous submission or link to it in your
header so reviewers can find previous comments
In this case, v1 was over at
https://lore.kernel.org/git/pull.1606.git.1699010701704.gitgitgadget@gmail.com/.
On Tue, Dec 26, 2023 at 12:21 PM Sam James <sam@gentoo.org> wrote:
>
> This patch adds a config value for 'diff.renames' called 'copies-harder'
> which make it so '-C -C' is in effect always passed for 'git log -p',
> 'git diff', etc.
>
> This allows specifying that 'git log -p', 'git diff', etc should always act
> as if '-C --find-copies-harder' was passed.
>
> It has proven this especially useful for certain types of repository (like
> Gentoo's ebuild repositories) because files are often copies of a previous
> version:
>
> Suppose a directory 'sys-devel/gcc' contains recipes for building
> GCC, with one file for each supported upstream branch:
> gcc-13.x.build.recipe
> gcc-12.x.build.recipe
> gcc-11.x.build.recipe
> gcc-10.x.build.recipe
>
> gcc-13.x.build.recipe was started as a copy of gcc-12.x.build.recipe
> (which was started as a copy of gcc-11.x.build.recipe, etc.). Previous versions
> are kept around to support parallel installation of multiple versions.
>
> Being able to easily observe the diff relative to other recipes within the
> directory has been a quality of life improvement for such repo layouts.
>
> Cc: Elijah Newren <newren@gmail.com>
> Cc: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Sam James <sam@gentoo.org>
While the "Cc:" lines don't hurt, note that everything above this
point is included in the commit message, so we'd typically rather
those two lines be below the triple-dashed line.
> ---
> v2: Improve the commit message using Elijah Newren's example (it is indeed
> precisely what I was thinking of, but phrased better), and improve the documentation
> to explain better what the new config option actually does & refer to git-diff's
> --find-copies-harder.
>
> Documentation/config/diff.txt | 8 +++++---
> Documentation/config/status.txt | 3 ++-
> diff.c | 12 +++++++++---
> diff.h | 1 +
> diffcore-rename.c | 4 ++--
> merge-ort.c | 2 +-
> merge-recursive.c | 2 +-
> 7 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
> index bd5ae0c337..cdd8a74ec0 100644
> --- a/Documentation/config/diff.txt
> +++ b/Documentation/config/diff.txt
> @@ -131,9 +131,11 @@ diff.renames::
> Whether and how Git detects renames. If set to "false",
> rename detection is disabled. If set to "true", basic rename
> detection is enabled. If set to "copies" or "copy", Git will
> - detect copies, as well. Defaults to true. Note that this
> - affects only 'git diff' Porcelain like linkgit:git-diff[1] and
> - linkgit:git-log[1], and not lower level commands such as
> + detect copies, as well. If set to "copies-harder", Git will spend extra
> + cycles to find more copies even in unmodified paths, see
> + '--find-copies-harder' in linkgit:git-diff[1]. Defaults to true.
> + Note that this affects only 'git diff' Porcelain like linkgit:git-diff[1]
> + and linkgit:git-log[1], and not lower level commands such as
> linkgit:git-diff-files[1].
>
> diff.suppressBlankEmpty::
> diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
> index 2ff8237f8f..7ca7a4becd 100644
> --- a/Documentation/config/status.txt
> +++ b/Documentation/config/status.txt
> @@ -33,7 +33,8 @@ status.renames::
> Whether and how Git detects renames in linkgit:git-status[1] and
> linkgit:git-commit[1] . If set to "false", rename detection is
> disabled. If set to "true", basic rename detection is enabled.
> - If set to "copies" or "copy", Git will detect copies, as well.
> + If set to "copies" or "copy", Git will detect copies, as well. If
> + set to "copies-harder", Git will try harder to detect copies.
It'd be nice to have the improved text from diff.renames here ("If set
to "copies-harder", Git will spend extra cycles to find more copies
even in unmodified paths.").
> Defaults to the value of diff.renames.
>
> status.showStash::
> diff --git a/diff.c b/diff.c
> index a2def45644..585acf9a99 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -206,8 +206,11 @@ int git_config_rename(const char *var, const char *value)
> {
> if (!value)
> return DIFF_DETECT_RENAME;
> + if (!strcasecmp(value, "copies-harder"))
> + return DIFF_DETECT_COPY_HARDER;
> if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
> - return DIFF_DETECT_COPY;
> + return DIFF_DETECT_COPY;
> +
I pointed out in response to v1 that these last couple lines, while a
nice cleanup, should be in a separate patch.
> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
> }
>
> @@ -4849,8 +4852,11 @@ void diff_setup_done(struct diff_options *options)
> else
> options->flags.diff_from_contents = 0;
>
> - if (options->flags.find_copies_harder)
> + /* Just fold this in as it makes the patch-to-git smaller */
> + if (options->flags.find_copies_harder || options->detect_rename == DIFF_DETECT_COPY_HARDER) {
Again, I already responded to v1 that four of your lines were too long
and needed to be split. All four remain unsplit in your resubmission.
For future reference, when you resubmit a patch, please either (a)
first respond to the review explaining why you don't agree with the
suggested changes, or (b) include the fixes reviewers point out, or
(c) state in your cover letter or explanation after the '---' why you
chose to not include the suggested changes.
> + options->flags.find_copies_harder = 1;
> options->detect_rename = DIFF_DETECT_COPY;
> + }
>
> if (!options->flags.relative_name)
> options->prefix = NULL;
> @@ -5281,7 +5287,7 @@ static int diff_opt_find_copies(const struct option *opt,
> if (*arg != 0)
> return error(_("invalid argument to %s"), opt->long_name);
>
> - if (options->detect_rename == DIFF_DETECT_COPY)
> + if (options->detect_rename == DIFF_DETECT_COPY || options->detect_rename == DIFF_DETECT_COPY_HARDER)
> options->flags.find_copies_harder = 1;
> else
> options->detect_rename = DIFF_DETECT_COPY;
> diff --git a/diff.h b/diff.h
> index 66bd8aeb29..b29e5b777f 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -555,6 +555,7 @@ int git_config_rename(const char *var, const char *value);
>
> #define DIFF_DETECT_RENAME 1
> #define DIFF_DETECT_COPY 2
> +#define DIFF_DETECT_COPY_HARDER 3
>
> #define DIFF_PICKAXE_ALL 1
> #define DIFF_PICKAXE_REGEX 2
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 5a6e2bcac7..856291d66f 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -299,7 +299,7 @@ static int find_identical_files(struct hashmap *srcs,
> }
> /* Give higher scores to sources that haven't been used already */
> score = !source->rename_used;
> - if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY)
> + if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY && options->detect_rename != DIFF_DETECT_COPY_HARDER)
> continue;
> score += basename_same(source, target);
> if (score > best_score) {
> @@ -1405,7 +1405,7 @@ void diffcore_rename_extended(struct diff_options *options,
> trace2_region_enter("diff", "setup", options->repo);
> info.setup = 0;
> assert(!dir_rename_count || strmap_empty(dir_rename_count));
> - want_copies = (detect_rename == DIFF_DETECT_COPY);
> + want_copies = (detect_rename == DIFF_DETECT_COPY || detect_rename == DIFF_DETECT_COPY_HARDER);
> if (dirs_removed && (break_idx || want_copies))
> BUG("dirs_removed incompatible with break/copy detection");
> if (break_idx && relevant_sources)
> diff --git a/merge-ort.c b/merge-ort.c
> index 6491070d96..7749835465 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4782,7 +4782,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> * sanity check them anyway.
> */
> assert(opt->detect_renames >= -1 &&
> - opt->detect_renames <= DIFF_DETECT_COPY);
> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
> assert(opt->verbosity >= 0 && opt->verbosity <= 5);
> assert(opt->buffer_output <= 2);
> assert(opt->obuf.len == 0);
> diff --git a/merge-recursive.c b/merge-recursive.c
> index e3beb0801b..d52dd53660 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3708,7 +3708,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
> assert(opt->branch1 && opt->branch2);
>
> assert(opt->detect_renames >= -1 &&
> - opt->detect_renames <= DIFF_DETECT_COPY);
> + opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
> assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
> opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
> assert(opt->rename_limit >= -1);
> --
> 2.43.0
The patch looks close, but it does continue to violate style
guidelines and make unrelated changes, like v1. And the wording in
one of the documentation blurbs could be improved a little. Looking
forward to a v3 with those fixed.
^ permalink raw reply
* subscribe to git mailing list
From: Kirandeep Paul @ 2023-12-27 1:17 UTC (permalink / raw)
To: git
subscribe git
^ permalink raw reply
* [PATCH v4 3/3] apply: code simplification
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git
In-Reply-To: <20231226233218.472054-1-gitster@pobox.com>
Rewrite a bit hard-to-read ternary ?: expression into a cascade of
if/else.
Given that read-cache.c:add_index_entry() makes sure that the
.ce_mode member is filled with a reasonable value before placing a
cache entry in the index, if we see (ce_mode == 0), there is
something seriously wrong going on. Catch such a bug and abort,
instead of silently ignoring such an entry and silently skipping
the check.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/apply.c b/apply.c
index 6b1adccb2f..493a263a48 100644
--- a/apply.c
+++ b/apply.c
@@ -3780,11 +3780,15 @@ static int check_preimage(struct apply_state *state,
}
if (!state->cached && !previous) {
- if (!trust_executable_bit)
- st_mode = (*ce && (*ce)->ce_mode)
- ? (*ce)->ce_mode : patch->old_mode;
- else
+ if (*ce && !(*ce)->ce_mode)
+ BUG("ce_mode == 0 for path '%s'", old_name);
+
+ if (trust_executable_bit)
st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ else if (*ce)
+ st_mode = (*ce)->ce_mode;
+ else
+ st_mode = patch->old_mode;
}
if (patch->is_new < 0)
--
2.43.0-174-g055bb6e996
^ permalink raw reply related
* [PATCH v4 2/3] apply: correctly reverse patch's pre- and post-image mode bits
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <20231226233218.472054-1-gitster@pobox.com>
When parsing the patch header, unless it is a patch that changes
file modes, we only read the mode bits into the .old_mode member of
the patch structure and leave .new_mode member as initialized, i.e.,
to 0. Later when we need the original mode bits, we consult .old_mode.
However, reverse_patches() that is used to swap the names and modes
of the preimage and postimage files is not aware of this convention,
leading the .old_mode to be 0 while the mode we read from the patch
is left in .new_mode.
Only swap .old_mode and .new_mode when .new_mode is not 0 (i.e. we
saw a patch that modifies the filemode and know what the new mode
is). When .new_mode is set to 0, it means the preimage and the
postimage files have the same mode (which is in the .old_mode member)
and when applying such a patch in reverse, the value in .old_mode is
what we expect the (reverse-) preimage file to have.
Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/apply.c b/apply.c
index 3b090652cf..6b1adccb2f 100644
--- a/apply.c
+++ b/apply.c
@@ -2220,7 +2220,8 @@ static void reverse_patches(struct patch *p)
struct fragment *frag = p->fragments;
SWAP(p->new_name, p->old_name);
- SWAP(p->new_mode, p->old_mode);
+ if (p->new_mode)
+ SWAP(p->new_mode, p->old_mode);
SWAP(p->is_new, p->is_delete);
SWAP(p->lines_added, p->lines_deleted);
SWAP(p->old_oid_prefix, p->new_oid_prefix);
@@ -3780,9 +3781,8 @@ static int check_preimage(struct apply_state *state,
if (!state->cached && !previous) {
if (!trust_executable_bit)
- st_mode = (*ce && (*ce)->ce_mode) ? (*ce)->ce_mode :
- (state->apply_in_reverse
- ? patch->new_mode : patch->old_mode);
+ st_mode = (*ce && (*ce)->ce_mode)
+ ? (*ce)->ce_mode : patch->old_mode;
else
st_mode = ce_mode_from_stat(*ce, st->st_mode);
}
--
2.43.0-174-g055bb6e996
^ permalink raw reply related
* [PATCH v4 1/3] apply: ignore working tree filemode when !core.filemode
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Johannes Schindelin
In-Reply-To: <20231226233218.472054-1-gitster@pobox.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
When applying a patch that adds an executable file, git apply
ignores the core.fileMode setting (core.fileMode in git config
specifies whether the executable bit on files in the working tree
should be honored or not) resulting in warnings like:
warning: script.sh has type 100644, expected 100755
even when core.fileMode is set to false, which is undesired. This
is extra true for systems like Windows.
Fix this by inferring the correct file mode from either the existing
index entry, and when it is unavailable, assuming that the file mode
was OK by pretending it had the mode that the preimage wants to see,
when core.filemode is set to false. Add a test case that verifies
the change and prevents future regression.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
apply.c | 10 ++++++++--
t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/apply.c b/apply.c
index 3d69fec836..3b090652cf 100644
--- a/apply.c
+++ b/apply.c
@@ -3778,8 +3778,14 @@ static int check_preimage(struct apply_state *state,
return error_errno("%s", old_name);
}
- if (!state->cached && !previous)
- st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ if (!state->cached && !previous) {
+ if (!trust_executable_bit)
+ st_mode = (*ce && (*ce)->ce_mode) ? (*ce)->ce_mode :
+ (state->apply_in_reverse
+ ? patch->new_mode : patch->old_mode);
+ else
+ st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ }
if (patch->is_new < 0)
patch->is_new = 0;
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index e7a7295f1b..e7026507dc 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -101,4 +101,31 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
)
'
+test_expect_success 'git apply respects core.fileMode' '
+ test_config core.fileMode false &&
+ echo true >script.sh &&
+ git add --chmod=+x script.sh &&
+ git ls-files -s script.sh >ls-files-output &&
+ test_grep "^100755" ls-files-output &&
+ test_tick && git commit -m "Add script" &&
+ git ls-tree -r HEAD script.sh >ls-tree-output &&
+ test_grep "^100755" ls-tree-output &&
+
+ echo true >>script.sh &&
+ test_tick && git commit -m "Modify script" script.sh &&
+ git format-patch -1 --stdout >patch &&
+ test_grep "^index.*100755$" patch &&
+
+ git switch -c branch HEAD^ &&
+ git apply --index patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err &&
+ git reset --hard &&
+
+ git apply patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err &&
+
+ git apply --cached patch 2>err &&
+ test_grep ! "has type 100644, expected 100755" err
+'
+
test_done
--
2.43.0-174-g055bb6e996
^ permalink raw reply related
* [PATCH v4 0/3] apply with core.filemode=false
From: Junio C Hamano @ 2023-12-26 23:32 UTC (permalink / raw)
To: git
In-Reply-To: <pull.1620.v3.git.1703066893657.gitgitgadget@gmail.com>
Chandra Pratap noticed that "git apply" on a filesystem without
executable bit support gives a warning when applying a patch that
expects the preimage file to have executable bit on. Dscho noticed
that the initial fix by Chandra did not work well when applying a
patch in reverse. It turns out that apply.c:reverse_patches()
invalidates the "a patch that does not change mode bits have the
mode bits in .old_mode member and not in .new_mode member" invariant
we rely on.
Here is the result of concerted effort.
Chandra Pratap (1):
apply: ignore working tree filemode when !core.filemode
Junio C Hamano (2):
apply: correctly reverse patch's pre- and post-image mode bits
apply: code simplification
apply.c | 16 +++++++++++++---
t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
2 files changed, 40 insertions(+), 3 deletions(-)
--
2.43.0-174-g055bb6e996
^ permalink raw reply
* Accessing Git
From: Marc Casanova @ 2023-12-26 22:26 UTC (permalink / raw)
To: git
I have a Git server installed on an Ubuntu container in a Proxmox
hypervisor. That part was easy.
I want to access that server from my Windows computers on the same local
network. How do I accomplish that?
^ permalink raw reply
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-26 21:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
In-Reply-To: <xmqqv88kfzpr.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be
>>> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,
>>> and we must use `new_mode` instead.
>>
>> Good finding.
>
> Hmph, this is puzzling and I spoke way before I understand why/how
> the fixup patch works X-<.
>
> The caller of check_preimage(), check_patch(), should happen only
> after reverse_patches() swapped old and new names and modes in
> apply_patch(), so at this point, we should be able to consistently
> use old_mode for checking, shouldn't we, even with "-R"?
I think I figured it out. When we parse an incoming patch, unless
the patch is about changing the mode, we only fill old_mode (e.g.,
follow the callflow from gitdiff_index() -> gitdiff_oldmode() ->
parse_mode_line() to see how this is done). In check_patch(), which
is the caller of check_preimage() in question, there are lines that
propagate old_mode to new_mode (because unfilled new_mode can come
only because there was no mode change), but that happens much later
after check_preimage() was called and returned.
I also suspect that this copying from old to new should be done much
earlier, after parse_chunk() finds out the (old)mode by calling
find_header(), and by doing so, you wouldn't have been hit by the
"-R makes old_mode 0" because both sides would be correctly filled
before we call reverse_patches() gets called. But I haven't traced
all existing codepaths to see if such a change has unintended
consequences (e.g., somebody may incorrectly assuming that "old_mode
== new_mode == 100644" and "old_mode == 100644, new_mode == 0" mean
different things and acting differently).
In any case, I suspect that a better check is not about "are we
applying in reverse?", but more like the attached patch, i.e. if old
side is not filled, then we should pick up the other side.
apply.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git c/apply.c w/apply.c
index 697ab2eb1f..5896056a69 100644
--- c/apply.c
+++ w/apply.c
@@ -3779,12 +3779,18 @@ static int check_preimage(struct apply_state *state,
}
if (!state->cached && !previous) {
- if (!trust_executable_bit)
- st_mode = *ce ? (*ce)->ce_mode :
- (state->apply_in_reverse
- ? patch->new_mode : patch->old_mode);
- else
+ if (!trust_executable_bit) {
+ if (*ce && !(*ce)->ce_mode)
+ BUG("ce_mode == 0 for path '%s'", old_name);
+ if (*ce)
+ st_mode = (*ce)->ce_mode;
+ else if (!patch->old_mode)
+ st_mode = patch->new_mode;
+ else
+ st_mode = patch->old_mode;
+ } else {
st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ }
}
if (patch->is_new < 0)
^ permalink raw reply related
* Re: [PATCH v3] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-26 20:58 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Chandra Pratap via GitGitGadget, git, Torsten Bögershausen,
Chandra Pratap, Chandra Pratap
In-Reply-To: <xmqq4jg4j28z.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> As pointed out e.g. by t2016.3(git checkout -p), if the patch is to be
>> applied in reverse (`git apply -R`), then the `old_mode` is actually 0,
>> and we must use `new_mode` instead.
>
> Good finding.
Hmph, this is puzzling and I spoke way before I understand why/how
the fixup patch works X-<.
The caller of check_preimage(), check_patch(), should happen only
after reverse_patches() swapped old and new names and modes in
apply_patch(), so at this point, we should be able to consistently
use old_mode for checking, shouldn't we, even with "-R"?
^ permalink raw reply
* Re: [PATCH 00/12] Introduce `refStorage` extension
From: Junio C Hamano @ 2023-12-26 20:56 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, git
In-Reply-To: <CAOLa=ZR0mM_rVBa3LG-etpgYGCpiXinCZ83hcWLVeZVUhKH3UA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Hello,
>
> Patrick Steinhardt <ps@pks.im> writes:
>> Hi,
>>
>> this patch series introduces a new `refStorage` extension and related
>> tooling. While there is only a single user-visible ref backend for now,
>> this extension will eventually allow us to determine which backend shall
>> be used for a repository. Furthermore, the series introduces tooling so
>> that the ref storage format becomes more accessible.
>
> Apart from small nits, the series looks good to me, thanks!
Thanks. Hopefully we can merge it down after a quick, small, and
final reroll?
^ permalink raw reply
* [PATCH v2] diff: implement config.diff.renames=copies-harder
From: Sam James @ 2023-12-26 20:20 UTC (permalink / raw)
To: git; +Cc: Sam James, Elijah Newren, Junio C Hamano
This patch adds a config value for 'diff.renames' called 'copies-harder'
which make it so '-C -C' is in effect always passed for 'git log -p',
'git diff', etc.
This allows specifying that 'git log -p', 'git diff', etc should always act
as if '-C --find-copies-harder' was passed.
It has proven this especially useful for certain types of repository (like
Gentoo's ebuild repositories) because files are often copies of a previous
version:
Suppose a directory 'sys-devel/gcc' contains recipes for building
GCC, with one file for each supported upstream branch:
gcc-13.x.build.recipe
gcc-12.x.build.recipe
gcc-11.x.build.recipe
gcc-10.x.build.recipe
gcc-13.x.build.recipe was started as a copy of gcc-12.x.build.recipe
(which was started as a copy of gcc-11.x.build.recipe, etc.). Previous versions
are kept around to support parallel installation of multiple versions.
Being able to easily observe the diff relative to other recipes within the
directory has been a quality of life improvement for such repo layouts.
Cc: Elijah Newren <newren@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Sam James <sam@gentoo.org>
---
v2: Improve the commit message using Elijah Newren's example (it is indeed
precisely what I was thinking of, but phrased better), and improve the documentation
to explain better what the new config option actually does & refer to git-diff's
--find-copies-harder.
Documentation/config/diff.txt | 8 +++++---
Documentation/config/status.txt | 3 ++-
diff.c | 12 +++++++++---
diff.h | 1 +
diffcore-rename.c | 4 ++--
merge-ort.c | 2 +-
merge-recursive.c | 2 +-
7 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
index bd5ae0c337..cdd8a74ec0 100644
--- a/Documentation/config/diff.txt
+++ b/Documentation/config/diff.txt
@@ -131,9 +131,11 @@ diff.renames::
Whether and how Git detects renames. If set to "false",
rename detection is disabled. If set to "true", basic rename
detection is enabled. If set to "copies" or "copy", Git will
- detect copies, as well. Defaults to true. Note that this
- affects only 'git diff' Porcelain like linkgit:git-diff[1] and
- linkgit:git-log[1], and not lower level commands such as
+ detect copies, as well. If set to "copies-harder", Git will spend extra
+ cycles to find more copies even in unmodified paths, see
+ '--find-copies-harder' in linkgit:git-diff[1]. Defaults to true.
+ Note that this affects only 'git diff' Porcelain like linkgit:git-diff[1]
+ and linkgit:git-log[1], and not lower level commands such as
linkgit:git-diff-files[1].
diff.suppressBlankEmpty::
diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
index 2ff8237f8f..7ca7a4becd 100644
--- a/Documentation/config/status.txt
+++ b/Documentation/config/status.txt
@@ -33,7 +33,8 @@ status.renames::
Whether and how Git detects renames in linkgit:git-status[1] and
linkgit:git-commit[1] . If set to "false", rename detection is
disabled. If set to "true", basic rename detection is enabled.
- If set to "copies" or "copy", Git will detect copies, as well.
+ If set to "copies" or "copy", Git will detect copies, as well. If
+ set to "copies-harder", Git will try harder to detect copies.
Defaults to the value of diff.renames.
status.showStash::
diff --git a/diff.c b/diff.c
index a2def45644..585acf9a99 100644
--- a/diff.c
+++ b/diff.c
@@ -206,8 +206,11 @@ int git_config_rename(const char *var, const char *value)
{
if (!value)
return DIFF_DETECT_RENAME;
+ if (!strcasecmp(value, "copies-harder"))
+ return DIFF_DETECT_COPY_HARDER;
if (!strcasecmp(value, "copies") || !strcasecmp(value, "copy"))
- return DIFF_DETECT_COPY;
+ return DIFF_DETECT_COPY;
+
return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
}
@@ -4849,8 +4852,11 @@ void diff_setup_done(struct diff_options *options)
else
options->flags.diff_from_contents = 0;
- if (options->flags.find_copies_harder)
+ /* Just fold this in as it makes the patch-to-git smaller */
+ if (options->flags.find_copies_harder || options->detect_rename == DIFF_DETECT_COPY_HARDER) {
+ options->flags.find_copies_harder = 1;
options->detect_rename = DIFF_DETECT_COPY;
+ }
if (!options->flags.relative_name)
options->prefix = NULL;
@@ -5281,7 +5287,7 @@ static int diff_opt_find_copies(const struct option *opt,
if (*arg != 0)
return error(_("invalid argument to %s"), opt->long_name);
- if (options->detect_rename == DIFF_DETECT_COPY)
+ if (options->detect_rename == DIFF_DETECT_COPY || options->detect_rename == DIFF_DETECT_COPY_HARDER)
options->flags.find_copies_harder = 1;
else
options->detect_rename = DIFF_DETECT_COPY;
diff --git a/diff.h b/diff.h
index 66bd8aeb29..b29e5b777f 100644
--- a/diff.h
+++ b/diff.h
@@ -555,6 +555,7 @@ int git_config_rename(const char *var, const char *value);
#define DIFF_DETECT_RENAME 1
#define DIFF_DETECT_COPY 2
+#define DIFF_DETECT_COPY_HARDER 3
#define DIFF_PICKAXE_ALL 1
#define DIFF_PICKAXE_REGEX 2
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 5a6e2bcac7..856291d66f 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -299,7 +299,7 @@ static int find_identical_files(struct hashmap *srcs,
}
/* Give higher scores to sources that haven't been used already */
score = !source->rename_used;
- if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY)
+ if (source->rename_used && options->detect_rename != DIFF_DETECT_COPY && options->detect_rename != DIFF_DETECT_COPY_HARDER)
continue;
score += basename_same(source, target);
if (score > best_score) {
@@ -1405,7 +1405,7 @@ void diffcore_rename_extended(struct diff_options *options,
trace2_region_enter("diff", "setup", options->repo);
info.setup = 0;
assert(!dir_rename_count || strmap_empty(dir_rename_count));
- want_copies = (detect_rename == DIFF_DETECT_COPY);
+ want_copies = (detect_rename == DIFF_DETECT_COPY || detect_rename == DIFF_DETECT_COPY_HARDER);
if (dirs_removed && (break_idx || want_copies))
BUG("dirs_removed incompatible with break/copy detection");
if (break_idx && relevant_sources)
diff --git a/merge-ort.c b/merge-ort.c
index 6491070d96..7749835465 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4782,7 +4782,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
* sanity check them anyway.
*/
assert(opt->detect_renames >= -1 &&
- opt->detect_renames <= DIFF_DETECT_COPY);
+ opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
assert(opt->verbosity >= 0 && opt->verbosity <= 5);
assert(opt->buffer_output <= 2);
assert(opt->obuf.len == 0);
diff --git a/merge-recursive.c b/merge-recursive.c
index e3beb0801b..d52dd53660 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3708,7 +3708,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
assert(opt->branch1 && opt->branch2);
assert(opt->detect_renames >= -1 &&
- opt->detect_renames <= DIFF_DETECT_COPY);
+ opt->detect_renames <= DIFF_DETECT_COPY_HARDER);
assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
assert(opt->rename_limit >= -1);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2] sparse-checkout: be consistent with end of options markers
From: Junio C Hamano @ 2023-12-26 20:12 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget
Cc: git, Randall S. Becker, Jeff King, Elijah Newren
In-Reply-To: <pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Remove the erroneous PARSE_OPT_KEEP_UNKNOWN_OPT flag now to fix this
> bug. Note that this does mean that anyone who might have been using
>
> git sparse-checkout [add|set] [--[no-]cone] --foo --bar
>
> to request paths or patterns '--foo' and '--bar' will now have to use
>
> git sparse-checkout [add|set] [--[no-]cone] -- --foo --bar
>
> That makes sparse-checkout more consistent with other git commands,
> provides users much friendlier error messages and behavior, and is
> consistent with the all-caps warning in git-sparse-checkout.txt that
> this command "is experimental...its behavior...will likely change". :-)
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
Let me drop jc/sparse-checkout-eoo topic and queue this instead, and
then resurrect the "use default for 'set' only when !stdin" as a
separate topic.
Thanks.
^ 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