* [PATCH v5 2/2] t7501: add tests for --amend --signoff
From: Ghanshyam Thakkar @ 2024-01-13 4:21 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240113042254.38602-1-shyamthakkar001@gmail.com>
Add tests for amending the commit to add Signed-off-by trailer. And
also to check if it does not add another trailer if one already exists.
Currently, there are tests for --signoff separately in t7501, however,
they are not tested with --amend.
Therefore, these tests belong with other similar tests of --amend in
t7501-commit-basic-functionality.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3e18b879b5..db37e9051b 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# FIXME: Test the various index usages, test reflog,
-# signoff
+# FIXME: Test the various index usages, test reflog
test_description='git commit'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -464,6 +463,28 @@ test_expect_success 'amend commit to fix date' '
'
+test_expect_success 'amend commit to add signoff' '
+
+ test_commit "msg" file content &&
+ git commit --amend --signoff &&
+ test_commit_message HEAD <<-EOF
+ msg
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+'
+
+test_expect_success 'amend does not add signoff if it already exists' '
+
+ test_commit --signoff "tenor" file newcontent &&
+ git commit --amend --signoff &&
+ test_commit_message HEAD <<-EOF
+ tenor
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+'
+
test_expect_success 'commit mentions forced date in output' '
git commit --amend --date=2010-01-02T03:04:05 >output &&
grep "Date: *Sat Jan 2 03:04:05 2010" output
--
2.43.0
^ permalink raw reply related
* [PATCH v5 1/2] t7501: add tests for --include and --only
From: Ghanshyam Thakkar @ 2024-01-13 4:21 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240113042254.38602-1-shyamthakkar001@gmail.com>
Add tests for --only (-o) and --include (-i). This include testing
with or without staged changes for both -i and -o. Also to test
for committing untracked files with -i, -o and without -i/-o.
Some tests already exist in t7501 for testing --only, however,
it is tested in combination with --amend and --allow-empty and on
to-be-born branch. The addition of these tests check, when the
pathspec is provided, that only the files matching the pathspec
get committed. This behavior is same when we provide --only and
checked by the tests.
(as --only is the default mode of operation when pathspec is
provided.)
As for --include, there is no prior test for checking if --include
also commits staged changes, thus add test for that. Along with
the test also document a potential bug, in which, when provided
with -i and an untracked pathspec, commit does not fail if there
are staged changes. And when there are no staged changes commit
fails. However, no error is returned to stderr in either of the
cases.
Thus, these tests belong in t7501 with other similar existing tests,
as described in the case of --only.
And also add a test for checking incompatibilty when using -o and
-i together.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 77 ++++++++++++++++++++++++++-
1 file changed, 76 insertions(+), 1 deletion(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..3e18b879b5 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
# Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
#
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
# signoff
test_description='git commit'
@@ -92,6 +92,32 @@ test_expect_success '--long fails with nothing to commit' '
test_must_fail git commit -m initial --long
'
+test_expect_success 'fail to commit untracked pathspec (even with --include/--only)' '
+ echo content >baz &&
+ error="error: pathspec .baz. did not match any file(s) known to git" &&
+
+ test_must_fail git commit -m "baz" baz 2>err &&
+ test_grep -e "$error" err &&
+
+ test_must_fail git commit --only -m "baz" baz 2>err &&
+ test_grep -e "$error" err &&
+
+ # TODO: as for --include, the below command will fail because nothing is
+ # staged. If something was staged, it would not fail even if the
+ # pathspec was untracked (however, pathspec will remain untracked and
+ # staged changes would be committed.) In either cases, no error is
+ # returned to stderr like in (-o and without -o/-i) cases. In a
+ # similar manner, "git add -u baz" also does not error out.
+ #
+ # Therefore, the below test is just to document the current behavior
+ # and is not an endorsement to the current behavior, and we may
+ # want to fix this. And when that happens, this test should be
+ # updated accordingly.
+
+ test_must_fail git commit --include -m "baz" baz 2>err &&
+ test_must_be_empty err
+'
+
test_expect_success 'setup: non-initial commit' '
echo bongo bongo bongo >file &&
git commit -m next -a
@@ -117,6 +143,55 @@ test_expect_success '--long with stuff to commit returns ok' '
git commit -m next -a --long
'
+for opt in "" "-o" "--only"
+do
+ test_expect_success 'exclude additional staged changes when given pathspec' '
+ echo content >>file &&
+ echo content >>baz &&
+ git add baz &&
+ git commit $opt -m "file" file &&
+
+ git diff --name-only >actual &&
+ test_must_be_empty actual &&
+
+ git diff --name-only --staged >actual &&
+ test_cmp - actual <<-EOF &&
+ baz
+ EOF
+
+ git diff --name-only HEAD^ HEAD >actual &&
+ test_cmp - actual <<-EOF
+ file
+ EOF
+ '
+done
+
+test_expect_success '-i/--include includes staged changes' '
+ echo content >>file &&
+ echo content >>baz &&
+ git add file &&
+
+ # baz is in the index, therefore, it will be committed
+ git commit --include -m "file and baz" baz &&
+
+ git diff --name-only HEAD >remaining &&
+ test_must_be_empty remaining &&
+
+ git diff --name-only HEAD^ HEAD >changes &&
+ test_cmp - changes <<-EOF
+ baz
+ file
+ EOF
+'
+
+test_expect_success '--include and --only do not mix' '
+ test_when_finished "git reset --hard" &&
+ echo content >>file &&
+ echo content >>baz &&
+ test_must_fail git commit --include --only -m "file baz" file baz 2>actual &&
+ test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual
+'
+
test_expect_success 'commit message from non-existing file' '
echo more bongo: bongo bongo bongo bongo >file &&
test_must_fail git commit -F gah -a
--
2.43.0
^ permalink raw reply related
* [PATCH v5 0/2] t7501: add tests for --include, --only, --signoff
From: Ghanshyam Thakkar @ 2024-01-13 4:21 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240112180109.59350-1-shyamthakkar001@gmail.com>
The v5 of this series updates the '--only' test to use the for loop
and >> (append) operator to test all three cases (-o/--only/none) via a
single test.
This version also documents a bug found via the previous iteration of
this series, which does not report error when --include is used with
invalid pathspec.
Signoff tests remain unchanged.
Ghanshyam Thakkar (2):
t7501: add tests for --include and --only
t7501: add tests for --amend --signoff
t/t7501-commit-basic-functionality.sh | 100 +++++++++++++++++++++++++-
1 file changed, 98 insertions(+), 2 deletions(-)
--
2.43.0
^ permalink raw reply
* RE: Suggested clarification for .gitattributes reference documentation
From: Michael Litwak @ 2024-01-13 2:56 UTC (permalink / raw)
To: brian m. carlson; +Cc: git@vger.kernel.org
In-Reply-To: <SJ0PR10MB56937175632E5D82793CE13EFA6F2@SJ0PR10MB5693.namprd10.prod.outlook.com>
I just installed Git for Windows 2.43.0 and noticed the installer offers three options for altering the PATH:
1) Run git from git bash only
2) Run git from git bash, cmd.exe and PowerShell (RECOMMENDED)
3) Run git from git bash, cmd.exe and PowerShell with optional utilities (warning: will override find, sort and other system utilities).
It turns out iconv.exe is accessible from cmd.exe (Command Prompt) only when you take the third option. But iconv.exe is NOT optional. It is required for git to deal with UTF-16LE with BOM text conversions (and probably for numerous other encoding conversions).
But when PATH option #2 is chosen, and iconv.exe is unreachable from a Windows Command Prompt, the git commands which call upon iconv.exe do NOT indicate the error. The call to iconv.exe fails silently. It is only later after you commit, push and clone the repo again that you see the encoding failures.
And the warning about overriding find and sort must be taken with a grain of salt, since the Windows versions of those programs are accessed via a Windows folder which appears earlier in the PATH.
So this Git for Windows installer screen is misleading. And perhaps iconv.exe should be relocated so it is accessible even when PATH option #2 is chosen. I intend to submit an issue on the Git for Windows issue tracker regarding this. I'll also submit an issue about the lack of an error when running 'git add' for a UTF-16LE with BOM file under PATH option #2.
Thanks,
- Michael
-----Original Message-----
From: brian m. carlson <sandals@crustytoothpaste.net>
Sent: Friday, January 12, 2024 1:50 PM
To: Michael Litwak <michael.litwak@nuix.com>
Cc: git@vger.kernel.org
Subject: [EXTERNAL]Re: Suggested clarification for .gitattributes reference documentation
On 2024-01-12 at 21:25:19, Michael Litwak wrote:
> Please note, Git for Windows does not support UTF-16LE encoding when running git
> commands from an ordinary Command Prompt. Use a git bash console instead.
This sounds like a Git for Windows bug. Rather than documenting it, could you open an issue for it on their project?
> NEW TEXT:
>
> You can get a list of all available encodings on your platform with the following command:
>
> iconv --list
>
> For Git for Windows users the command, above, is only supported when running in a 'git bash' console.
That sounds like a PATH misconfiguration on your part. Have you checked your PATH settings to make sure that the path including the binary is included?
> CONCLUSION:
>
> Text files encoded with UTF-16LE with BOM are common in the Windows
> world, as some versions of Visual Studio will use this as the default
> encoding for .rc or .mc files. Solution files, project files and
> other Visual Studio files can also be in this format. Other encodings
> are common, too, e.g. some older versions of PowerShell defaulted to
> UTF-16BE with BOM for new .ps1 files. Yet users continue to experience
> encoding errors even when they are using the proper
> working-tree-encoding in their .gitattributes file. Part of this is
> due to the complexity of Git and the number of different platforms it
> supports.
I should point out that UTF-8 is pretty much the standard these days in many domains, even on Windows. For example, nobody is going to be pleased if you write a web page in any variant of UTF-16, and some languages, such as Rust, are simply defined to be in UTF-8 and won't work if you put them in any other encoding. Almost all editors these days do support UTF-8 (without BOM), even on Windows, so we do want to strongly encourage that rather than having people use UTF-16. The Git FAQ specifically outlines UTF-8 as the recommended way, which is most portable and most functional.
We have also documented the UTF-16LE-BOM case specifically in the Git FAQ (git help gitfaq) under "I'm on Windows and my text files are detected as binary". Answering questions on Stack Overflow, I realize that nobody actually reads the FAQ, but we did clearly document how to do it. That being said, I'm not opposed to an additional mention in the
gitattributes(5) page if you want to send an actual patch.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
^ permalink raw reply
* Re: [PATCH v4 1/2] t7501: add tests for --include and --only
From: Ghanshyam Thakkar @ 2024-01-13 1:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, phillip.wood123, christian.couder
In-Reply-To: <xmqqmsta58ya.fsf@gitster.g>
On Sat Jan 13, 2024 at 6:46 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > +test_expect_success '-i/--include includes staged changes' '
> > + echo newcontent >file &&
> > + echo newcontent >baz &&
> > + git add file &&
> > + git commit --include -m "file baz" baz &&
>
> I may have said this already, but the command invocation that does
> not result in an error smells like a bug, and I doubt that we want
> to etch the current behaviour into stone, which may make it harder
> to fix [*].
Yeah, that is why baz is added in the index in the test before the one
you quoted. And as I understand it, when everything is in the index, it
works as intended. Therefore to not get in the way of amending this
behaviour, no untracked files are being (attempted to be) committed in
these tests (except 'fail to commit untracked files' test, but that is
not what you quoted above).
> Another related behaviour that I suspect is a bug is that if you did
>
> git add -u baz
>
> instead of this "git commit -i baz", I think the command will
> silently succeed without doing anything. They may be the same bug,
> because "git commit -i <pathspec>" is an equivalent to "git add -u
> <pathspec>" followed by "git commit" after all. Both should say
> "there is no such path to update that matches the pathspec <baz>"
> and error out, I suspect.
Yeah, just checked, that also succeeds silently.
> Thanks.
>
> [Footnote]
>
> * A reasonable way out to unblock this particular patch may be to
> clarify that this test is only documenting the current behaviour
> without necessarily endorsing it. Perhaps
>
> echo more >>file &&
> echo more >>baz &&
> git add file &&
>
> # Note: "git commit -i baz" is like "git add -u baz"
> # followed by "git commit" but because baz is untracked,
> # only "file" is committed.
> # This test only documents this current behaviour, which we
> # may want to fix, and when it happens, this needs to be
> # adjusted to the new behaviour.
> git commit -i -m "file and baz" baz &&
>
> or something, probably.
as stated above, baz is tracked from the previous test. In any case,
I will add a note just to document that untracked files also do not
give any error (when there are staged changes) but are not committed.
Thanks.
^ permalink raw reply
* Re: [PATCH] diffcore-delta: avoid ignoring final 'line' of file
From: Elijah Newren @ 2024-01-13 1:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <xmqqedenearc.fsf@gitster.g>
On Thu, Jan 11, 2024 at 3:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/diffcore-delta.c b/diffcore-delta.c
> > index c30b56e983b..7136c3dd203 100644
> > --- a/diffcore-delta.c
> > +++ b/diffcore-delta.c
> > @@ -159,6 +159,10 @@ static struct spanhash_top *hash_chars(struct repository *r,
> > n = 0;
> > accum1 = accum2 = 0;
> > }
> > + if (n > 0) {
> > + hashval = (accum1 + accum2 * 0x61) % HASHBASE;
> > + hash = add_spanhash(hash, hashval, n);
> > + }
>
> OK, so we were ignoring the final short bit that is not terminated
> with LF due to the "continue". Nicely found.
>
> > diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> > index 85be1367de6..29299acbce7 100755
> > --- a/t/t4001-diff-rename.sh
> > +++ b/t/t4001-diff-rename.sh
> > @@ -286,4 +286,23 @@ test_expect_success 'basename similarity vs best similarity' '
> > test_cmp expected actual
> > '
> >
> > +test_expect_success 'last line matters too' '
> > + test_write_lines a 0 1 2 3 4 5 6 7 8 9 >nonewline &&
> > + printf "git ignores final up to 63 characters if not newline terminated" >>nonewline &&
> > + git add nonewline &&
> > + git commit -m "original version of file with no final newline" &&
>
> I found it misleading that the file whose name is nonewline has
> bunch of LF including on its last line ;-).
Yeah, sorry. It's been a while, but I think I started with a file
with only one line that had no LF, but then realized for inexact
rename detection to kick in I needed some other lines, at least one of
which didn't match...but I forgot to update the filename after adding
the other lines...
> > + # Change ONLY the first character of the whole file
> > + test_write_lines b 0 1 2 3 4 5 6 7 8 9 >nonewline &&
>
> Same here, but it is too much to bother rewriting it ...
>
> {
> test_write_lines ...
> printf ...
> } >incomplete
>
> ... like so ("incomplete" stands for "file ending with an incomplete line"),
> so I'll let it pass.
>
> > + printf "git ignores final up to 63 characters if not newline terminated" >>nonewline &&
>
>
> > + git add nonewline &&
> > + git mv nonewline still-no-newline &&
> > + git commit -a -m "rename nonewline -> still-no-newline" &&
> > + git diff-tree -r -M01 --name-status HEAD^ HEAD >actual &&
> > + cat >expected <<-\EOF &&
> > + R097 nonewline still-no-newline
>
> I am not very happy with the hardcoded 97. You are already using
> the non-standard 10% threshold. If the delta detection that
> forgets about the last line is so broken as your proposed log
> message noted, shouldn't you be able to construct a sample pair of
> preimage and postimage for which the broken version gives so low
> similarity to be judged not worth treating as a rename, while the
> fixed version gives reasonable similarity to be made into a rename,
> by the default threshold? That way, the test only needs to see if
> we got a rename (with any similarity) or a delete and an add.
Oops, the threshold is entirely unnecessary here; not sure why I
didn't remember to take it out (originally used the threshold while
testing without the fix to just how low of a similarity git thought
these nearly identical files had).
Since you don't like the threshold, and since we don't seem to have a
summary format that reports on the rename without the percentage, I
guess I need to munge the output with sed:
sed -e "s/^R[0-9]* /R /" actual >actual.munged &&
and then compare the expected output to that. I'll send in a patch
doing so and fix up the filenames and drop the rename threshold while
at it.
^ permalink raw reply
* Re: [PATCH 00/10] Enrich Trailer API
From: Linus Arver @ 2024-01-13 1:35 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <xmqqy1cx9dm4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This patch series is the first 10 patches of a much larger series I've been
>> working. The main goal of this series is to enrich the API in trailer.h. The
>> larger series brings a number of additional code simplifications and
>> cleanups (exposing and fixing some bugs along the way), and builds on top of
>> this series. The goal of the larger series is to make the trailer interface
>> ready for unit testing. By "trailer API" I mean those functions exposed in
>> trailer.h.
>
> Are there places in the current code that deals with trailers but
> does not use the trailer API (e.g., manually parse and/or insert the
> trailer in an in-core buffer)?
While working on this series I unfortunately did not search for such use
cases (I limited the scope of my work to mainly the interpret-trailers
builtin). But, a quick search just now on master branch turned up
append_signoff() in sequencer.c which constructs a Signed-off-by trailer
manually with a raw strbuf [1].
This is somewhat understandable though, because AFAICS the current
trailer API does not support creating and formatting single trailer
objects.
> Is it part of the larger goal to
> update these places so that we will always use the trailer API to
> touch trailers
That sounds like The Right Thing to do.
> , and if so, have these places been identified?
Not yet, but, it should be easy to check any remaining cases other than
the one I identified up above.
> Obviously the reason why I ask is that testing cannot be the goal by
> itself.
I now seem to recall an offline discussion where I said I wanted to
enrich the API to make other parts of Git also use this new API. Somehow
I've left that motivation out of the cover letter (will include in the
next reroll).
> The "alternative" ...
>
>> As an alternative to this patch series, we could keep trailer.h intact and
>> decide to unit-test the existing "trailer_info_get()" function which does
>> most of the trailer parsing work.
>
> ... may allow you to "test", but it would make it more difficult in
> the future to revamp the trailer API, if it is needed, in order to
> cover code paths that ought to be using but currently bypassing the
> trailer API.
Agreed. I should include this rationale as well in the next cover
letter, thanks.
>> This series breaks up "process_trailers()" into smaller pieces, exposing
>> many of the parts relevant to trailer-related processing in trailer.h. This
>> forces us to start writing unit tests for these now public functions, but
>> that is a good thing because those same unit tests should be easy to write
>> (due to their small(er) sizes), but also, because those unit tests will now
>> ensure some degree of stability across new versions of trailer.h (we will
>> start noticing when the behavior of any of these API functions change).
>
> And helper functions, each of which does one small thing well, may
> be more applicable to other code paths that are currently bypassing
> the API.
Yep.
>> Thanks to the aggressive refactoring in this series, I've been able to
>> identify and fix a couple bugs in our existing implementation. Those fixes
>> build on top of this series but were not included here, in order to keep
>> this series small.
>
> It would be nicer to have a concise list of these fixes (in the form
> of "git shortlog") as a teaser here ;-). That would hopefully
> entice others into reviewing this part that forms the foundation.
Ah, good idea. Here's a shortlog (with a short summary of each one) of
bug fixes that are forthcoming:
trailer: trailer replacement should not change its position
(Summary: If we found a trailer we'd like to replace, preserve its
position relative to the other trailers found in the trailer
block, instead of always moving it to the beginning or end of the
entire trailer block.)
interpret-trailers: preserve trailers coming from the input
(Summary: Sometimes, the parsed trailers from the input will be
formatted differently depending on whether we provide
--only-trailers or not. Make the trailers that were not modified
and are coming directly from the input get formatted the same way,
regardless of this flag.)
interpret-trailers: fail if given unrecognized arguments
(Summary: E.g., for "--where", only accept recognized WHERE_* enum
values. If we get something unrecognized, fail with an error
instead of silently doing nothing. Ditto for "--if-exists" and
"--if-missing".)
The last one is a different class of bug than the first two, and perhaps
less interesting.
Thanks.
[1] https://github.com/git/git/blob/d4dbce1db5cd227a57074bcfc7ec9f0655961bba/sequencer.c#L5299-L5301
^ permalink raw reply
* Re: [PATCH v4 1/2] t7501: add tests for --include and --only
From: Junio C Hamano @ 2024-01-13 1:16 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, phillip.wood123, christian.couder
In-Reply-To: <20240112180109.59350-2-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> +test_expect_success '-i/--include includes staged changes' '
> + echo newcontent >file &&
> + echo newcontent >baz &&
> + git add file &&
> + git commit --include -m "file baz" baz &&
I may have said this already, but the command invocation that does
not result in an error smells like a bug, and I doubt that we want
to etch the current behaviour into stone, which may make it harder
to fix [*].
Another related behaviour that I suspect is a bug is that if you did
git add -u baz
instead of this "git commit -i baz", I think the command will
silently succeed without doing anything. They may be the same bug,
because "git commit -i <pathspec>" is an equivalent to "git add -u
<pathspec>" followed by "git commit" after all. Both should say
"there is no such path to update that matches the pathspec <baz>"
and error out, I suspect.
Thanks.
[Footnote]
* A reasonable way out to unblock this particular patch may be to
clarify that this test is only documenting the current behaviour
without necessarily endorsing it. Perhaps
echo more >>file &&
echo more >>baz &&
git add file &&
# Note: "git commit -i baz" is like "git add -u baz"
# followed by "git commit" but because baz is untracked,
# only "file" is committed.
# This test only documents this current behaviour, which we
# may want to fix, and when it happens, this needs to be
# adjusted to the new behaviour.
git commit -i -m "file and baz" baz &&
or something, probably.
^ permalink raw reply
* Re: [PATCH v4 1/2] t7501: add tests for --include and --only
From: Ghanshyam Thakkar @ 2024-01-13 1:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, phillip.wood123, christian.couder
In-Reply-To: <xmqqbk9q6tcs.fsf@gitster.g>
On Sat Jan 13, 2024 at 4:40 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
> > test_must_fail git commit -m initial --long
> > '
> >
> > +test_expect_success 'fail to commit untracked file' '
> > + echo content >baz &&
> > + test_must_fail git commit -m "baz" baz
> > +'
> > +
> > +test_expect_success '--only also fail to commit untracked file' '
> > + test_must_fail git commit --only -m "baz" baz
> > +'
> > +
> > +test_expect_success '--include also fail to commit untracked file' '
> > + test_must_fail git commit --include -m "baz" baz
> > +'
>
> As the latter two depends on the first one's side effect of leaving
> an untracked 'baz' file in the working tree, I do not know if it is
> sensible to split these into three tests. An obvious alternative is
> to have a single test
>
> test_expect_success 'pathspec that do not match any tracked path' '
> echo content >baz &&
> test_must_fail git commit -m baz baz &&
> test_must_fail git commit -o -m baz baz &&
> test_must_fail git commit -i -m baz baz
> '
>
> By the way, I do not think presence of 'baz' in the working tree
> matters in the failures from these tests all that much, as the
> reason they fail is because the pathspec does not match any tracked
> file, whose contents in the index to be updated before made into a
> commit.
Yes, that is true. However, as per your prior email in which you stated
about --include:
"Now which behaviour between "error out because there is no path in
the index that matches pathspec 'baz'" and "add baz to the index and
commit that addition, together with what is already in the index" we
would want to take is probably open for discussion. Such a discussion
may decide that the current behaviour is fine. Until then..."
If in such a discussion it is decided that -i should add to index and
commit, then by not having 'baz' in the working tree, the -i test
would still error out regardless of whether its behaviour is to [add
to the index and commit] or [error out]. Therefore, by having 'baz'
we can detect the change between [-i adds to index and commits] or
[errors out].
> Likewise. An obvious thing to notice is that this cannot use the
> same "contents" text as before, even though it claims to be "same as
> above". If the final contents of "file" and "baz" does not matter,
> but it matters more that these files have been changed, it often is
> a good idea to append to the file. That way, you can ensure that
> you will be making them different, no matter what the initial
> condition was, i.e.,
>
> for opt in "" "-o" "--only"
> do
> test_expect_success 'skip over already added change' '
> echo more >>file &&
> echo more >>baz &&
> git add baz &&
> git commit $opt -m "file" file &&
>
> ... ensure that changes to file are committed
> ... and changes to baz is only in the index
> '
> done
>
> let's you test all three combinations.
Yeah, that is a more effective approach. I will change it and reroll
quickly.
>
> Thanks.
Thank you for all the help!
^ permalink raw reply
* Re: Help: Trying to setup triangular workflow
From: Matthew B. Gray @ 2024-01-13 0:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240112073136.GG618729@coredump.intra.peff.net>
Hi Peff,
> This push step is rewriting your upstream config. Do you have
> push.autoSetupRemote configured? In general you wouldn't want that for a
> triangular flow.
Thanks for the pointer, yes have both push.autoSetupRemote and
branch.autoSetupMerge set. Turning these off has fixed my problem.
With neither option set the example yields:
@{upstream}: refs/remotes/origin/main
@{push}: refs/remotes/myfork/mybranch
With push.autosetupremote=true set:
@{upstream}: refs/remotes/origin/main
@{push}: refs/remotes/myfork/mybranch
With branch.autoSetupMerge=simple set:
@{upstream}:
@{push}: refs/remotes/myfork/mybranch
With branch.autoSetupMerge=simple and push.autosetupremote=true:
@{upstream}: refs/remotes/myfork/mybranch
@{push}: refs/remotes/myfork/mybranch
> Though I think it also is only supposed to kick in if there is no
> tracking configured already. Why did the "git switch" invocation not set
> up tracking itself? When I run those commands it does. Do you have
> branch.autoSetupMerge turned off in your config?
If you're interested in looking at my full gitconfig it's hosted here:
https://github.com/heymatthew/dotfiles/blob/trunk/softlinks/.gitconfig
However looks like taking those out of global config has fixed my issue.
Thanks for the help!
Ngā mihi,
Matthew
^ permalink raw reply
* Re: [PATCH] strvec: use correct member name in comments
From: Linus Arver @ 2024-01-13 0:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Linus Arver via GitGitGadget, git
In-Reply-To: <xmqqjzoe8br0.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Side note: should we start naming the parameters in strvec.h? I would
>> think that it wouldn't hurt at this point (as the API is pretty stable).
>> If you think that's worth it, I could reroll to include that in this
>> series (and also improve my commit message for this patch).
>
> I am not sure if it adds more value to outweigh the cost of
> churning. When the meaning of the parameters are obvious only by
> looking at their types, a prototype without parameter names is
> easier to maintain, by allowing the parameters to be renamed only
> once in the implementation. When the meaning of parameters are not
> obvious from their types, we do want them to be named so that you
> only have to refer to the header files to know the argument order.
This sounds like a good rule to me.
> "void *calloc(size_t, size_t)" would not tell us if we should pass
> the size of individual element or the number of elements first, and
> writing "void *calloc(size_t nmemb, size_t size)" to make it more
> obvious is a good idea.
>
> On the other hand, "void *realloc(void *, size_t)" is sufficient to
> tell us that we are passing a pointer as the first parameter and the
> desired size as the second parameter, without them having any name.
Thanks for the illuminating examples. Agreed.
> Are there functions declared in strvec.h you have in mind that their
> parameters are confusing and hard to guess what they mean?
TBH I only learned recently (while writing the patch in this thread)
that parameter names in prototypes were optional. I got a little
confused initially when looking at strvec.h for the first time because
none of the parameters were named. Having thought a bit more about these
functions, none of them have repeated types like in your example where
naming is warranted, so I think they're fine as is.
OTOH if we were treating these .h files as something meant for direct
external consumption (that is, if strvec.h is libified and external
users outside of Git are expected to use it directly as their first
point of documentation), at that point it might make sense to name the
parameters (akin to the style of manpages for syscalls). But I imagine
at that point we would have some other means of developer docs (beyond
raw header files) for libified parts of Git, so even in that case it's
probably fine to keep things as is.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 1/2] t7501: add tests for --include and --only
From: Junio C Hamano @ 2024-01-12 23:10 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, phillip.wood123, christian.couder
In-Reply-To: <20240112180109.59350-2-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
> test_must_fail git commit -m initial --long
> '
>
> +test_expect_success 'fail to commit untracked file' '
> + echo content >baz &&
> + test_must_fail git commit -m "baz" baz
> +'
> +
> +test_expect_success '--only also fail to commit untracked file' '
> + test_must_fail git commit --only -m "baz" baz
> +'
> +
> +test_expect_success '--include also fail to commit untracked file' '
> + test_must_fail git commit --include -m "baz" baz
> +'
As the latter two depends on the first one's side effect of leaving
an untracked 'baz' file in the working tree, I do not know if it is
sensible to split these into three tests. An obvious alternative is
to have a single test
test_expect_success 'pathspec that do not match any tracked path' '
echo content >baz &&
test_must_fail git commit -m baz baz &&
test_must_fail git commit -o -m baz baz &&
test_must_fail git commit -i -m baz baz
'
By the way, I do not think presence of 'baz' in the working tree
matters in the failures from these tests all that much, as the
reason they fail is because the pathspec does not match any tracked
file, whose contents in the index to be updated before made into a
commit.
> @@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' '
> git commit -m next -a --long
> '
>
> +test_expect_success 'only commit given path (also excluding additional staged changes)' '
> + echo content >file &&
> + echo content >baz &&
> + git add baz &&
> + git commit -m "file" file &&
> +
> + git diff --name-only >actual &&
> + test_must_be_empty actual &&
> +
> + git diff --name-only --staged >actual &&
> + test_cmp - actual <<-EOF &&
> + baz
> + EOF
> +
> + git diff --name-only HEAD^ HEAD >actual &&
> + test_cmp - actual <<-EOF
> + file
> + EOF
> +'
OK. The change to baz is already in the index, but "commit file"
will skip over it and record only the changes to file in the commit.
Very much makes sense.
> +test_expect_success 'same as above with -o/--only' '
> + echo change >file &&
> + echo change >baz &&
> + git add baz &&
> + git commit --only -m "file" file &&
> +
> + git diff --name-only >actual &&
> + test_must_be_empty actual &&
> +
> + git diff --name-only --staged >actual &&
> + test_cmp - actual <<-EOF &&
> + baz
> + EOF
> +
> + git diff --name-only HEAD^ HEAD >actual &&
> + test_cmp - actual <<-EOF
> + file
> + EOF
> +'
Likewise. An obvious thing to notice is that this cannot use the
same "contents" text as before, even though it claims to be "same as
above". If the final contents of "file" and "baz" does not matter,
but it matters more that these files have been changed, it often is
a good idea to append to the file. That way, you can ensure that
you will be making them different, no matter what the initial
condition was, i.e.,
for opt in "" "-o" "--only"
do
test_expect_success 'skip over already added change' '
echo more >>file &&
echo more >>baz &&
git add baz &&
git commit $opt -m "file" file &&
... ensure that changes to file are committed
... and changes to baz is only in the index
'
done
let's you test all three combinations.
Thanks.
^ permalink raw reply
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Junio C Hamano @ 2024-01-12 22:44 UTC (permalink / raw)
To: Achu Luma, Rubén Justo; +Cc: git, christian.couder, Christian Couder
In-Reply-To: <20240112102122.1422-1-ach.lumap@gmail.com>
Achu Luma <ach.lumap@gmail.com> writes:
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for advise_if_enabled functionality
> from the legacy approach using the test-tool command `test-tool advise`
> in t/helper/test-advise.c to the new unit testing framework
> (t/unit-tests/test-lib.h).
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
In the light of the presense of work like this one
https://lore.kernel.org/git/c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com/
I am not sure if moving this to unit-test framework is a good thing,
at least right at this moment.
To test the effect of setting one configuration variable, and ensure
it results in a slightly different advice message output to the
standard error stream, "test-tool advice" needs only a single line
of patch, but if we started with this version, how much work does it
take to run the equivalent test in the other patch if it were to be
rebased on top of this change? It won't be just the matter of
adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
will it?
I doubt the issue is not about "picking the right moment" to
transition from the test-tool to unit testing framework in this
particular case. Is "check-advice-if-enabled" a bit too high level
a feature to be a good match to "unit" testing, I have to wonder?
Thanks.
^ permalink raw reply
* Re: Suggested clarification for .gitattributes reference documentation
From: Michael Litwak @ 2024-01-12 22:36 UTC (permalink / raw)
To: brian m. carlson; +Cc: git@vger.kernel.org
In-Reply-To: <ZaG0EkADl8hQZaqf@tapette.crustytoothpaste.net>
Regarding Git for Windows requiring 'git bash' console to perform text conversion...
>This sounds like a Git for Windows bug. Rather than documenting it, could you open an issue for it on their project?
>That sounds like a PATH misconfiguration on your part. Have you checked your PATH settings to make sure that the path including the binary is included?
If it is merely that I need to adjust my PATH so iconv.exe is accessible, that simplifies everything; however, it could possibly be a Git for Windows installer bug (or perhaps the installer offered to change my PATH and I declined).
I'll check out both possibilities.
>We have also documented the UTF-16LE-BOM case specifically in the Git FAQ (git help gitfaq) under "I'm on Windows and my text files are detected as binary".
I'll take a look and perhaps revise my suggested documentation edits.
Thanks,
- Michael
-----Original Message-----
From: brian m. carlson <sandals@crustytoothpaste.net>
Sent: Friday, January 12, 2024 1:50 PM
To: Michael Litwak <michael.litwak@nuix.com>
Cc: git@vger.kernel.org
Subject: [EXTERNAL]Re: Suggested clarification for .gitattributes reference documentation
On 2024-01-12 at 21:25:19, Michael Litwak wrote:
> Please note, Git for Windows does not support UTF-16LE encoding when running git
> commands from an ordinary Command Prompt. Use a git bash console instead.
This sounds like a Git for Windows bug. Rather than documenting it, could you open an issue for it on their project?
> NEW TEXT:
>
> You can get a list of all available encodings on your platform with the following command:
>
> iconv --list
>
> For Git for Windows users the command, above, is only supported when running in a 'git bash' console.
That sounds like a PATH misconfiguration on your part. Have you checked your PATH settings to make sure that the path including the binary is included?
> CONCLUSION:
>
> Text files encoded with UTF-16LE with BOM are common in the Windows
> world, as some versions of Visual Studio will use this as the default
> encoding for .rc or .mc files. Solution files, project files and
> other Visual Studio files can also be in this format. Other encodings
> are common, too, e.g. some older versions of PowerShell defaulted to
> UTF-16BE with BOM for new .ps1 files. Yet users continue to experience
> encoding errors even when they are using the proper
> working-tree-encoding in their .gitattributes file. Part of this is
> due to the complexity of Git and the number of different platforms it
> supports.
I should point out that UTF-8 is pretty much the standard these days in many domains, even on Windows. For example, nobody is going to be pleased if you write a web page in any variant of UTF-16, and some languages, such as Rust, are simply defined to be in UTF-8 and won't work if you put them in any other encoding. Almost all editors these days do support UTF-8 (without BOM), even on Windows, so we do want to strongly encourage that rather than having people use UTF-16. The Git FAQ specifically outlines UTF-8 as the recommended way, which is most portable and most functional.
We have also documented the UTF-16LE-BOM case specifically in the Git FAQ (git help gitfaq) under "I'm on Windows and my text files are detected as binary". Answering questions on Stack Overflow, I realize that nobody actually reads the FAQ, but we did clearly document how to do it. That being said, I'm not opposed to an additional mention in the
gitattributes(5) page if you want to send an actual patch.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
^ permalink raw reply
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-12 22:19 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Taylor Blau, Jeff King, Dragan Simic
In-Reply-To: <c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> advice.c | 109 +++++++++++++++++++++++++---------------------
> t/t0018-advice.sh | 1 -
> 2 files changed, 59 insertions(+), 51 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index f6e4c2f302..8474966ce1 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
> return "";
> }
>
> +enum advice_level {
> + ADVICE_LEVEL_DEFAULT = 0,
We may want to say "_NONE" not "_DEFAULT" to match the convention
used elsewhere, but I have no strong preference. I do have a
preference to see that, when we explicitly say "In this enum, there
is ADVICE_LEVEL_DEFAULT and its value is zero" with "= 0" in the
definition, the places we use a variable of this enum type, we say
if (!variable)
and not
if (variable == ADVICE_LEVEL_DEFAULT)
> + ADVICE_LEVEL_DISABLED,
> + ADVICE_LEVEL_ENABLED,
> +};
> +
> static struct {
> const char *key;
> - int enabled;
> + enum advice_level level;
> } advice_setting[] = {
> ...
> - [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
> + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" },
> ...
> + [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" },
> };
It certainly becomes nicer to use the "unspecified is left to 0"
convention like this.
> static const char turn_off_instructions[] =
> @@ -116,13 +122,13 @@ void advise(const char *advice, ...)
>
> int advice_enabled(enum advice_type type)
> {
> - switch(type) {
> - case ADVICE_PUSH_UPDATE_REJECTED:
> - return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
> - advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
> - default:
> - return advice_setting[type].enabled;
> - }
> + int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
OK.
> + if (type == ADVICE_PUSH_UPDATE_REJECTED)
> + return enabled &&
> + advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
I like the textbook use of a simple recursion here ;-)
> + return enabled;
> }
> void advise_if_enabled(enum advice_type type, const char *advice, ...)
> @@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
> return;
>
> va_start(params, advice);
> - vadvise(advice, 1, advice_setting[type].key, params);
> + vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
> + advice_setting[type].key, params);
OK. Once you configure this advice to be always shown, you no
longer are using the _DEFAULT, so we pass 0 as the second parameter.
Makes sense.
As I said, if we explicitly document that _DEFAULT's value is zero
with "= 0" in the enum definition, which we also rely on the
initialization of the advice_setting[] array, then it may probably
be better to write it more like so:
vadvice(advice, !advice_setting[type].level,
advice_setting[type].key, params);
I wonder if it makes this caller simpler to update the return value
of advice_enabled() to a tristate. Then we can do
int enabled = advice_enabled(type);
if (!enabled)
return;
va_start(...);
vadvice(advice, enabled != ADVICE_LEVEL_ENABLED, ...);
va_end(...);
but it does not make that much difference.
> va_end(params);
> }
>
> @@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
> for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
> if (strcasecmp(k, advice_setting[i].key))
> continue;
> - advice_setting[i].enabled = git_config_bool(var, value);
> + advice_setting[i].level = git_config_bool(var, value)
> + ? ADVICE_LEVEL_ENABLED
> + : ADVICE_LEVEL_DISABLED;
OK. We saw (var, value) in the configuration files we are parsing,
so we find [i] that corresponds to the advice key and tweak the
level.
This loop obviously is O(N*M) for the number of "advice.*"
configuration variables N and the number of advice keys M, but that
is not new to this patch---our expectation is that N will be small,
even though M will grow over time.
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index c13057a4ca..0dcfb760a2 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
> test_expect_success 'advice should be printed when config variable is set to true' '
> cat >expect <<-\EOF &&
> hint: This is a piece of advice
> - hint: Disable this message with "git config advice.nestedTag false"
> EOF
> test_config advice.nestedTag true &&
> test-tool advise "This is a piece of advice" 2>actual &&
OK. The commit changes behaviour for existing users who explicitly
said "I want to see this advice" by configuring advice.* key to 'true',
and it probably is a good thing, even though it may be a bit surprising.
One thing that may be missing is a documentation update. Something
along this line to highlight that now 'true' has a bit different
meaning from before (and as a consequence, we can no longer say
"defaults to true").
Documentation/config/advice.txt | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git c/Documentation/config/advice.txt w/Documentation/config/advice.txt
index 2737381a11..364a8268b6 100644
--- c/Documentation/config/advice.txt
+++ w/Documentation/config/advice.txt
@@ -1,7 +1,11 @@
advice.*::
- These variables control various optional help messages designed to
- aid new users. All 'advice.*' variables default to 'true', and you
- can tell Git that you do not need help by setting these to 'false':
+
+ These variables control various optional help messages
+ designed to aid new users. When left unconfigured, Git will
+ give you the help message with an instruction on how to
+ squelch it. When set to 'true', the help message is given,
+ but without hint on how to squelch the message. You can
+ tell Git that you do not need help by setting these to 'false':
+
--
ambiguousFetchRefspec::
^ permalink raw reply related
* RE: Make git ls-files omit deleted files
From: rsbecker @ 2024-01-12 22:18 UTC (permalink / raw)
To: 'Junio C Hamano', 'Raul Rangel'; +Cc: git
In-Reply-To: <xmqqply68c87.fsf@gitster.g>
On Friday, January 12, 2024 4:37 PM, Junio C Hamano wrote:
>Raul Rangel <rrangel@google.com> writes:
>
>> I'm trying to copy my current git worktree to a new directory, while
>> including all modified and untracked files, but excluding any ignored
>> files.
>
>Curiously missing from the above is "unmodified". You only talked about
modified,
>untracked, and ignored, but what do you want to do with them?
>
>As you are grabbing the files from the working tree, I suspect that you do
not want
>to base your decision on what is in the index, which means that ls-files
might be a
>wrong tool for the job.
Coupled with ls-files, using git status --porcelain might give some insight
into what the condition of each modified and untracked file/directory is.
^ permalink raw reply
* Re: Make git ls-files omit deleted files
From: Raul Rangel @ 2024-01-12 21:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqply68c87.fsf@gitster.g>
On Fri, Jan 12, 2024 at 2:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Raul Rangel <rrangel@google.com> writes:
>
> > I'm trying to copy my current git worktree to a new directory, while
> > including all modified and untracked files, but excluding any ignored
> > files.
>
> Curiously missing from the above is "unmodified". You only talked
> about modified, untracked, and ignored, but what do you want to do
> with them?
I guess another way of saying it is, I want to make a copy of the
worktree while honoring .gitignore. i.e., I don't want my copied work
tree to contain any ignored files.
>
> As you are grabbing the files from the working tree, I suspect that
> you do not want to base your decision on what is in the index, which
> means that ls-files might be a wrong tool for the job.
Hrmm, that makes sense. Do you have any recommendations?
^ permalink raw reply
* Re: Suggested clarification for .gitattributes reference documentation
From: brian m. carlson @ 2024-01-12 21:50 UTC (permalink / raw)
To: Michael Litwak; +Cc: git@vger.kernel.org
In-Reply-To: <SJ0PR10MB569379A093B83BE01A04C789FA6F2@SJ0PR10MB5693.namprd10.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]
On 2024-01-12 at 21:25:19, Michael Litwak wrote:
> Please note, Git for Windows does not support UTF-16LE encoding when running git
> commands from an ordinary Command Prompt. Use a git bash console instead.
This sounds like a Git for Windows bug. Rather than documenting it,
could you open an issue for it on their project?
> NEW TEXT:
>
> You can get a list of all available encodings on your platform with the following command:
>
> iconv --list
>
> For Git for Windows users the command, above, is only supported when running in a 'git bash' console.
That sounds like a PATH misconfiguration on your part. Have you checked
your PATH settings to make sure that the path including the binary is
included?
> CONCLUSION:
>
> Text files encoded with UTF-16LE with BOM are common in the Windows
> world, as some versions of Visual Studio will use this as the default
> encoding for .rc or .mc files. Solution files, project files and
> other Visual Studio files can also be in this format. Other encodings
> are common, too, e.g. some older versions of PowerShell defaulted to
> UTF-16BE with BOM for new .ps1 files. Yet users continue to experience
> encoding errors even when they are using the proper
> working-tree-encoding in their .gitattributes file. Part of this is
> due to the complexity of Git and the number of different platforms it
> supports.
I should point out that UTF-8 is pretty much the standard these days in
many domains, even on Windows. For example, nobody is going to be
pleased if you write a web page in any variant of UTF-16, and some
languages, such as Rust, are simply defined to be in UTF-8 and won't
work if you put them in any other encoding. Almost all editors these
days do support UTF-8 (without BOM), even on Windows, so we do want to
strongly encourage that rather than having people use UTF-16. The Git
FAQ specifically outlines UTF-8 as the recommended way, which is most
portable and most functional.
We have also documented the UTF-16LE-BOM case specifically in the Git
FAQ (git help gitfaq) under "I'm on Windows and my text files are
detected as binary". Answering questions on Stack Overflow, I realize
that nobody actually reads the FAQ, but we did clearly document how to
do it. That being said, I'm not opposed to an additional mention in the
gitattributes(5) page if you want to send an actual patch.
--
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
* Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper
From: Junio C Hamano @ 2024-01-12 21:50 UTC (permalink / raw)
To: Linus Arver; +Cc: Jiang Xin, Git List, Jiang Xin
In-Reply-To: <owlyy1cvhua5.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>>
>> When commit b236752a (Support remote archive from all smart transports,
>> 2009-12-09) added "remote archive" support for "smart transports", it
>> was for transport that supports the ".connect" method. The
>> "connect_helper()" function protected itself from getting called for a
>> transport without the method before calling process_connect_service(),
>
> OK.
>
>> which did not work with such a transport.
>
> How about 'which only worked with the ".connect" method.' ?
>
>>
>> Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
>> 2018-03-15) added a way for a transport without the ".connect" method
>> to establish a "stateless" connection in protocol-v2, which
>
> s/which/where
>
>> process_connect_service() was taught to handle the "stateless"
>> connection,
>
> I think using 'the ".stateless_connect" method' is more consistent with
> the rest of this text.
>
>> making the old safety valve in its caller that insisted
>> that ".connect" method must be defined too strict, and forgot to loosen
>> it.
>
> I think just "...making the old protection too strict. But edc9caf7
> forgot to adjust this protection accordingly." is simpler to read.
>
>> Remove the restriction in the "connect_helper()" function and give the
>> function "process_connect_service()" the opportunity to establish a
>> connection using ".connect" or ".stateless_connect" for protocol v2. So
>> we can connect with a stateless-rpc and do something useful. E.g., in a
>> later commit, implements remote archive for a repository over HTTP
>> protocol.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> ---
>> transport-helper.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/transport-helper.c b/transport-helper.c
>> index 49811ef176..2e127d24a5 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>>
>> /* Get_helper so connect is inited. */
>> get_helper(transport);
>> - if (!data->connect)
>> - die(_("operation not supported by protocol"));
>
> Should we still terminate early here if both data->connect and
> data->stateless_connect are not truthy? It would save a few CPU cycles,
> but even better, remain true to the the original intent of the code.
> Maybe there was a really good reason to terminate early here that we're
> not aware of?
>
> But also, what about the case where both are enabled? Should we print an
> error message? (Maybe this concern is outside the scope of this series?)
>
>> if (!process_connect_service(transport, name, exec))
>> die(_("can't connect to subservice %s"), name);
>> --
>> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev
Thanks for a review to get the topic that hasn't seen much reviews
unstuck. Very much appreciated.
^ permalink raw reply
* [PATCH] add ctx menu for tags, impl "remove tag" and "copy tag name"
From: Ulrich Hornung via GitGitGadget @ 2024-01-12 21:48 UTC (permalink / raw)
To: git; +Cc: Ulrich Hornung, Ulrich Hornung
From: Ulrich Hornung <hornunguli@gmx.de>
Signed-off-by: Ulrich Hornung <hornunguli@gmx.de>
---
gitk: add ctx menu for tags, impl "remove tag" and "copy tag name"
Hello,
I'm currently using git gui and gitk quite strongly. It happens to me
once in a while that I accidentially create a tag instead of a new
branch when using the GUI of gitk for this. Sadly, in this situation I
needed to go to the console to remove this tag again, because the latest
version of gitk doesn't have a "remove tag" button.
Lukily I was able to implement that button (and the copy name button) by
myself. I would be happy if you could integrate the new feature into the
official sources.
Bildschirmfoto vom 2024-01-01 17-26-07
[https://github.com/gitgitgadget/git/assets/252806/079c28dd-b4a0-486c-ad1e-27f7f2fde814]
Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1629%2Fcre4ture%2Ffeature%2Ftag_ctx_menu_remove_tag-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1629/cre4ture/feature/tag_ctx_menu_remove_tag-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1629
gitk-git/gitk | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 7a087f123d7..7a70e1fba31 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1894,6 +1894,21 @@ proc removehead {id name} {
unset headids($name)
}
+# update things when a tag has been removed
+proc removetag {id name} {
+ global tagids idtags
+
+ if {$idtags($id) eq $name} {
+ unset idtags($id)
+ } else {
+ set i [lsearch -exact $idtags($id) $name]
+ if {$i >= 0} {
+ set idtags($id) [lreplace $idtags($id) $i $i]
+ }
+ }
+ unset tagids($name)
+}
+
proc ttk_toplevel {w args} {
global use_ttk
eval [linsert $args 0 ::toplevel $w]
@@ -2096,7 +2111,7 @@ proc makewindow {} {
global uifgcolor uifgdisabledcolor
global filesepbgcolor filesepfgcolor
global mergecolors foundbgcolor currentsearchhitbgcolor
- global headctxmenu progresscanv progressitem progresscoords statusw
+ global headctxmenu tagctxmenu progresscanv progressitem progresscoords statusw
global fprogitem fprogcoord lastprogupdate progupdatepending
global rprogitem rprogcoord rownumsel numcommits
global have_tk85 use_ttk NS
@@ -2705,6 +2720,13 @@ proc makewindow {} {
}
$headctxmenu configure -tearoff 0
+ set tagctxmenu .tagctxmenu
+ makemenu $tagctxmenu {
+ {mc "Remove this tag" command rmtag}
+ {mc "Copy tag name" command {clipboard clear; clipboard append $tagmenutag}}
+ }
+ $tagctxmenu configure -tearoff 0
+
global flist_menu
set flist_menu .flistctxmenu
makemenu $flist_menu {
@@ -6701,6 +6723,7 @@ proc drawtags {id x xt y1} {
-font $font -tags [list tag.$id text]]
if {$ntags >= 0} {
$canv bind $t <1> $tagclick
+ $canv bind $t $ctxbut [list tagmenu %X %Y $id $tag_quoted]
} elseif {$nheads >= 0} {
$canv bind $t $ctxbut [list headmenu %X %Y $id $tag_quoted]
}
@@ -9938,6 +9961,20 @@ proc headmenu {x y id head} {
tk_popup $headctxmenu $x $y
}
+# context menu for a tag
+proc tagmenu {x y id head} {
+ global tagmenuid tagmenutag tagctxmenu
+
+ stopfinding
+ set tagmenuid $id
+ set tagmenutag $head
+ array set state {0 normal 1 normal}
+ foreach i {0 1} {
+ $tagctxmenu entryconfigure $i -state $state($i)
+ }
+ tk_popup $tagctxmenu $x $y
+}
+
proc cobranch {} {
global headmenuid headmenuhead headids
global showlocalchanges
@@ -10042,6 +10079,27 @@ proc rmbranch {} {
run refill_reflist
}
+proc rmtag {} {
+ global tagmenuid tagmenutag
+
+ set tag $tagmenutag
+ set id $tagmenuid
+
+ nowbusy rmtag
+ update
+ if {[catch {exec git tag -d $tag} err]} {
+ notbusy rmtag
+ error_popup $err
+ return
+ }
+ removetag $id $tag
+ removedtag $id $tag
+ redrawtags $id
+ notbusy rmtag
+ dispneartags 0
+ run refill_reflist
+}
+
# Display a list of tags and heads
proc showrefs {} {
global showrefstop bgcolor fgcolor selectbgcolor NS
@@ -11273,6 +11331,13 @@ proc removedhead {hid head} {
unset -nocomplain cached_dheads
}
+proc removedtag {hid head} {
+ global cached_dtags cached_atags
+
+ unset -nocomplain cached_dtags
+ unset -nocomplain cached_atags
+}
+
proc movedhead {hid head} {
global arcnos arcout cached_dheads
base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] strvec: use correct member name in comments
From: Junio C Hamano @ 2024-01-12 21:47 UTC (permalink / raw)
To: Linus Arver; +Cc: Jeff King, Linus Arver via GitGitGadget, git
In-Reply-To: <owlyo7dqig1w.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Side note: should we start naming the parameters in strvec.h? I would
> think that it wouldn't hurt at this point (as the API is pretty stable).
> If you think that's worth it, I could reroll to include that in this
> series (and also improve my commit message for this patch).
I am not sure if it adds more value to outweigh the cost of
churning. When the meaning of the parameters are obvious only by
looking at their types, a prototype without parameter names is
easier to maintain, by allowing the parameters to be renamed only
once in the implementation. When the meaning of parameters are not
obvious from their types, we do want them to be named so that you
only have to refer to the header files to know the argument order.
"void *calloc(size_t, size_t)" would not tell us if we should pass
the size of individual element or the number of elements first, and
writing "void *calloc(size_t nmemb, size_t size)" to make it more
obvious is a good idea.
On the other hand, "void *realloc(void *, size_t)" is sufficient to
tell us that we are passing a pointer as the first parameter and the
desired size as the second parameter, without them having any name.
Are there functions declared in strvec.h you have in mind that their
parameters are confusing and hard to guess what they mean?
Thanks.
^ permalink raw reply
* Re: Make git ls-files omit deleted files
From: Junio C Hamano @ 2024-01-12 21:37 UTC (permalink / raw)
To: Raul Rangel; +Cc: git
In-Reply-To: <CAHQZ30Ad6+YM9qnCOeNNy8e2k-AbYR_bBXTups-7z6=ioyqS5Q@mail.gmail.com>
Raul Rangel <rrangel@google.com> writes:
> I'm trying to copy my current git worktree to a new directory, while
> including all modified and untracked files, but excluding any ignored
> files.
Curiously missing from the above is "unmodified". You only talked
about modified, untracked, and ignored, but what do you want to do
with them?
As you are grabbing the files from the working tree, I suspect that
you do not want to base your decision on what is in the index, which
means that ls-files might be a wrong tool for the job.
^ permalink raw reply
* Suggested clarification for .gitattributes reference documentation
From: Michael Litwak @ 2024-01-12 21:25 UTC (permalink / raw)
To: git@vger.kernel.org
The .gitattributes documentation should be clarified to ensure files encoded as UTF-16 are properly accounted for,
In particular for Windows users.
Specifically, within the working-tree encoding topic https://git-scm.com/docs/gitattributes#_working_tree_encoding, I suggest the following edits:
NEW BULLETED PARAGRAPH UNDER THE HEADING "Please note that using the working-tree-encoding
attribute may have a number of pitfalls:"
* Git for Windows is not able to access the iconv.exe text conversion program from an ordinary
Command Prompt. Be sure to run 'git clone' or 'git add' from a git bash console or a Git
GUI.
OLD TEXT
As an example, use the following attributes if your *.ps1 files are UTF-16 encoded with byte order mark (BOM)
and you want Git to perform automatic line ending conversion based on your platform.
*.ps1 text working-tree-encoding=UTF-16
Use the following attributes if your *.ps1 files are UTF-16 little endian encoded without BOM
and you want Git to use Windows line endings in the working directory (use UTF-16LE-BOM instead
of UTF-16LE if you want UTF-16 little endian with BOM). Please note, it is highly recommended
to explicitly define the line endings with eol if the working-tree-encoding attribute is used
to avoid ambiguity.
*.ps1 text working-tree-encoding=UTF-16LE eol=CRLF
NEW TEXT (SPECIFYING UTF-16BE EXPLICITLY IN THE FIRST EXAMPLE, AND WITH A NEW SEPARATE EXAMPLE FOR UTF-16LE WITH BOM)
As an example, use the following attributes if your *.ps1 files are UTF-16 big endian encoded
with byte order mark (BOM) and you want Git to perform automatic line ending conversion
based on your platform.
*.ps1 text working-tree-encoding=UTF-16
Use the following attributes if your *.ps1 files are UTF-16 little endian encoded without BOM
and you want Git to use Windows line endings in the working directory.
*.ps1 text working-tree-encoding=UTF-16LE eol=CRLF
Use the following attributes if your *.ps1 files are UTF-16 little endian encoded with BOM
and you want Git to use Windows line endings in the working directory.
*.ps1 text working-tree-encoding=UTF-16LE-BOM eol=CRLF
Please note, it is highly recommended to explicitly define the line endings with eol
if the working-tree-encoding attribute is used to avoid ambiguity.
Please note, Git for Windows does not support UTF-16LE encoding when running git
commands from an ordinary Command Prompt. Use a git bash console instead.
OLD TEXT:
You can get a list of all available encodings on your platform with the following command:
iconv --list
NEW TEXT:
You can get a list of all available encodings on your platform with the following command:
iconv --list
For Git for Windows users the command, above, is only supported when running in a 'git bash' console.
In the thread "help request: unable to merge UTF-16-LE "text" file" at https://lore.kernel.org/git/Yl8uiflurfjuLIvD@camp.crustytoothpaste.net/, Brian m. Carlson, Chris Torek and others describe tips for dealing with improper encoding, such as the following:
if you have already checked the file in without an appropriate
working-tree-encoding, you should run `git add --renormalize .` and then
commit. You'll need to do that (or merge in a commit that does that) on
every branch you want to work with.
> For that to work, it is likely that you'd need to convert not just
> the tips of two branches getting merged, but also the merge base
> commit, so that all three trees involved in the 3-way merge are in
> the same text encoding.
The old merge-recursive has `-X renormalize` that I believe would
do this for you. (I see code in merge-ort for this as well, but have no
handy means to test it myself.)
So a NEW SECTION describing ways to deal with improper text file encoding could be added under the
working-tree-encoding topic, specifically a description of what the following two
commands can do to remedy improper encoding:
git add --renormalize
git merge-recursive -X renormalize
CONCLUSION:
Text files encoded with UTF-16LE with BOM are common in the Windows world, as some versions of Visual Studio will use this as the default encoding for .rc or .mc files. Solution files, project files and other Visual Studio files can also be in this format. Other encodings are common, too, e.g. some older versions of PowerShell defaulted to UTF-16BE with BOM for new .ps1 files. Yet users continue to experience encoding errors even when they are using the proper working-tree-encoding in their .gitattributes file. Part of this is due to the complexity of Git and the number of different platforms it supports.
Ideally Git would automatically detect the most common UTF encodings and treat these files as diffable text files on all platforms -- without the need for entries in .gitattributes. And it would be great if Git for Windows could handle common UTF text encodings when executed in an ordinary Command Prompt. Until then, clarifying and enhancing the documentation for .gitattributes could go a long way in making text encoding easier for Git users. Thanks for considering these revisions.
- Michael
^ permalink raw reply
* Make git ls-files omit deleted files
From: Raul Rangel @ 2024-01-12 21:19 UTC (permalink / raw)
To: git
Hello,
I'm trying to copy my current git worktree to a new directory, while
including all modified and untracked files, but excluding any ignored
files. I essentially want a copy of the tree assuming someone ran `git
add --all && git commit` and cloned that commit into a different
directory.
I got pretty close with the following command:
$ git ls-files --cached --modified --others --exclude-standard
--deduplicate | xargs cp --parents --no-dereference
--target-directory=/tmp/clone/
The problem is that if I have an unstaged delete, `ls-files` will
print it out as a modification and `cp` will print an error saying
that it failed to copy the non-existing file.
The man page states this behavior:
> --modified: Show files with an unstaged modification (note that an unstaged deletion also counts as an unstaged modification)
Is there another way to achieve what I'm doing that's not too
expensive? If not, could a `--no-deleted-as-modified` flag be added?
Thanks,
Raul
^ permalink raw reply
* Re: [PATCH v2 2/3] advice: fix an unexpected leading space
From: Junio C Hamano @ 2024-01-12 21:19 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <xmqqsf32bfsn.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> ... How about doing it in the pre-commit hook?
Sorry, as it runs before obtaining the proposed commit log message
and making a commit, pre-commit is a wrong one to use. I meant to
say commit-msg hook that is used to verify the contents of the
proposed commit log message.
^ 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