* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-17 16:44 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git, Alexey Gladkov
In-Reply-To: <9203cd46-6a81-38e4-f191-da0b51e238c1@web.de>
René, sorry for late reply,
On 09/14, René Scharfe wrote:
>
> Am 13.09.23 um 11:46 schrieb Oleg Nesterov:
> >
> > I have another opinion. To me the 2nd "=..." marker does help to
> > understand the hit location. But this doesn't matter.
>
> You see it as another layer of information, as an annotation, an
> additional line containing meta-information. I saw them as context
> lines, i.e. lines from the original file shown in the original order
> without duplication, like - lines, with the only place for meta-
> information being the marker character itself.
Yes,
> > But without my patch, in this case I get
> >
> > TEST.c 1 void func1(struct pid *);
> > TEST.c 3 void func2(struct pid *pid)
> > TEST.c 5 use1(pid);
> > TEST.c 8 void func3(struct pid *pid)
> > TEST.c 10 use2(pid);
> >
> > because the output from git-grep
> >
> > $ git grep --untracked -pn pid TEST.c
> > TEST.c:1:void func1(struct pid *);
> > TEST.c:3:void func2(struct pid *pid)
> > TEST.c:5: use1(pid);
> > TEST.c:8:void func3(struct pid *pid)
> > TEST.c:10: use2(pid);
> >
> > doesn't have the "=..." markers at all.
>
> Sure, that's a problem. You could easily check whether a match is also
> a function line according to the default heuristic
Yes, but...
> there are some impressive regexes in userdiff.c
> and the script would have to figure out which language the file is
> configured to be for Git in the first place.
Yes, and this is what I'd like to avoid, I do not want to duplicate the
builtin_drivers[] logic.
> > in my editor without this patch, I get
> >
> > kernel/sys.c 224 sys_setpriority struct pid *pgrp;
[...snip...]
> Well, your script turns "SYSCALL_DEFINE3(setpriority, [...]" into
> "sys_setpriority" etc., so it is already knows a lot about function lines.
No, not a lot ;)
But yes sure, I can adapt this script to the current behaviour. In fact I
can even change it to not use "-p", the script can read the file backwards
itself (but of course I'd prefer to not do this).
Nevermind. Thanks for discussion. If I can't convince maintainers - lets
forget this patch. Although I will probably keep it (and another one I
had from the very begginning) for myself, it works for me.
> Can we use two markers, i.e. both : and =? No idea what that might break.
...
> So with the patch below this would look like this:
...
> kernel/sys.c#1073#SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
This works for me too. So please CC me if you ever push this change ;)
Thanks,
Oleg.
^ permalink raw reply
* Re: [BUG] `git push` sends unnecessary objects
From: Bagas Sanjaya @ 2023-09-17 13:21 UTC (permalink / raw)
To: Javier Mora, Git Mailing List; +Cc: Derrick Stolee, Junio C Hamano
In-Reply-To: <CAH1-q0iV+E73RrUDA8jcoFgNEfQDNwRnX5P5Z7r3Qj3GESV_7g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2863 bytes --]
On Wed, Sep 13, 2023 at 11:59:35PM +0100, Javier Mora wrote:
> I came across this issue accidentally when trying to move a directory
> containing a very large file, and deleting another file in that
> directory while I was at it.
> It seems to be caused by `pack.useSparse=true` being the default since
> v2.27 (which I found out after spending quite a while manually
> bisecting and compiling git since I noticed that this didn't happen in
> v2.25; commit de3a864 introduces this regression).
>
> * Expected:
> Pushing a commit that moves a file without modifying it shouldn't
> require sending a blob object for that file, since the remote server
> already has that blob object.
> * Observed:
> Pushing a commit that moves a directory containing a file and also
> adds/deletes other files in that directory will for some reason also
> send blobs for all the files in that directory, even the ones that
> were already in the remote.
> * Consequences:
> This has a very big impact in push times for very small commits
> that just move around files, if those files are very big (I had this
> happen with a >100MB file over a problematic connection... yikes!)
> * Note:
> The commit introducing the regression does warn about possible
> scenarios involving a special arrangement of exact copies across
> directories, but these are not "copies", I just moved a file, which
> seems like a rather common operation.
>
> Code snippet for reproduction:
> ```
> mkdir TEST_git
> cd TEST_git
>
> mkdir -p local remote/origin.git
> cd remote/origin.git
> git init --bare
> cd ../../local
> git init
> git remote add origin file://"${PWD%/*}"/remote/origin.git
>
> mkdir zig
> for i in a b c d e; do
> dd if=/dev/urandom of=zig/"$i" bs=1M count=1
> done
> git add .
> git commit -m 'Add big files'
> git push -u origin master
> #>> Writing objects: 100% (8/8), 5.00 MiB | 13.27 MiB/s, done.
> #^ makes sense: 1 commit + 2 trees (/ and /zig) + 5 files = 8;
> # 5 MiB in total for the 5x 1 MiB binary files
>
> git mv zig zag
> git commit -m 'Move zig'
> git push
> #>> Writing objects: 100% (2/2), 233 bytes | 233.00 KiB/s, done.
> #^ makes sense: 1 commit + 1 tree (/ renames /zig to /zag) = 2;
> # a,b,c,d,e objects already in remote
>
> git mv zag zog
> touch zog/f
> git add zog/f
> git commit -m 'For great justice'
> git push
> #>> Writing objects: 100% (9/9), 5.00 MiB | 24.63 MiB/s, done.
> #^ It re-uploaded the 5x 1 MiB blobs
> # even though remote already had them.
> ```
>
> Note that the latter doesn't happen if I use `git -c pack.useSparse=false push`.
I can reproduce this regression on v2.42.0 (self-compiled) on my Debian
testing system.
Cc'ing Derrick and Junio.
Thanks for the report!
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: Please explain avoiding history simplifications when diffing merges
From: Bagas Sanjaya @ 2023-09-17 9:58 UTC (permalink / raw)
To: Magnus Holmgren, git
Cc: Santi Béjar, Junio C Hamano, Sergey Organov,
Christian Couder, Jeff King, René Scharfe
In-Reply-To: <2250343.okVFLFBGsW@utklippan>
[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]
On Fri, Sep 15, 2023 at 05:10:28PM +0200, Magnus Holmgren wrote:
> Friday, 8 September 2023 11:09:20 CEST, I wrote
> > QGit was bitten by
> > https://github.com/git/git/commit/0dec322d31db3920872f43bdd2a7ddd282a5be67
>
> Maybe I should link to the QGit issue:
> https://github.com/tibirna/qgit/issues/129
>
> > It looks like passing --simplify-merges to override the default solves the
> > problem, but I still want to ask here because I'm not sure I fully
> > understand
> > the reasoning:
> > > the default history simplification would remove merge commits from
> > > consideration if the file "path" matched the second parent.
>
> As I wrote at the above URL, I realized that the old git log output without --
> simplify-merges and the output with --simplify-merges aren't quite the same.
> The old output indeed omits some interesting merge commits, which may explain
> why the change was made, but git log --simplify-merges does include them, so
> it seems a reasonable default to me.
Can you provide examples?
>
> However, QGit has a problem: git log --diff-merges=separate includes a
> separate diff for each parent, but only for each parent with differences
> compared to the merge commit, *and* there's no custom format placeholder for
> the current parent, only for the list of parents (%P/%p). How should one go
> about adding that? I figure the format_commit_context struct in pretty.c needs
> another field.
What are you trying to accomplish with your proposed formatting verbs?
Confused...
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re:Dear respectable I am waiting for your soonest response
From: Mr. Petrova @ 2023-09-16 20:35 UTC (permalink / raw)
To: git
My name is V. Petrova from Kyiv, Ukraine. I have tried to reach
you but find it difficult due to internet scarcity here in this
town. I am contacting you because I want to come over to you
country, I have some funds I plan to invest in your country
because I want to relocate my Late Father business due to ongoing
war in my country and visit your country to set up new investment
business you may advice profitable in your area.
I also have some million dollars i want to invest and 995kg of
24carat gold I want to ship to your country and establish gold
jewellery manufacturing company. I will offer you good monetary
rewards for your help, please for fast discussion, you can add me
on WhatsApp and lets talk, this my
WhatsApp number below
+380976656349
Thanking you as i wait to hear from you soon so i can tell you
more details.
God bless you for your care.
Regards,
V. Petrova
^ permalink raw reply
* Re: [PATCH 5/4] merge-ort: lowercase a few error messages
From: Jeff King @ 2023-09-16 22:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren
In-Reply-To: <xmqq34zdbxgn.fsf@gitster.g>
On Sat, Sep 16, 2023 at 02:49:12PM -0700, Junio C Hamano wrote:
> > <sigh> This fails CI because with GIT_TEST_MERGE_ALGORITHM=recursive, we
> > run the old merge-recursive code, which uses the capitalized version.
> >
> > I'm inclined to just drop this minor cleanup for now, and we can worry
> > about it later once merge-recursive goes the way of the dodo.
>
> I wonder if it is just the matter of making matching changes to the
> original error messages in merge-recursive that share the
> capitalized version?
It is, but I didn't want to touch merge-recursive at all if it is
destined for removal. But really, it is not much extra work to do so. So
here's an updated patch.
-- >8 --
Subject: [PATCH] merge-ort: lowercase a few error messages
As noted in CodingGuidelines, error messages should not be capitalized.
Fix up a few of these that were copied verbatim from merge-recursive to
match our modern style.
We'll likewise fix up the matching ones from merge-recursive. We care a
bit less there, since the hope is that it will eventually go away. But
besides being the right thing to do in the meantime, it is necessary for
t6406 to pass both with and without GIT_TEST_MERGE_ALGORITHM set (one of
our CI jobs sets it to "recursive", which will use the merge-recursive.c
code). An alternative would be to use "grep -i" in the test to check
the message, but it's nice for the test suite to be be more exact (we'd
notice if the capitalization fix regressed).
Signed-off-by: Jeff King <peff@peff.net>
---
merge-ort.c | 4 ++--
merge-recursive.c | 4 ++--
t/t6406-merge-attr.sh | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 3953c9f745..7857ce9fbd 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2105,12 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
&result_buf);
if ((merge_status < 0) || !result_buf.ptr)
- ret = error(_("Failed to execute internal merge"));
+ ret = error(_("failed to execute internal merge"));
if (!ret &&
write_object_file(result_buf.ptr, result_buf.size,
OBJ_BLOB, &result->oid))
- ret = error(_("Unable to add %s to database"), path);
+ ret = error(_("unable to add %s to database"), path);
free(result_buf.ptr);
if (ret)
diff --git a/merge-recursive.c b/merge-recursive.c
index 6a4081bb0f..0d7e57e2df 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1383,12 +1383,12 @@ static int merge_mode_and_contents(struct merge_options *opt,
extra_marker_size);
if ((merge_status < 0) || !result_buf.ptr)
- ret = err(opt, _("Failed to execute internal merge"));
+ ret = err(opt, _("failed to execute internal merge"));
if (!ret &&
write_object_file(result_buf.ptr, result_buf.size,
OBJ_BLOB, &result->blob.oid))
- ret = err(opt, _("Unable to add %s to database"),
+ ret = err(opt, _("unable to add %s to database"),
a->path);
free(result_buf.ptr);
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 05ad13b23e..72f8c1722f 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -180,7 +180,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
>./please-abort &&
echo "* merge=custom" >.gitattributes &&
test_must_fail git merge main 2>err &&
- grep "^error: Failed to execute internal merge" err &&
+ grep "^error: failed to execute internal merge" err &&
git ls-files -u >output &&
git diff --name-only HEAD >>output &&
test_must_be_empty output
--
2.42.0.662.g15203389d6
^ permalink raw reply related
* Re: [PATCH v2] git-gui - re-enable use of hook scripts
From: Junio C Hamano @ 2023-09-16 21:51 UTC (permalink / raw)
To: Mark Levedahl; +Cc: johannes.schindelin, me, git
In-Reply-To: <20230916210131.78593-1-mlevedahl@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> Linux: puts [file split {foo}] ==> foo
> Linux: puts [file split {/foo}] ==> / foo
> Linux: puts [file split {.git/foo}] ==> .git foo
> Windows: puts [file split {foo}] ==> foo
> Windows: puts [file split {c:\foo}] ==> c:/ foo
> Windows: puts [file split {.git\foo}] ==> .git foo
;-) Nice documentation of what you found out.
> diff --git a/git-gui.sh b/git-gui.sh
> index 8bc8892..8603437 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {
> set i $from_index
> while {$i < [llength $command_line]} {
> set cmd [lindex $command_line $i]
> - if {[file pathtype $cmd] ne "absolute"} {
> + if {[llength [file split $cmd]] < 2} {
> set fullpath [_which $cmd]
> if {$fullpath eq ""} {
> throw {NOT-FOUND} "$cmd not found in PATH"
Nice. Now we need to find a replacement maintainer for Git-gui ;-)
In the meantime, I can queue this patch on top of what I updated
git-gui part the last time with and merge it in.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/4] merge-ort: lowercase a few error messages
From: Junio C Hamano @ 2023-09-16 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: git, Elijah Newren
In-Reply-To: <20230916072909.GA992098@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Sep 16, 2023 at 02:01:00AM -0400, Jeff King wrote:
>
>> Here's one more clean-up on top. I hesitated on this for the initial
>> send just because I didn't know if we might want to switch these error
>> messages to path_msg(), which does capitalize sometimes. But Elijah's
>> response convinced me that we should just leave them in place, in which
>> case it makes sense to do a minimal style fixup.
>>
>> Junio, this is on top of what you've queued in
>> jk/ort-unused-parameter-cleanups.
>>
>> -- >8 --
>> Subject: [PATCH] merge-ort: lowercase a few error messages
>>
>> As noted in CodingGuidelines, error messages should not be capitalized.
>> Fix up a few of these that were copied verbatim from merge-recursive to
>> match our modern style.
>
> <sigh> This fails CI because with GIT_TEST_MERGE_ALGORITHM=recursive, we
> run the old merge-recursive code, which uses the capitalized version.
>
> I'm inclined to just drop this minor cleanup for now, and we can worry
> about it later once merge-recursive goes the way of the dodo.
I wonder if it is just the matter of making matching changes to the
original error messages in merge-recursive that share the
capitalized version?
^ permalink raw reply
* [PATCH v2] git-gui - re-enable use of hook scripts
From: Mark Levedahl @ 2023-09-16 21:01 UTC (permalink / raw)
To: gitster, johannes.schindelin, me, git; +Cc: Mark Levedahl
In-Reply-To: <xmqqy1h6auy7.fsf@gitster.g>
Earlier, commit aae9560a introduced search in $PATH to find executables
before running them, avoiding an issue where on Windows a same named
file in the current directory can be executed in preference to anything
in a directory in $PATH. This search is intended to find an absolute
path for a bare executable ( e.g, a function "foo") by finding the first
instance of "foo" in a directory given in $PATH, and this search works
correctly. The search is explicitly avoided for an executable named
with an absolute path (e.g., /bin/sh), and that works as well.
Unfortunately, the search is also applied to commands named with a
relative path. A hook script (or executable) $HOOK is usually located
relative to the project directory as .git/hooks/$HOOK. The search for
this will generally fail as that relative path will (probably) not exist
on any directory in $PATH. This means that git hooks in general now fail
to run. Considerable mayhem could occur should a directory on $PATH be
git controlled. If such a directory includes .git/hooks/$HOOK, that
repository's $HOOK will be substituted for the one in the current
project, with unknown consequences.
This lookup failure also occurs in worktrees linked to a remote .git
directory using git-new-workdir. However, a worktree using a .git file
pointing to a separate git directory apparently avoids this: in that
case the hook command is resolved to an absolute path before being
passed down to the code introduced in aae9560a.
Fix this by replacing the test for an "absolute" pathname to a check for
a command name having more than one pathname component. This limits the
search and absolute pathname resolution to bare commands. The new test
uses tcl's "file split" command. Experiments on Linux and Windows, using
tclsh, show that command names with relative and absolute paths always
give at least two components, while a bare command gives only one.
Linux: puts [file split {foo}] ==> foo
Linux: puts [file split {/foo}] ==> / foo
Linux: puts [file split {.git/foo}] ==> .git foo
Windows: puts [file split {foo}] ==> foo
Windows: puts [file split {c:\foo}] ==> c:/ foo
Windows: puts [file split {.git\foo}] ==> .git foo
The above results show the new test limits search and replacement
to bare commands on both Linux and Windows.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-gui.sh b/git-gui.sh
index 8bc8892..8603437 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {
set i $from_index
while {$i < [llength $command_line]} {
set cmd [lindex $command_line $i]
- if {[file pathtype $cmd] ne "absolute"} {
+ if {[llength [file split $cmd]] < 2} {
set fullpath [_which $cmd]
if {$fullpath eq ""} {
throw {NOT-FOUND} "$cmd not found in PATH"
--
2.41.0.99.19
^ permalink raw reply related
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Sergey Organov @ 2023-09-16 18:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzg1nfixw.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> I don't see why desire to look at diff-to-first-parent on "side"
>> branches is any different from desire to look at them on "primary"
>> branch
>
> Yeah, but that is not what I meant. The above argues for why
> "--diff-merges=first-parent" should exist independently from the
> "--first-parent" traversal *and* display option. I am not saying
> it should not exist.
I was not assuming you were saying this, as it has been discussed and
agreed upon when --diff-merges=first-parent was introduced, though I
think I now see your point more clearly.
>
> But I view that the desire to look at any commits and its changes on
> the "side" branch at all *is* at odds with the wish to look at
> first-parent change for merge commits.
I think I do now understand what you mean, yet I have alternative view
on the issue.
> Once you decide to look at first-parent change for a merge commit,
> then every change you see for each commit on the "side" branch,
> whether it is shown as first-parent diff or N pairwise diffs, is what
> you have already seen in the change in the merge commit,
Actually, this happens to be exactly one of intended use-cases for "-d".
It's useful to see how some change introduced by the merge looked in the
context of the original commit, or to figure where the change came from.
> because "git log" goes newer to older, and the commits on the side
> branches appear after the merge that brings them to the mainline.
The exact order is orthogonal to the issue at hands, I think.
> Making "log -d" mean "log --diff-merges=first-parent --patch" lets
> that less useful combination ("show first-parent patches but
> traverse side branches as well") squat on the short and sweet "-d"
> that could be used for more useful "log --first-parent --patch",
> which would also be more common and intuitive to users, and that is
> what I suspect will become problematic in the longer run.
Sorry, "-d ≡ --first-parent --patch" you suggest contradicts my view on
the whole scheme of things, for several reasons:
* I still find it problematic if -d, intended to fit nicely among --cc,
-c, -d, -m, -p, --remerge-diff options, suddenly implies --first-parent.
This would bring yet another inconsistency, and I don't want to be the
one who introduced it.
* In its current state -d conveniently means: "gimme simple diff output
for everything", where --first-parent you suggest doesn't fit at all.
* Current -d implementation is semantically as close to -p as possible,
tweaking exactly one thing compared to -p: the format of output for
merge commits, so is simpler than what you suggest from all angles, as
--first-parent tweaks more than one thing.
* To me what you argue for looks mostly like a desire to have a
short-cut for "--first-parent --patch", and my patch in question does
not seem to contradict this desire, as it'd be very surprising if
somebody came up with the name "-d" for such a short-cut. Definitely not
me.
* Finally, if -d becomes "--patch --first-parent", how do I get back
useful "--patch --diff-merges=first-parent" part of it, provided
--first-parent is unreversable? And even if it were reversable, then
git log -d --no-first-parent =
git log --patch --first-parent --no-first-parent =
git log --patch
is definitely not what is needed, nor frequent demand to revert implied
things indicates optimal design. Compare this to
git log -d --first-parent
that current -d provides for you to get what you need, and that
unambiguously reads: "gimme *d*iff for all commits while following
*first parent* through the history" (while, unlike, -p not requiring
--first-parent to implicitly tweak diff for merges output).
Overall, after considering your concern, I'd still prefer to leave "-d"
semantics as implemented, consistent with the rest of similar options,
and let somebody else define more shortcuts for their frequent use-cases
if they feel like it.
Thanks,
-- Sergey Organov
P.S. I also figure that maybe our divergence comes from the fact that I
consider merge commits to be primarily commits (introducing particular
set of changes, and then having reference to the source of the changes),
whereas you consider them primarily merges (joining two histories, and
then maybe some artificial changes that make merges "evil"). That's why
we often end up agreeing to disagree, as both these points of view seem
pretty valid.
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Junio C Hamano @ 2023-09-16 17:45 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <20230912084029.GD1630538@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> True. Though I don't fully understand these warnings (why not then
>> also warn about if without else?), but taking them away is a bit rude
>> to those who care.
>
> I think losing warnings is unfortunate, but it's just one example.
> We're losing the type information completely from the values.
> ...
>> Or to use an int to point to and then copy into a companion enum
>> variable to after parsing, which would be my choice.
>
> Yeah, I had the same thought. I'm just not sure how to do that in a way
> that isn't a pain for the callers.
The discussion seems to have petered out around this point.
What (if anything) do we want to do with this topic?
^ permalink raw reply
* Re: [PATCH] git-config: fix misworded --type=path explanation
From: Junio C Hamano @ 2023-09-16 17:45 UTC (permalink / raw)
To: Evan Gates; +Cc: git
In-Reply-To: <20230915202610.21206-1-evan.gates@gmail.com>
Evan Gates <evan.gates@gmail.com> writes:
> When `--type=<type>` was added as a prefered alias for `--<type>`
> the explanation for the path type was reworded. Whereas the previous
> explanation said "expand a leading `~`" this was changed to "adding a
> leading `~`". Change "adding" to "expanding" to correctly explain the
> canonicalization.
Nice spotting (and good archaeology, too).
Thanks.
^ permalink raw reply
* Re: [PATCH] git-gui - re-enable use of hook scripts
From: Junio C Hamano @ 2023-09-16 17:28 UTC (permalink / raw)
To: Mark Levedahl; +Cc: johannes.schindelin, me, git
In-Reply-To: <20230916003516.51053-1-mlevedahl@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> Commit aae9560a introduced search in $PATH to find executables before
> running them, avoiding an issue where on Windows a same named file in
> the current directory can be executed in preference to anything on the
> path. The updated search excludes files given with an absolute path (e.g.,
> /bin/sh). However this change precludes operation of hook scripts as these
> are named with a relative path (.git/hooks/$HOOK), while a search on $PATH
> can succeed only for bare file names, not relative paths. Furthermore,
> the current repository's .git/hooks directory is in general not listed
> in PATH.
>
> Fix this by changing the "absolute" check to a check for more than one
> component in the pathname, thereby avoiding the PATH check for anything
> given with a relative path as well. Bare "git" has one component, "/sh"
> has two components, and .git/hooks/$HOOK has more than two, so relative
> and absolute pathnames avoid the check.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
With your experiments in the other thread, I think this is quite a
reasonable fix to the problem. I'd prefer a few updates to the
proposed log message above, though.
* Refer the older commit like so:
Earlier, aae9560a (Work around Tcl's default `PATH` lookup,
2022-11-23) introduced ...
* It would help readers if you clarify that "The updated search
excludes ..." and the rest of that paragraph of the log gives a
bug/problem/undesirable behaviour of the current code introduced
by the earlier change. Perhaps something along the lines of ...
The updated search excludes commands given as an absolute
path (e.g., /bin/sh), which is good, but it also tries to
find commands given as a path relative to the current
directory with directory separator (e.g.,
.git/hooks/pre-commit), which makes the hooks from running
at all. We only want to apply the $PATH logic to a token
without any directory separator in it.
* Mention that we already know the new logic works for absolute
paths even on Windows by tweaking the sentence starting with
"Bare 'git' has ...". Something like:
Bare "git" has one component (which we want to use $PATH),
"/bin/sh", "C:\some\command", and ".git/hooks/$HOOK" all
split into 2 or more (which we want to use as-is). The only
case we want to use $PATH is when result of [file split] has
only one element.
But other than that it looks good.
Dscho?
> git-gui.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8bc8892..8603437 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {
> set i $from_index
> while {$i < [llength $command_line]} {
> set cmd [lindex $command_line $i]
> - if {[file pathtype $cmd] ne "absolute"} {
> + if {[llength [file split $cmd]] < 2} {
> set fullpath [_which $cmd]
> if {$fullpath eq ""} {
> throw {NOT-FOUND} "$cmd not found in PATH"
^ permalink raw reply
* Re: BUG: git-gui no longer executes hook scripts
From: Junio C Hamano @ 2023-09-16 17:31 UTC (permalink / raw)
To: Mark Levedahl; +Cc: git, johannes.schindelin, me
In-Reply-To: <2ce41212-41e7-fe5f-cc9f-bcfaa2641e59@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> On 9/16/23 08:56, Mark Levedahl wrote:
>>
>>
>>
>> So, there is hope c:\foo will split into c: foo, or c:\ foo, but
>> testing on Windows is needed. Really need Dscho or someone else from
>> g4w to help out here.
>>
>>
> I did install git for windows into a bare VM, running tclsh.exe on that
>
>
> puts [file split {c:\foo}]
> c:/ foo
Great. That is exactly what we want to see. Thanks.
^ permalink raw reply
* Re: BUG: git-gui no longer executes hook scripts
From: Mark Levedahl @ 2023-09-16 14:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, me
In-Reply-To: <ffd5e1dc-bad7-2b1d-3344-76ffeb2858f5@gmail.com>
On 9/16/23 08:56, Mark Levedahl wrote:
>
>
>
> So, there is hope c:\foo will split into c: foo, or c:\ foo, but
> testing on Windows is needed. Really need Dscho or someone else from
> g4w to help out here.
>
>
I did install git for windows into a bare VM, running tclsh.exe on that
puts [file split {c:\foo}]
c:/ foo
puts [llength [file split {c:\foo}]]
2
puts [file split {hooks\foo}]
hooks foo
puts [llength [file split {hooks\foo}]]
2
puts [file split {foo}]
foo
puts [llength [file split {foo}]]
1
So, file split seems to work as needed on Windows.
^ permalink raw reply
* Re: [PATCH] completion: commit: complete configured trailer tokens
From: ZheNing Hu @ 2023-09-16 13:30 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <pull.1583.git.1694108551683.gitgitgadget@gmail.com>
Thank you for the improvement, I believe this interactive mode with tab
completion will be very useful.
Philippe Blain via GitGitGadget <gitgitgadget@gmail.com> 于2023年9月8日周五 01:42写道：
>
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Since 2daae3d1d1 (commit: add --trailer option, 2021-03-23), 'git
> commit' can add trailers to commit messages. To make that feature more
> pleasant to use at the command line, update the Bash completion code to
> offer configured trailer tokens.
>
> Add a __git_trailer_tokens function to list the configured trailers
> tokens, and use it in _git_commit to suggest the configured tokens,
> suffixing the completion words with ':' so that the user only has to add
> the trailer value.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> completion: commit: complete configured trailer tokens
>
> Since 2daae3d1d1 (commit: add --trailer option, 2021-03-23), 'git
> commit' can add trailers to commit messages. To make that feature more
> pleasant to use at the command line, update the Bash completion code to
> offer configured trailer tokens.
>
> Add a __git_trailer_tokens function to list the configured trailers
> tokens, and use it in _git_commit to suggest the configured tokens,
> suffixing the completion words with ':' so that the user only has to add
> the trailer value.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1583%2Fphil-blain%2Fcompletion-commit-trailers-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1583/phil-blain/completion-commit-trailers-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1583
>
> contrib/completion/git-completion.bash | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 133ec92bfae..b5eb75aadc5 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1677,6 +1677,11 @@ _git_clone ()
>
> __git_untracked_file_modes="all no normal"
>
> +__git_trailer_tokens ()
> +{
> + git config --name-only --get-regexp trailer.\*.key | awk -F. '{print $2}'
> +}
> +
> _git_commit ()
> {
> case "$prev" in
> @@ -1701,6 +1706,10 @@ _git_commit ()
> __gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
> return
> ;;
> + --trailer=*)
> + __gitcomp_nl "$(__git_trailer_tokens)" "" "${cur##--trailer=}" ":"
> + return
> + ;;
> --*)
> __gitcomp_builtin commit
> return
>
> base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
> --
> gitgitgadget
^ permalink raw reply
* Re: BUG: git-gui no longer executes hook scripts
From: Mark Levedahl @ 2023-09-16 12:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, me
In-Reply-To: <xmqqil8ad8un.fsf@gitster.g>
On 9/16/23 00:45, Junio C Hamano wrote:
> Mark Levedahl <mlevedahl@gmail.com> writes:
>
>> I think a simpler fix is just to examine the number of path components
>> - more than one means a relative or absolute command (/foo splits into
>> two parts). The below works for me on Linux.
> That is clever, but I cannot convince myself that it is not too
> clever for its own sake. The "pathtype" thing Dscho used in his
> original is documented to be aware of things like "C:\path\name",
> but I didn't re-read the Tcl manual page too carefully to know what
> "file split" does for such pathname to be certain.
>
The manual does not talk about Windows explicitly. From
https://www.tcl.tk/man/tcl/TclCmd/file.html#M35
*file split */name/
Returns a list whose elements are the path components in /name/. The
first element of the list will have the same path type as /name/.
All other elements will be relative. Path separators will be
discarded unless they are needed to ensure that an element is
unambiguously relative. For example, under Unix
*file split* /foo/~bar/baz
returns “*/ foo ./~bar baz*” to ensure that later commands that use
the third component do not attempt to perform tilde substitution.
So, there is hope c:\foo will split into c: foo, or c:\ foo, but testing
on Windows is needed. Really need Dscho or someone else from g4w to help
out here.
^ permalink raw reply
* Re: [PATCH v5] merge-tree: add -X strategy option
From: 唐宇奕 @ 2023-09-16 8:38 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren, Jeff King
In-Reply-To: <pull.1565.v5.git.1694853437494.gitgitgadget@gmail.com>
Thank you for your advice! I've uploaded new patch.
On Sat, Sep 16, 2023 at 4:37 PM Izzy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
>
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---
> merge-tree: add -X strategy option
>
> Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v4:
>
> 1: d2d8fcc2e9b ! 1: 28d4282e0d8 merge-tree: add -X strategy option
> @@ Commit message
>
> ## builtin/merge-tree.c ##
> @@
> + #include "quote.h"
> #include "tree.h"
> #include "config.h"
> ++#include "strvec.h"
>
> -+static const char **xopts;
> -+static size_t xopts_nr, xopts_alloc;
> static int line_termination = '\n';
>
> - struct merge_list {
> @@ builtin/merge-tree.c: struct merge_tree_options {
> int show_messages;
> int name_only;
> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
>
> opt.branch1 = branch1;
> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> - return !result.clean; /* result.clean < 0 handled above */
> - }
> -
> -+static int option_parse_x(const struct option *opt,
> -+ const char *arg, int unset)
> -+{
> -+ if (unset)
> -+ return 0;
> -+
> -+ ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
> -+ xopts[xopts_nr++] = xstrdup(arg);
> -+ return 0;
> -+}
> -+
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> struct merge_tree_options o = { .show_messages = -1 };
> ++ struct strvec xopts = STRVEC_INIT;
> + int expected_remaining_argc;
> + int original_argc;
> + const char *merge_base = NULL;
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> &merge_base,
> N_("commit"),
> N_("specify a merge-base for the merge")),
> -+ OPT_CALLBACK('X', "strategy-option", &xopts,
> -+ N_("option=value"),
> -+ N_("option for selected merge strategy"),
> -+ option_parse_x),
> ++ OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> ++ N_("option for selected merge strategy")),
> OPT_END()
> };
>
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>
> -+ for (int x = 0; x < xopts_nr; x++)
> -+ if (parse_merge_opt(&o.merge_options, xopts[x]))
> -+ die(_("unknown strategy option: -X%s"), xopts[x]);
> ++ for (int x = 0; x < xopts.nr; x++)
> ++ if (parse_merge_opt(&o.merge_options, xopts.v[x]))
> ++ die(_("unknown strategy option: -X%s"), xopts.v[x]);
> +
> /* Handle --stdin */
> if (o.use_stdin) {
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> -+ git rev-parse HEAD^{tree} > expected &&
> ++ git rev-parse HEAD^{tree} >expected &&
> +
> -+ git merge-tree -X ours side1 side4 > actual &&
> ++ git merge-tree -X ours side1 side4 >actual &&
> +
> + test_cmp expected actual
> +'
>
>
> builtin/merge-tree.c | 11 +++++++++++
> t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..97d0fe6c952 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -18,6 +18,7 @@
> #include "quote.h"
> #include "tree.h"
> #include "config.h"
> +#include "strvec.h"
>
> static int line_termination = '\n';
>
> @@ -414,6 +415,7 @@ struct merge_tree_options {
> int show_messages;
> int name_only;
> int use_stdin;
> + struct merge_options merge_options;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ -439,6 +441,8 @@ static int real_merge(struct merge_tree_options *o,
>
> init_merge_options(&opt, the_repository);
>
> + opt.recursive_variant = o->merge_options.recursive_variant;
> +
> opt.show_rename_progress = 0;
>
> opt.branch1 = branch1;
> @@ -513,6 +517,7 @@ static int real_merge(struct merge_tree_options *o,
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> struct merge_tree_options o = { .show_messages = -1 };
> + struct strvec xopts = STRVEC_INIT;
> int expected_remaining_argc;
> int original_argc;
> const char *merge_base = NULL;
> @@ -548,6 +553,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> &merge_base,
> N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> + N_("option for selected merge strategy")),
> OPT_END()
> };
>
> @@ -556,6 +563,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>
> + for (int x = 0; x < xopts.nr; x++)
> + if (parse_merge_opt(&o.merge_options, xopts.v[x]))
> + die(_("unknown strategy option: -X%s"), xopts.v[x]);
> +
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 250f721795b..b2c8a43fce3 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -22,6 +22,7 @@ test_expect_success setup '
> git branch side1 &&
> git branch side2 &&
> git branch side3 &&
> + git branch side4 &&
>
> git checkout side1 &&
> test_write_lines 1 2 3 4 5 6 >numbers &&
> @@ -46,6 +47,13 @@ test_expect_success setup '
> test_tick &&
> git commit -m rename-numbers &&
>
> + git checkout side4 &&
> + test_write_lines 0 1 2 3 4 5 >numbers &&
> + echo yo >greeting &&
> + git add numbers greeting &&
> + test_tick &&
> + git commit -m other-content-modifications &&
> +
> git switch --orphan unrelated &&
> >something-else &&
> git add something-else &&
> @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
> test_cmp expect actual
> '
>
> +test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
> + git checkout side1^0 &&
> +
> + # make sure merge conflict exists
> + test_must_fail git merge side4 &&
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> + git rev-parse HEAD^{tree} >expected &&
> +
> + git merge-tree -X ours side1 side4 >actual &&
> +
> + test_cmp expected actual
> +'
> +
> test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
> # Mis-spell with single "s" instead of double "s"
> test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
>
> base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
> --
> gitgitgadget
^ permalink raw reply
* [PATCH v5] merge-tree: add -X strategy option
From: Izzy via GitGitGadget @ 2023-09-16 8:37 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Jeff King, Izzy, Tang Yuyi
In-Reply-To: <pull.1565.v4.git.1694836025469.gitgitgadget@gmail.com>
From: Tang Yuyi <winglovet@gmail.com>
Add merge strategy option to produce more customizable merge result such
as automatically resolving conflicts.
Signed-off-by: Tang Yuyi <winglovet@gmail.com>
---
merge-tree: add -X strategy option
Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1565
Range-diff vs v4:
1: d2d8fcc2e9b ! 1: 28d4282e0d8 merge-tree: add -X strategy option
@@ Commit message
## builtin/merge-tree.c ##
@@
+ #include "quote.h"
#include "tree.h"
#include "config.h"
++#include "strvec.h"
-+static const char **xopts;
-+static size_t xopts_nr, xopts_alloc;
static int line_termination = '\n';
- struct merge_list {
@@ builtin/merge-tree.c: struct merge_tree_options {
int show_messages;
int name_only;
@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
opt.branch1 = branch1;
@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
- return !result.clean; /* result.clean < 0 handled above */
- }
-
-+static int option_parse_x(const struct option *opt,
-+ const char *arg, int unset)
-+{
-+ if (unset)
-+ return 0;
-+
-+ ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-+ xopts[xopts_nr++] = xstrdup(arg);
-+ return 0;
-+}
-+
int cmd_merge_tree(int argc, const char **argv, const char *prefix)
{
struct merge_tree_options o = { .show_messages = -1 };
++ struct strvec xopts = STRVEC_INIT;
+ int expected_remaining_argc;
+ int original_argc;
+ const char *merge_base = NULL;
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
-+ OPT_CALLBACK('X', "strategy-option", &xopts,
-+ N_("option=value"),
-+ N_("option for selected merge strategy"),
-+ option_parse_x),
++ OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
++ N_("option for selected merge strategy")),
OPT_END()
};
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
-+ for (int x = 0; x < xopts_nr; x++)
-+ if (parse_merge_opt(&o.merge_options, xopts[x]))
-+ die(_("unknown strategy option: -X%s"), xopts[x]);
++ for (int x = 0; x < xopts.nr; x++)
++ if (parse_merge_opt(&o.merge_options, xopts.v[x]))
++ die(_("unknown strategy option: -X%s"), xopts.v[x]);
+
/* Handle --stdin */
if (o.use_stdin) {
@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c
+ git merge --abort &&
+
+ git merge -X ours side4 &&
-+ git rev-parse HEAD^{tree} > expected &&
++ git rev-parse HEAD^{tree} >expected &&
+
-+ git merge-tree -X ours side1 side4 > actual &&
++ git merge-tree -X ours side1 side4 >actual &&
+
+ test_cmp expected actual
+'
builtin/merge-tree.c | 11 +++++++++++
t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 0de42aecf4b..97d0fe6c952 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "tree.h"
#include "config.h"
+#include "strvec.h"
static int line_termination = '\n';
@@ -414,6 +415,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
+ struct merge_options merge_options;
};
static int real_merge(struct merge_tree_options *o,
@@ -439,6 +441,8 @@ static int real_merge(struct merge_tree_options *o,
init_merge_options(&opt, the_repository);
+ opt.recursive_variant = o->merge_options.recursive_variant;
+
opt.show_rename_progress = 0;
opt.branch1 = branch1;
@@ -513,6 +517,7 @@ static int real_merge(struct merge_tree_options *o,
int cmd_merge_tree(int argc, const char **argv, const char *prefix)
{
struct merge_tree_options o = { .show_messages = -1 };
+ struct strvec xopts = STRVEC_INIT;
int expected_remaining_argc;
int original_argc;
const char *merge_base = NULL;
@@ -548,6 +553,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
+ OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ N_("option for selected merge strategy")),
OPT_END()
};
@@ -556,6 +563,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+ for (int x = 0; x < xopts.nr; x++)
+ if (parse_merge_opt(&o.merge_options, xopts.v[x]))
+ die(_("unknown strategy option: -X%s"), xopts.v[x]);
+
/* Handle --stdin */
if (o.use_stdin) {
struct strbuf buf = STRBUF_INIT;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 250f721795b..b2c8a43fce3 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -22,6 +22,7 @@ test_expect_success setup '
git branch side1 &&
git branch side2 &&
git branch side3 &&
+ git branch side4 &&
git checkout side1 &&
test_write_lines 1 2 3 4 5 6 >numbers &&
@@ -46,6 +47,13 @@ test_expect_success setup '
test_tick &&
git commit -m rename-numbers &&
+ git checkout side4 &&
+ test_write_lines 0 1 2 3 4 5 >numbers &&
+ echo yo >greeting &&
+ git add numbers greeting &&
+ test_tick &&
+ git commit -m other-content-modifications &&
+
git switch --orphan unrelated &&
>something-else &&
git add something-else &&
@@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' '
test_cmp expect actual
'
+test_expect_success 'Auto resolve conflicts by "ours" strategy option' '
+ git checkout side1^0 &&
+
+ # make sure merge conflict exists
+ test_must_fail git merge side4 &&
+ git merge --abort &&
+
+ git merge -X ours side4 &&
+ git rev-parse HEAD^{tree} >expected &&
+
+ git merge-tree -X ours side1 side4 >actual &&
+
+ test_cmp expected actual
+'
+
test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' '
# Mis-spell with single "s" instead of double "s"
test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&
base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 5/4] merge-ort: lowercase a few error messages
From: Jeff King @ 2023-09-16 7:29 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Junio C Hamano
In-Reply-To: <20230916060059.GA498798@coredump.intra.peff.net>
On Sat, Sep 16, 2023 at 02:01:00AM -0400, Jeff King wrote:
> Here's one more clean-up on top. I hesitated on this for the initial
> send just because I didn't know if we might want to switch these error
> messages to path_msg(), which does capitalize sometimes. But Elijah's
> response convinced me that we should just leave them in place, in which
> case it makes sense to do a minimal style fixup.
>
> Junio, this is on top of what you've queued in
> jk/ort-unused-parameter-cleanups.
>
> -- >8 --
> Subject: [PATCH] merge-ort: lowercase a few error messages
>
> As noted in CodingGuidelines, error messages should not be capitalized.
> Fix up a few of these that were copied verbatim from merge-recursive to
> match our modern style.
<sigh> This fails CI because with GIT_TEST_MERGE_ALGORITHM=recursive, we
run the old merge-recursive code, which uses the capitalized version.
I'm inclined to just drop this minor cleanup for now, and we can worry
about it later once merge-recursive goes the way of the dodo.
-Peff
^ permalink raw reply
* Re: [PATCH] git-config: fix misworded --type=path explanation
From: Jeff King @ 2023-09-16 6:16 UTC (permalink / raw)
To: Evan Gates; +Cc: git
In-Reply-To: <20230915202610.21206-1-evan.gates@gmail.com>
On Fri, Sep 15, 2023 at 02:24:59PM -0600, Evan Gates wrote:
> When `--type=<type>` was added as a prefered alias for `--<type>`
> the explanation for the path type was reworded. Whereas the previous
> explanation said "expand a leading `~`" this was changed to "adding a
> leading `~`". Change "adding" to "expanding" to correctly explain the
> canonicalization.
Yeah, the sentence as-is does not make any sense. Your suggested wording
looks good to me. Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH v4] merge-tree: add -X strategy option
From: Jeff King @ 2023-09-16 6:11 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren, Izzy
In-Reply-To: <pull.1565.v4.git.1694836025469.gitgitgadget@gmail.com>
On Sat, Sep 16, 2023 at 03:47:05AM +0000, Izzy via GitGitGadget wrote:
> +static int option_parse_x(const struct option *opt,
> + const char *arg, int unset)
> +{
> + if (unset)
> + return 0;
> +
> + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
> + xopts[xopts_nr++] = xstrdup(arg);
> + return 0;
> +}
This callback was presumably copied from the one in builtin/merge.c, and
it suffers from the same "--no-strategy-option" bug. You should make a
similar change here to the one we did in dee02da826 (merge: make xopts a
strvec, 2023-08-31). And as a bonus, it will make your patch even
shorter. :)
It would also make it easier to get rid of the global variables, I
think. Something like (squashed into your patch):
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 2ec6ec0d39..e13dbc4c79 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,9 +18,8 @@
#include "quote.h"
#include "tree.h"
#include "config.h"
+#include "strvec.h"
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
static int line_termination = '\n';
struct merge_list {
@@ -515,20 +514,10 @@ static int real_merge(struct merge_tree_options *o,
return !result.clean; /* result.clean < 0 handled above */
}
-static int option_parse_x(const struct option *opt,
- const char *arg, int unset)
-{
- if (unset)
- return 0;
-
- ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
- xopts[xopts_nr++] = xstrdup(arg);
- return 0;
-}
-
int cmd_merge_tree(int argc, const char **argv, const char *prefix)
{
struct merge_tree_options o = { .show_messages = -1 };
+ struct strvec xopts = STRVEC_INIT;
int expected_remaining_argc;
int original_argc;
const char *merge_base = NULL;
@@ -564,10 +553,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
- OPT_CALLBACK('X', "strategy-option", &xopts,
- N_("option=value"),
- N_("option for selected merge strategy"),
- option_parse_x),
+ OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ N_("option for selected merge strategy")),
OPT_END()
};
@@ -576,9 +563,9 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
- for (int x = 0; x < xopts_nr; x++)
- if (parse_merge_opt(&o.merge_options, xopts[x]))
- die(_("unknown strategy option: -X%s"), xopts[x]);
+ for (int x = 0; x < xopts.nr; x++)
+ if (parse_merge_opt(&o.merge_options, xopts.v[x]))
+ die(_("unknown strategy option: -X%s"), xopts.v[x]);
/* Handle --stdin */
if (o.use_stdin) {
^ permalink raw reply related
* [PATCH 5/4] merge-ort: lowercase a few error messages
From: Jeff King @ 2023-09-16 6:00 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Junio C Hamano
In-Reply-To: <20230914093409.GA2254811@coredump.intra.peff.net>
On Thu, Sep 14, 2023 at 05:34:09AM -0400, Jeff King wrote:
> A few small cleanups for merge-ort collected from playing with
> -Wunused-parameter. The first one actually fixes a user-visible bug, and
> the rest are just cleanups.
>
> [1/4]: merge-ort: drop custom err() function
> [2/4]: merge-ort: stop passing "opt" to read_oid_strbuf()
> [3/4]: merge-ort: drop unused parameters from detect_and_process_renames()
> [4/4]: merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
Here's one more clean-up on top. I hesitated on this for the initial
send just because I didn't know if we might want to switch these error
messages to path_msg(), which does capitalize sometimes. But Elijah's
response convinced me that we should just leave them in place, in which
case it makes sense to do a minimal style fixup.
Junio, this is on top of what you've queued in
jk/ort-unused-parameter-cleanups.
-- >8 --
Subject: [PATCH] merge-ort: lowercase a few error messages
As noted in CodingGuidelines, error messages should not be capitalized.
Fix up a few of these that were copied verbatim from merge-recursive to
match our modern style.
Signed-off-by: Jeff King <peff@peff.net>
---
merge-ort.c | 4 ++--
t/t6406-merge-attr.sh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 3953c9f745..7857ce9fbd 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2105,12 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
&result_buf);
if ((merge_status < 0) || !result_buf.ptr)
- ret = error(_("Failed to execute internal merge"));
+ ret = error(_("failed to execute internal merge"));
if (!ret &&
write_object_file(result_buf.ptr, result_buf.size,
OBJ_BLOB, &result->oid))
- ret = error(_("Unable to add %s to database"), path);
+ ret = error(_("unable to add %s to database"), path);
free(result_buf.ptr);
if (ret)
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 05ad13b23e..72f8c1722f 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -180,7 +180,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
>./please-abort &&
echo "* merge=custom" >.gitattributes &&
test_must_fail git merge main 2>err &&
- grep "^error: Failed to execute internal merge" err &&
+ grep "^error: failed to execute internal merge" err &&
git ls-files -u >output &&
git diff --name-only HEAD >>output &&
test_must_be_empty output
--
2.42.0.661.g2507eb519e
^ permalink raw reply related
* Re: [PATCH 4/4] merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
From: Jeff King @ 2023-09-16 5:52 UTC (permalink / raw)
To: Elijah Newren; +Cc: git
In-Reply-To: <CABPp-BEi1CSXEE=-dDi_bhtSsGeVWtivfT-jQP+hjWdv4agq=Q@mail.gmail.com>
On Fri, Sep 15, 2023 at 08:09:00PM -0700, Elijah Newren wrote:
> On Thu, Sep 14, 2023 at 2:40 AM Jeff King <peff@peff.net> wrote:
> >
> > The merge_options parameter has never been used since the function was
> > introduced in 64aceb6d73 (merge-ort: add code to check for whether
> > cached renames can be reused, 2021-05-20). In theory some merge options
> > might impact our decisions here, but that has never been the case so
> > far.
>
> Yeah, it was used in some preliminary versions of the code while I was
> developing the new algorithm, but there were lots of changes between
> when I started working on merge-ort and when it was finally ready to
> submit for review. I must have just overlooked that this parameter
> was no longer needed. Thanks for catching and cleaning up.
Yeah, that's what I figured. I actually queued quite a few of these
-Wunused-parameter fixups, because the initial iterations of merge-ort
had a lot of stub functions or unimplemented bits. I sat on them for a
year or so because I figured you'd eventually use those parameters. And
indeed, most of them fell out naturally, and what was left for this
series was all pretty easy to understand.
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] merge-ort: drop custom err() function
From: Jeff King @ 2023-09-16 5:50 UTC (permalink / raw)
To: Elijah Newren; +Cc: git
In-Reply-To: <CABPp-BEhgZB3Q5VKTznOFwt2+Ptcf6ffyJSbXXnmoa_4_zRAVg@mail.gmail.com>
On Fri, Sep 15, 2023 at 07:54:28PM -0700, Elijah Newren wrote:
> Oops, when I simplified the err() function copied from
> merge-recursive.c in one way, I failed to notice that it enabled
> further simplifications.
Ah, I didn't even realize that it had been copied from there. That makes
a lot more sense now.
> > 2. It formats the error string into a strbuf, prepending "error: ",
> > and then feeds the result into error(). But this is wrong! The
> > error() function already adds the prefix, so we end up with:
> >
> > error: error: Failed to execute internal merge
>
> ...and the same problem can be found in merge-recursive.c's err() function.
>
> Not sure what current opinions on whether we should bother fixing
> those up. I do intend on nuking merge-recursive.c, but I obviously
> haven't had much Git time this year.
Hmm, I'm not sure that it does. It has this code:
if (opt->buffer_output < 2)
flush_output(opt);
else {
strbuf_complete(&opt->obuf, '\n');
strbuf_addstr(&opt->obuf, "error: ");
}
so we only add the extra "error:" tag when we are in a buffering mode
that writes into the strbuf (indicated by a high buffer_output field).
And then later, after formatting the new string into opt->obuf, we do
this:
if (opt->buffer_output > 1)
strbuf_addch(&opt->obuf, '\n');
else {
error("%s", opt->obuf.buf);
strbuf_reset(&opt->obuf);
}
So we call error() iff buffer is low, in which case we would not have
added the prefix earlier. And that makes sense. If we are collecting
messages in obuf, we cannot use error(), and we have to handle the
prefix ourselves.
So I think it's correct? If you started with merge-recursive's err() and
then stripped it down to remove the extra buffering stuff, I can see
that it would be a natural error to accidentally leave in the extra
prefix while doing so.
I certainly find the code confusing (the split "< 2" and "> 1"
conditions to trigger related cases is a nice obfuscation bonus), but I
_think_ it's right. And if the end goal is to ditch merge-recursive.c, I
don't think it's worth spending any more brain power on it.
> > A few of these messages starts with capital letters, which is unlike our
> > usual error message style. I didn't clean that up here. We could do so
> > on top,
>
> There are two of these. In my defense, they were copied verbatim from
> merge-recursive.c. And I, um, never noticed the problem over there
> before copying. Or after.
That makes sense. We have a ton of these lurking in the code base. I
usually try to clean them up when I touch relevant lines, but I wasn't
sure about the path_msg thing here.
> Yeah, all callers of err()/error() are for things that should never
> happen regardless of repository contents and should result in an
> instant abort, whereas anything calling path_msg() is a conflict or
> informational message that is expected for various kinds of repository
> data -- these messages are accumulated and later shown.
That makes sense in general. And I think most of these err() calls are
of that form (in fact, I'd expect most corruption to just trigger a
die() at a low level anyway, but we've been slowly lib-ifying that
away).
The one that gave me pause was my "the external merge driver gave us a
failure" case that I used for testing. In theory we could say "oops,
your merge driver is broken" and keep going. But I think it's not just
"broken", but "oops, your merge driver died with a signal", since a
non-zero exit just means conflicts. So at that point probably something
really has gone terribly wrong (not necessarily with Git, but with your
merge driver!).
> Another distinction is that any call to path_msg() is associated to a
> very specific path (or a few specific paths in special cases like
> renames or add/add with conflict modes), whereas none of the calls to
> err()/error() have a specific path they are about. This serves a few
> purposes:
I think some of them are about specific paths. We hit err() if
merge_3way() returns -1, but that call can of course also result in a
regular conflict.
But if they're catastrophic errors anyway (as above), this part is kind
of moot.
> Anyway, long story short is that I think continuing to use error()
> instead of path_msg() or something else makes sense here. The capital
> to lowercase cleanups make sense; we could even #leftoverbits for that
> piece.
Sounds good. I'll post a patch in a second, just to take care of it
while we're thinking about it.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Jeff King @ 2023-09-16 5:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Rubén Justo, Git List,
Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqq5y4bfhpm.fsf@gitster.g>
On Fri, Sep 15, 2023 at 10:51:17AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > But having done so, the main value in re-rolling would be preventing
> > somebody else from reading the code and having the same question.
>
> Indeed. It would be valuable to help future developers not to waste
> time on wondering what we already know they may do so on.
I guess I was thinking "maybe that is not a good reason to change what
the code does" when I wrote my other email. But it is probably a good
reason to make a note in the commit message. :)
-Peff
^ 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