* 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
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Junio C Hamano @ 2024-01-12 21:14 UTC (permalink / raw)
To: Michael Lohmann; +Cc: phillip.wood123, git, phillip.wood
In-Reply-To: <20240112150346.73735-1-mi.al.lohmann@gmail.com>
Michael Lohmann <mi.al.lohmann@gmail.com> writes:
> Almost, but not quite: "git log —merge" only shows the commits touching the
> conflict, so it would be equivalent to (I think):
>
> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)
>
> (or replace CHERRY_PICK with one of the other actions)
It can certainly _reduce_ the noise, but I am not sure if it works
over the right history segment. Let me think aloud a bit.
Let's imagine that in a history forked long time ago,
O----O----O----O----X HEAD
\
Z---Z---Z---Z---A---B CHERRY_PICK_HEAD
where all commits modified the same path in question that you saw
conflicts in when your "git cherry-pick B" stopped.
I do not know what to think about the changes to that path by the
commits denoted by 'O', but the changes done to the path by commits
denoted by 'Z' should have absolutely no relevance, as the whole
point of cherry-picking B relative to A is because we do not care
about what Zs did, no? For that matter, given that how we structure
the 3-way merge for such a cherry-pick to "move from X the same way
as you would move from A to B", how you got to X (i.e. Os) should
not matter, either.
On the other hand, such a conflict may arise from the fact that Os
and Zs made changes differently to cause the contents of the path at
X and A differ significantly. So, OK, I can buy your argument that
what Os and Zs to the conflicted path did can be interesting when
understanding the conflict during such a cherry-pick.
>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.
>
> True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
But that is very different, isn't it? Merging two unrelated
histories is like merging two histories where the common ancestor
had an empty tree, i.e.
o---o---o---X HEAD
/
(v) an imaginary ancestor with an empty tree
\
o---o---o---O MERGE_HEAD
so it is a reasonable degenerated behaviour to consider what all
commits on both sides did to the paths in question.
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Michael Lohmann @ 2024-01-12 21:06 UTC (permalink / raw)
To: gitster; +Cc: git, mi.al.lohmann, phillip.wood123
In-Reply-To: <xmqqr0im9ub9.fsf@gitster.g>
> > tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a
> > common ancestor. As I say I've not used "git log --merge" so maybe I'm
> > missing the reason why it would be useful when cherry-picking or
> > rebasing.
>
> Good question. It is the whole point of cherry-pick that
> CHERRY_PICK_HEAD and the current HEAD may not have any meaningful
> ancestry relationship, so yes, it is questionable to use the same
> logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using
> HEAD...MERGE_HEAD does make perfect sense.
I tried to say it in [1]: a merge also does not need a common ancestor! Try it:
git init test
cd test
echo something > README.md
git add --all
git commit -m "initial commit"
git remote add origin_git git://git.kernel.org/pub/scm/git/git.git
git fetch origin_git master # obviously no common ancestor
git merge --allow-unrelated-histories origin_git/master
# merge conflict
git log --merge # Still loggs all the conflicting commits
So indeed the command that Phillip wrote is not equivalent and the existing
logic already can deal with unrelated histories. Or am I misunderstanding?
Michael
[1] https://lore.kernel.org/git/20240112150346.73735-1-mi.al.lohmann@gmail.com/
^ permalink raw reply
* Re: help request: unable to merge UTF-16-LE "text" file
From: Michael Litwak @ 2024-01-12 20:34 UTC (permalink / raw)
To: kioplato@gmail.com; +Cc: git@vger.kernel.org, kevinlong206@gmail.com
This is in reply to the message at
https://lore.kernel.org/all/20220422184211.5z67sxrgq2zm3tvd@compass/#t
Sent via MS Outlook - hope this reaches the online thread.
------------------------------------------------------------------------
On Windows, if you do
git add
from an ordinary Command Prompt, it will fail to call iconv.exe to perform
the necessary text conversion. I.E. your UTF-16LE with BOM file will not be
properly converted by Git to UTF-8 for its internal storage, leading to
subsequent encoding errors.
So, in addition to setting the working-tree-encoding of the file to
UTF-16LE-BOM in .gitattributes prior to adding the file, be sure to run:
git add
from a git bash console when adding such files.
- Michael
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Junio C Hamano @ 2024-01-12 20:21 UTC (permalink / raw)
To: phillip.wood123; +Cc: Michael Lohmann, git
In-Reply-To: <47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com>
phillip.wood123@gmail.com writes:
> I should start by saying that I didn't know "git log --merge" existed
> before I saw this message so please correct me if I've misunderstood
> what this patch is doing. If I understand correctly it shows the
> commits from each side of the merge and is equivalent to
>
> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
>
> When a commit is cherry-picked the merge base used is
> CHERRY_PICK_HEAD^ [*] so I'm not sure what
>
> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
>
> tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a
> common ancestor. As I say I've not used "git log --merge" so maybe I'm
> missing the reason why it would be useful when cherry-picking or
> rebasing.
Good question. It is the whole point of cherry-pick that
CHERRY_PICK_HEAD and the current HEAD may not have any meaningful
ancestry relationship, so yes, it is questionable to use the same
logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using
HEAD...MERGE_HEAD does make perfect sense.
^ permalink raw reply
* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Junio C Hamano @ 2024-01-12 20:18 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Michael Lohmann, phillip.wood123, git
In-Reply-To: <ce46229d-8964-4445-9a17-cff40aca1cb4@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> What do you think about this idea? (Or am I maybe missing an easy way to
>> achieve it with the existing code as well?)
>
> Ha! Very nice patch. For comparison, have a look at my patch to achieve
> the same that I never submitted (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
>
> This implementation is more complete than mine. I like it.
I like the way your other_merge_candidate() loops over an array of
possible candidates, which makes it a lot easier to extend, though,
admittedly the "die()" message needs auto-generating if we really
wanted to make it maintenance free ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Junio C Hamano @ 2024-01-12 20:10 UTC (permalink / raw)
To: Michael Lohmann; +Cc: git, newren, phillip.wood123
In-Reply-To: <20240112155033.77204-1-mi.al.lohmann@gmail.com>
Michael Lohmann <mi.al.lohmann@gmail.com> writes:
>> but we may want to tighten the original's use of repo_get
>> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
>> > - die("--merge without MERGE_HEAD?");
>> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
>>
>> ... so that we won't be confused in a repository that has a tag
>> whose name happens to be MERGE_HEAD. We probably should be using
>> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
>
> Here I am really at the limit of my understanding of the core functions.
> Is this roughly what you had in mind? From my testing it seems to do the
> job, but I don't understand the details "refs_resolve_ref_unsafe"...
Perhaps there are others who are more familiar with the ref API than
I am, but I was just looking at refs.h today and wonder if something
along the lines of this ...
if (read_ref_full("MERGE_HEAD",
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
&oid, NULL))
die("no MERGE_HEAD");
if (is_null_oid(&oid))
die("MERGE_HEAD is a symbolic ref???");
... would be simpler.
With the above, we are discarding the refname read_ref_full()
obtains from its refs_resolve_ref_unsafe(), but I think that is
totally fine. We expect it to be "MERGE_HEAD" in the good case.
The returned value can be different from "MERGE_HEAD" if it is
a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
trusted, we should catch that case with the NULL-ness check on oid.
Which would mean that we do not need a separate "other_head"
variable, and instead can use "MERGE_HEAD".
> revision.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..786e1a3e89 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)
> struct commit *head, *other;
> struct object_id oid;
> const char **prune = NULL;
> + const char *other_head;
> int i, prune_num = 1; /* counting terminating NULL */
> struct index_state *istate = revs->repo->index;
>
> if (repo_get_oid(the_repository, "HEAD", &oid))
> die("--merge without HEAD?");
> head = lookup_commit_or_die(&oid, "HEAD");
> - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> + other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> + "MERGE_HEAD",
> + RESOLVE_REF_READING,
> + &oid,
> + NULL);
> + if (!other_head)
> die("--merge without MERGE_HEAD?");
> - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> + other = lookup_commit_or_die(&oid, other_head);
> add_pending_object(revs, &head->object, "HEAD");
> - add_pending_object(revs, &other->object, "MERGE_HEAD");
> + add_pending_object(revs, &other->object, other_head);
> bases = repo_get_merge_bases(the_repository, head, other);
> add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
> add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
^ permalink raw reply
* Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action
From: Michael Lohmann @ 2024-01-12 19:33 UTC (permalink / raw)
To: gitster
Cc: git, mi.al.lohmann, peff, phillip.wood123, phillip.wood,
wanja.hentze
In-Reply-To: <xmqqil3ybets.fsf@gitster.g>
On 12. Jan 2024, at 19:13, Junio C Hamano <gitster@pobox.com> wrote:
> > But your way of seeing it also makes sense to me. I think I just find
> > the "START" name jarring because we do not use that word elsewhere to
> > describe the action.
>
> Thanks. I forgot to say that I share the same feeling, both about
> "NONE could mean no-op" (but then seriously why would anybody sane
> want that?) and "START is not how we spell these things". I can see
> how DEFAULT could make sense, but if somebody picked DEFAULT between
> two sensible choices NONE and DEFAULT here, especially if they claim
> that they started this enum to mimick what is done in another place,
> and after they were told that the other place they are imitating
> follows the convention of using NONE for "nothing specified, so use
> default", I would have to say that they are trying to be different
> for the sake of being different, which is not a good sign. I'd want
> our contributors to be original where being original matters more.
I am sorry to have left this feeling in you. It was not my intention to
be original, but I just did not understand the reason for the other
name. If I wanted to be "sneaky" and wasn't truly open for a discussion
I would not have mentioned that it is different in the other file. I
don't try to be original for the sake of it, but yes indeed if I have a
hard time understanding some reasoning, in my day job it is my role to
ask these. But I think I am indeed questioning a bit too much here.
Sorry for that! You as the project lead constantly have to do the same
and I am in awe as how you handle it.
I am sorry that this discussion did get out of hand. Especially since
this patch does not even introduce a feature, but is only a refactoring
of an already perfectly fine codebase. My only intention was to align
builtin/refactor.c a bit more to builtin/rebase.c but the current state
is 100% good as is, so I think we should just drop this discussion.
> > + if (cmd != ACTION_START)
> >
> > Likewise here I'd probably leave this as "if (cmd)".
>
> I 100% agree with the suggestion to explicitly define something to
> be 0 when we are going to use it for its Boolean value. So an
> alternative would be to treat all ACTION_* enum values the same, and
> not define the first one explicitly to 0.
>
> Especially in the context of a patch that wants to turn if/elseif
> cascades to switch, I would suspect that the latter, as switch/case
> does not special case the falsehood among other possible values of
> integer type, might be easier to maintain in the longer term.
That is indeed what v4 of the patch did that I prepared half a day ago
and just did not have the time to properly check again before I submit
it. It also tackles the other issues you mentioned, but my feeling is
that the current state is good as it is with the characters and so we
should just drop this discussion.
Sorry to have caused such a stir and that I took so much of all of your
valuable time! I for myself have learned a great deal from all of you
and your interactions, so thank you!
Michael Lohmann
^ permalink raw reply
* Re: [PATCH] messages: mark some strings with "up-to-date" not to touch
From: Junio C Hamano @ 2024-01-12 18:19 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Benji Kay, Eric Sunshine
In-Reply-To: <20240112171910.11131-1-ericsunshine@charter.net>
Eric Sunshine <ericsunshine@charter.net> writes:
> Let's do the second best thing, leave a short comment near them
> explaining why those strings should not be modified or localized.
I simply could not come up with a short and concise in-code comment
;-) What you picked looks good to me.
Thanks.
>
> [es: make in-code comment more developer-friendly]
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> This is a reroll of Junio's[1] v1 which adds an in-code comment
> explaining that "up-to-date" messages in plumbing commands should not be
> changed, but doesn't explain _why_, which forces readers to dig through
> project history or the mailing list to understand the motivation. v2
> changes the comment to be more developer-friendly by adding the
> explanation directly to the comment.
>
> [1]: https://lore.kernel.org/git/xmqqjzofec0e.fsf@gitster.g/
>
> Range-diff:
> 1: 36596051c9 ! 1: 782169e0b1 messages: mark some strings with "up-to-date" not to touch
> @@ Commit message
> the commit is impossible to ask "git blame" to link back to the
> commit that did not touch them.
>
> - Let's do the second best thing, leave a short comment near them, to
> - make it possible for those who are motivated enough to find out why
> - we decided to tell them "do not modify".
> + Let's do the second best thing, leave a short comment near them
> + explaining why those strings should not be modified or localized.
> +
> + [es: make in-code comment more developer-friendly]
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> + Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>
> ## builtin/send-pack.c ##
> @@ builtin/send-pack.c: int cmd_send_pack(int argc, const char **argv, const char *prefix)
> }
>
> if (!ret && !transport_refs_pushed(remote_refs))
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> return ret;
> @@ http-push.c: int cmd_main(int argc, const char **argv)
>
> if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
> if (push_verbosely)
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "'%s': up-to-date\n", ref->name);
> if (helper_status)
> printf("ok %s up to date\n", ref->name);
> @@ http-push.c: int cmd_main(int argc, const char **argv)
> * commits at the remote end and likely
> * we were not up to date to begin with.
> */
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> error("remote '%s' is not an ancestor of\n"
> "local '%s'.\n"
> "Maybe you are not up-to-date and "
> @@ transport.c: int transport_push(struct repository *r,
> if (porcelain && !push_ret)
> puts("Done");
> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> -+ /* do not modify this string */
> ++ /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> done:
>
> builtin/send-pack.c | 1 +
> http-push.c | 2 ++
> transport.c | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index b7183be970..3df9eaad09 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -333,6 +333,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
> }
>
> if (!ret && !transport_refs_pushed(remote_refs))
> + /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> return ret;
> diff --git a/http-push.c b/http-push.c
> index b4d0b2a6aa..12d1113741 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1851,6 +1851,7 @@ int cmd_main(int argc, const char **argv)
>
> if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
> if (push_verbosely)
> + /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "'%s': up-to-date\n", ref->name);
> if (helper_status)
> printf("ok %s up to date\n", ref->name);
> @@ -1871,6 +1872,7 @@ int cmd_main(int argc, const char **argv)
> * commits at the remote end and likely
> * we were not up to date to begin with.
> */
> + /* stable plumbing output; do not modify or localize */
> error("remote '%s' is not an ancestor of\n"
> "local '%s'.\n"
> "Maybe you are not up-to-date and "
> diff --git a/transport.c b/transport.c
> index bd7899e9bf..df518ead70 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1467,6 +1467,7 @@ int transport_push(struct repository *r,
> if (porcelain && !push_ret)
> puts("Done");
> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> + /* stable plumbing output; do not modify or localize */
> fprintf(stderr, "Everything up-to-date\n");
>
> done:
^ permalink raw reply
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Junio C Hamano @ 2024-01-12 18:17 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Patrick Steinhardt, git, Stan Hu
In-Reply-To: <20240112151655.GA640039@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jan 12, 2024 at 02:12:43PM +0100, Johannes Schindelin wrote:
>
>> But my main concern is: Why does this happen in the first place? If we are
>> running with Bash, why does `BASH_XTRACEFD` to work as intended here and
>> makes it necessary to filter out the traced commands?
>
> BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient
> bash 3.2.57, which is the last GPLv2 version.
>
> One simple solution is to mark the script with test_untraceable. See
> 5fc98e79fc (t: add means to disable '-x' tracing for individual test
> scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as
> untraceable with '-x', 2018-02-24).
>
> That will disable tracing entirely in the script for older versions of
> bash, which could make debugging harder. But it will still work as
> expected for people on reasonable versions of bash, and doesn't
> introduce any complicated code.
Thank you, all three of you, for digging through to the bottom
quickly.
I too suspected a version of bash that is ancient and found out
about the "untraceable" bit just before I started reading this
thread ;-)
^ permalink raw reply
* Re: [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action
From: Junio C Hamano @ 2024-01-12 18:13 UTC (permalink / raw)
To: Jeff King
Cc: Michael Lohmann, git, phillip.wood123, phillip.wood, wanja.hentze
In-Reply-To: <20240112072404.GF618729@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But your way of seeing it also makes sense to me. I think I just find
> the "START" name jarring because we do not use that word elsewhere to
> describe the action.
Thanks. I forgot to say that I share the same feeling, both about
"NONE could mean no-op" (but then seriously why would anybody sane
want that?) and "START is not how we spell these things". I can see
how DEFAULT could make sense, but if somebody picked DEFAULT between
two sensible choices NONE and DEFAULT here, especially if they claim
that they started this enum to mimick what is done in another place,
and after they were told that the other place they are imitating
follows the convention of using NONE for "nothing specified, so use
default", I would have to say that they are trying to be different
for the sake of being different, which is not a good sign. I'd want
our contributors to be original where being original matters more.
>> +{
>> + switch (cmd) {
>> + case ACTION_CONTINUE: return "--continue";
>> + case ACTION_SKIP: return "--skip";
>> + case ACTION_ABORT: return "--abort";
>> + case ACTION_QUIT: return "--quit";
>> + case ACTION_START: BUG("no commandline flag for ACTION_START");
>> + }
>> +}
>
> I find this perfectly readable, and is likely the way I'd write it in a
> personal project. But in this project I find we tend to stick to more
> conventional formatting, like:
>
> switch (cmd) {
> case ACTION_CONTINUE:
> return "--continue";
> case ACTION_SKIP:
> return "--skip";
> ...and so on...
Same. I try to do the latter while working on this project, but I
do admit I use the former in small one-page tools I write outside
the context of this project.
>> + if (cmd != ACTION_START)
>
> Likewise here I'd probably leave this as "if (cmd)".
I 100% agree with the suggestion to explicitly define something to
be 0 when we are going to use it for its Boolean value. So an
alternative would be to treat all ACTION_* enum values the same, and
not define the first one explicitly to 0.
Especially in the context of a patch that wants to turn if/elseif
cascades to switch, I would suspect that the latter, as switch/case
does not special case the falsehood among other possible values of
integer type, might be easier to maintain in the longer term.
Thanks.
^ permalink raw reply
* Re: [PATCH] strvec: use correct member name in comments
From: Linus Arver @ 2024-01-12 18:04 UTC (permalink / raw)
To: Jeff King, Linus Arver via GitGitGadget; +Cc: git
In-Reply-To: <20240112074138.GH618729@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The source of the problem is that the patch originally used
> "items" in the struct, too
Ah, that makes sense.
> As you note, we still call use "items" for the vector passed in to
> pushv. I think that is OK, and there is no real need to use the terse
> "v" there (it is also purely internal; the declaration in strvec.h does
> not name it at all).
Indeed. Perhaps I should have included this in my commit message.
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).
^ permalink raw reply
* [PATCH v4 2/2] t7501: add tests for --amend --signoff
From: Ghanshyam Thakkar @ 2024-01-12 18:00 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240112180109.59350-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 e4633b4af5..33a9895e72 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
@@ -466,6 +465,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 v4 1/2] t7501: add tests for --include and --only
From: Ghanshyam Thakkar @ 2024-01-12 18:00 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240112180109.59350-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
(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.
Therefore, these tests belong in t7501 with other similar existing
tests, as described in the case of --only.
And also add test for checking incompatibilty when using -o and -i
together.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 79 ++++++++++++++++++++++++++-
1 file changed, 78 insertions(+), 1 deletion(-)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..e4633b4af5 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,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
+'
+
test_expect_success 'setup: non-initial commit' '
echo bongo bongo bongo >file &&
git commit -m next -a
@@ -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
+'
+
+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
+'
+
+test_expect_success '-i/--include includes staged changes' '
+ echo newcontent >file &&
+ echo newcontent >baz &&
+ git add file &&
+ git commit --include -m "file 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 new >file &&
+ echo new >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
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